[RFC] criu: add a debug version of rst-malloc

Submitted by Andrei Vagin on March 30, 2017, 5:30 a.m.

Details

Message ID 1490851850-8153-1-git-send-email-avagin@openvz.org
State New
Series "criu: add a debug version of rst-malloc"
Headers show

Commit Message

Andrei Vagin March 30, 2017, 5:30 a.m.
From: Andrei Vagin <avagin@virtuozzo.com>

rst_mem_alloc() can move all allocated objects and
the problem is that we don't know when it will do.

With this patch, if the criu is built with DEBUG=1,
rst_mem_alloc() will move objects each time.

Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 .travis.yml                 |  1 +
 criu/rst-malloc.c           | 19 +++++++++++++++++++
 scripts/travis/travis-tests |  2 +-
 3 files changed, 21 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/.travis.yml b/.travis.yml
index a5dfb9f..d62e086 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -7,6 +7,7 @@  services:
 env:
   - TR_ARCH=local       GCOV=1
   - TR_ARCH=local       CLANG=1
+  - TR_ARCH=local       CRIU_DEBUG=1
   - TR_ARCH=alpine
   - TR_ARCH=x86_64
   - TR_ARCH=armv7hf
diff --git a/criu/rst-malloc.c b/criu/rst-malloc.c
index 4f7357c..057fa31 100644
--- a/criu/rst-malloc.c
+++ b/criu/rst-malloc.c
@@ -23,6 +23,11 @@  static inline unsigned long rst_mem_grow(unsigned long need_size)
 {
 	int rst_mem_batch = 2 * page_size();
 
+#ifdef CR_DEBUG
+	if (need_size == 0)
+		return 0;
+#endif
+
 	need_size = round_up(need_size, page_size());
 	if (likely(need_size < rst_mem_batch))
 		need_size = rst_mem_batch;
@@ -88,8 +93,18 @@  static int grow_remap(struct rst_mem_type_s *t, int flag, unsigned long size)
 		 * a completely new one and force callers use objects'
 		 * cpos-s.
 		 */
+#ifdef CR_DEBUG
+		/* Move vma each time to another palce */
+		void *ptr = mmap(NULL, t->size + size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
+		if (ptr == MAP_FAILED)
+			return -1;
+		aux = mremap(t->buf, t->size,
+				t->size + size, MREMAP_MAYMOVE | MREMAP_FIXED, ptr);
+		munmap(t->buf, t->size);
+#else
 		aux = mremap(t->buf, t->size,
 				t->size + size, MREMAP_MAYMOVE);
+#endif
 	}
 	if (aux == MAP_FAILED)
 		return -1;
@@ -175,6 +190,10 @@  void *rst_mem_alloc(unsigned long size, int type)
 		pr_perror("Can't grow rst mem");
 		return NULL;
 	}
+#ifdef CR_DEBUG
+	else if (type == RM_PRIVATE && t->grow(t, 0)) /* Move a vma to another place */
+		return NULL;
+#endif
 
 	ret = t->free_mem;
 	t->free_mem += size;
diff --git a/scripts/travis/travis-tests b/scripts/travis/travis-tests
index 37117a3..3897542 100755
--- a/scripts/travis/travis-tests
+++ b/scripts/travis/travis-tests
@@ -41,7 +41,7 @@  ulimit -c unlimited
 echo "|`pwd`/test/abrt.sh %P %p %s %e" > /proc/sys/kernel/core_pattern
 
 export GCOV
-time make CC="$CC" -j4
+time make CC="$CC" DEBUG="$CRIU_DEBUG" -j4
 time make CC="$CC" -j4 -C test/zdtm
 
 [ -f "$CCACHE_LOGFILE" ] && cat $CCACHE_LOGFILE

Comments

Pavel Emelianov March 30, 2017, 10:06 a.m.
On 03/30/2017 08:30 AM, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@virtuozzo.com>
> 
> rst_mem_alloc() can move all allocated objects and
> the problem is that we don't know when it will do.
> 
> With this patch, if the criu is built with DEBUG=1,
> rst_mem_alloc() will move objects each time.

This sounds like we need one more fault-injection item, instead
of compile-time decision.

> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  .travis.yml                 |  1 +
>  criu/rst-malloc.c           | 19 +++++++++++++++++++
>  scripts/travis/travis-tests |  2 +-
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index a5dfb9f..d62e086 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -7,6 +7,7 @@ services:
>  env:
>    - TR_ARCH=local       GCOV=1
>    - TR_ARCH=local       CLANG=1
> +  - TR_ARCH=local       CRIU_DEBUG=1
>    - TR_ARCH=alpine
>    - TR_ARCH=x86_64
>    - TR_ARCH=armv7hf
> diff --git a/criu/rst-malloc.c b/criu/rst-malloc.c
> index 4f7357c..057fa31 100644
> --- a/criu/rst-malloc.c
> +++ b/criu/rst-malloc.c
> @@ -23,6 +23,11 @@ static inline unsigned long rst_mem_grow(unsigned long need_size)
>  {
>  	int rst_mem_batch = 2 * page_size();
>  
> +#ifdef CR_DEBUG
> +	if (need_size == 0)
> +		return 0;
> +#endif
> +
>  	need_size = round_up(need_size, page_size());
>  	if (likely(need_size < rst_mem_batch))
>  		need_size = rst_mem_batch;
> @@ -88,8 +93,18 @@ static int grow_remap(struct rst_mem_type_s *t, int flag, unsigned long size)
>  		 * a completely new one and force callers use objects'
>  		 * cpos-s.
>  		 */
> +#ifdef CR_DEBUG
> +		/* Move vma each time to another palce */
> +		void *ptr = mmap(NULL, t->size + size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> +		if (ptr == MAP_FAILED)
> +			return -1;
> +		aux = mremap(t->buf, t->size,
> +				t->size + size, MREMAP_MAYMOVE | MREMAP_FIXED, ptr);
> +		munmap(t->buf, t->size);
> +#else
>  		aux = mremap(t->buf, t->size,
>  				t->size + size, MREMAP_MAYMOVE);
> +#endif
>  	}
>  	if (aux == MAP_FAILED)
>  		return -1;
> @@ -175,6 +190,10 @@ void *rst_mem_alloc(unsigned long size, int type)
>  		pr_perror("Can't grow rst mem");
>  		return NULL;
>  	}
> +#ifdef CR_DEBUG
> +	else if (type == RM_PRIVATE && t->grow(t, 0)) /* Move a vma to another place */
> +		return NULL;
> +#endif
>  
>  	ret = t->free_mem;
>  	t->free_mem += size;
> diff --git a/scripts/travis/travis-tests b/scripts/travis/travis-tests
> index 37117a3..3897542 100755
> --- a/scripts/travis/travis-tests
> +++ b/scripts/travis/travis-tests
> @@ -41,7 +41,7 @@ ulimit -c unlimited
>  echo "|`pwd`/test/abrt.sh %P %p %s %e" > /proc/sys/kernel/core_pattern
>  
>  export GCOV
> -time make CC="$CC" -j4
> +time make CC="$CC" DEBUG="$CRIU_DEBUG" -j4
>  time make CC="$CC" -j4 -C test/zdtm
>  
>  [ -f "$CCACHE_LOGFILE" ] && cat $CCACHE_LOGFILE
>
Andrey Vagin March 30, 2017, 5:56 p.m.
On Thu, Mar 30, 2017 at 01:06:46PM +0300, Pavel Emelyanov wrote:
> On 03/30/2017 08:30 AM, Andrei Vagin wrote:
> > From: Andrei Vagin <avagin@virtuozzo.com>
> > 
> > rst_mem_alloc() can move all allocated objects and
> > the problem is that we don't know when it will do.
> > 
> > With this patch, if the criu is built with DEBUG=1,
> > rst_mem_alloc() will move objects each time.
> 
> This sounds like we need one more fault-injection item, instead
> of compile-time decision.

The idea is to have a debug version with addtional checks and run all
tests on this version. We can't run all tests with each fault injection
item.

> 
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > ---
> >  .travis.yml                 |  1 +
> >  criu/rst-malloc.c           | 19 +++++++++++++++++++
> >  scripts/travis/travis-tests |  2 +-
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index a5dfb9f..d62e086 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -7,6 +7,7 @@ services:
> >  env:
> >    - TR_ARCH=local       GCOV=1
> >    - TR_ARCH=local       CLANG=1
> > +  - TR_ARCH=local       CRIU_DEBUG=1
> >    - TR_ARCH=alpine
> >    - TR_ARCH=x86_64
> >    - TR_ARCH=armv7hf
> > diff --git a/criu/rst-malloc.c b/criu/rst-malloc.c
> > index 4f7357c..057fa31 100644
> > --- a/criu/rst-malloc.c
> > +++ b/criu/rst-malloc.c
> > @@ -23,6 +23,11 @@ static inline unsigned long rst_mem_grow(unsigned long need_size)
> >  {
> >  	int rst_mem_batch = 2 * page_size();
> >  
> > +#ifdef CR_DEBUG
> > +	if (need_size == 0)
> > +		return 0;
> > +#endif
> > +
> >  	need_size = round_up(need_size, page_size());
> >  	if (likely(need_size < rst_mem_batch))
> >  		need_size = rst_mem_batch;
> > @@ -88,8 +93,18 @@ static int grow_remap(struct rst_mem_type_s *t, int flag, unsigned long size)
> >  		 * a completely new one and force callers use objects'
> >  		 * cpos-s.
> >  		 */
> > +#ifdef CR_DEBUG
> > +		/* Move vma each time to another palce */
> > +		void *ptr = mmap(NULL, t->size + size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> > +		if (ptr == MAP_FAILED)
> > +			return -1;
> > +		aux = mremap(t->buf, t->size,
> > +				t->size + size, MREMAP_MAYMOVE | MREMAP_FIXED, ptr);
> > +		munmap(t->buf, t->size);
> > +#else
> >  		aux = mremap(t->buf, t->size,
> >  				t->size + size, MREMAP_MAYMOVE);
> > +#endif
> >  	}
> >  	if (aux == MAP_FAILED)
> >  		return -1;
> > @@ -175,6 +190,10 @@ void *rst_mem_alloc(unsigned long size, int type)
> >  		pr_perror("Can't grow rst mem");
> >  		return NULL;
> >  	}
> > +#ifdef CR_DEBUG
> > +	else if (type == RM_PRIVATE && t->grow(t, 0)) /* Move a vma to another place */
> > +		return NULL;
> > +#endif
> >  
> >  	ret = t->free_mem;
> >  	t->free_mem += size;
> > diff --git a/scripts/travis/travis-tests b/scripts/travis/travis-tests
> > index 37117a3..3897542 100755
> > --- a/scripts/travis/travis-tests
> > +++ b/scripts/travis/travis-tests
> > @@ -41,7 +41,7 @@ ulimit -c unlimited
> >  echo "|`pwd`/test/abrt.sh %P %p %s %e" > /proc/sys/kernel/core_pattern
> >  
> >  export GCOV
> > -time make CC="$CC" -j4
> > +time make CC="$CC" DEBUG="$CRIU_DEBUG" -j4
> >  time make CC="$CC" -j4 -C test/zdtm
> >  
> >  [ -f "$CCACHE_LOGFILE" ] && cat $CCACHE_LOGFILE
> > 
>
Dmitry Safonov March 31, 2017, 4:12 p.m.
2017-03-30 8:30 GMT+03:00 Andrei Vagin <avagin@openvz.org>:
> From: Andrei Vagin <avagin@virtuozzo.com>
>
> rst_mem_alloc() can move all allocated objects and
> the problem is that we don't know when it will do.
>
> With this patch, if the criu is built with DEBUG=1,
> rst_mem_alloc() will move objects each time.
>
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  .travis.yml                 |  1 +
>  criu/rst-malloc.c           | 19 +++++++++++++++++++
>  scripts/travis/travis-tests |  2 +-
>  3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index a5dfb9f..d62e086 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -7,6 +7,7 @@ services:
>  env:
>    - TR_ARCH=local       GCOV=1
>    - TR_ARCH=local       CLANG=1
> +  - TR_ARCH=local       CRIU_DEBUG=1
>    - TR_ARCH=alpine
>    - TR_ARCH=x86_64
>    - TR_ARCH=armv7hf
> diff --git a/criu/rst-malloc.c b/criu/rst-malloc.c
> index 4f7357c..057fa31 100644
> --- a/criu/rst-malloc.c
> +++ b/criu/rst-malloc.c
> @@ -23,6 +23,11 @@ static inline unsigned long rst_mem_grow(unsigned long need_size)
>  {
>         int rst_mem_batch = 2 * page_size();
>
> +#ifdef CR_DEBUG
> +       if (need_size == 0)
> +               return 0;
> +#endif
> +
>         need_size = round_up(need_size, page_size());
>         if (likely(need_size < rst_mem_batch))
>                 need_size = rst_mem_batch;
> @@ -88,8 +93,18 @@ static int grow_remap(struct rst_mem_type_s *t, int flag, unsigned long size)
>                  * a completely new one and force callers use objects'
>                  * cpos-s.
>                  */
> +#ifdef CR_DEBUG

Could we name it somehow more descriptive?
So we wouldn't introduce large CR_DEBUG for everything,
and say DEBUG_RST_ALLOC?
And just add it in makefile here:

ifeq ($(DEBUG),1)
        DEFINES        += -DCR_DEBUG -DDEBUG_RST_ALLOC

Or maybe you could come with a better name.

Maybe even some day we could have it as a build option
in .config along with some other.

> +               /* Move vma each time to another palce */
> +               void *ptr = mmap(NULL, t->size + size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> +               if (ptr == MAP_FAILED)
> +                       return -1;
> +               aux = mremap(t->buf, t->size,
> +                               t->size + size, MREMAP_MAYMOVE | MREMAP_FIXED, ptr);
> +               munmap(t->buf, t->size);
> +#else
>                 aux = mremap(t->buf, t->size,
>                                 t->size + size, MREMAP_MAYMOVE);
> +#endif
>         }
>         if (aux == MAP_FAILED)
>                 return -1;
> @@ -175,6 +190,10 @@ void *rst_mem_alloc(unsigned long size, int type)
>                 pr_perror("Can't grow rst mem");
>                 return NULL;
>         }
> +#ifdef CR_DEBUG
> +       else if (type == RM_PRIVATE && t->grow(t, 0)) /* Move a vma to another place */
> +               return NULL;
> +#endif
>
>         ret = t->free_mem;
>         t->free_mem += size;
> diff --git a/scripts/travis/travis-tests b/scripts/travis/travis-tests
> index 37117a3..3897542 100755
> --- a/scripts/travis/travis-tests
> +++ b/scripts/travis/travis-tests
> @@ -41,7 +41,7 @@ ulimit -c unlimited
>  echo "|`pwd`/test/abrt.sh %P %p %s %e" > /proc/sys/kernel/core_pattern
>
>  export GCOV
> -time make CC="$CC" -j4
> +time make CC="$CC" DEBUG="$CRIU_DEBUG" -j4
>  time make CC="$CC" -j4 -C test/zdtm
>
>  [ -f "$CCACHE_LOGFILE" ] && cat $CCACHE_LOGFILE
> --
> 2.7.4
>
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Andrey Vagin March 31, 2017, 9:26 p.m.
On Fri, Mar 31, 2017 at 07:12:05PM +0300, Dmitry Safonov wrote:
> 2017-03-30 8:30 GMT+03:00 Andrei Vagin <avagin@openvz.org>:
> > From: Andrei Vagin <avagin@virtuozzo.com>
> >
> > rst_mem_alloc() can move all allocated objects and
> > the problem is that we don't know when it will do.
> >
> > With this patch, if the criu is built with DEBUG=1,
> > rst_mem_alloc() will move objects each time.
> >
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > ---
> >  .travis.yml                 |  1 +
> >  criu/rst-malloc.c           | 19 +++++++++++++++++++
> >  scripts/travis/travis-tests |  2 +-
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/.travis.yml b/.travis.yml
> > index a5dfb9f..d62e086 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -7,6 +7,7 @@ services:
> >  env:
> >    - TR_ARCH=local       GCOV=1
> >    - TR_ARCH=local       CLANG=1
> > +  - TR_ARCH=local       CRIU_DEBUG=1
> >    - TR_ARCH=alpine
> >    - TR_ARCH=x86_64
> >    - TR_ARCH=armv7hf
> > diff --git a/criu/rst-malloc.c b/criu/rst-malloc.c
> > index 4f7357c..057fa31 100644
> > --- a/criu/rst-malloc.c
> > +++ b/criu/rst-malloc.c
> > @@ -23,6 +23,11 @@ static inline unsigned long rst_mem_grow(unsigned long need_size)
> >  {
> >         int rst_mem_batch = 2 * page_size();
> >
> > +#ifdef CR_DEBUG
> > +       if (need_size == 0)
> > +               return 0;
> > +#endif
> > +
> >         need_size = round_up(need_size, page_size());
> >         if (likely(need_size < rst_mem_batch))
> >                 need_size = rst_mem_batch;
> > @@ -88,8 +93,18 @@ static int grow_remap(struct rst_mem_type_s *t, int flag, unsigned long size)
> >                  * a completely new one and force callers use objects'
> >                  * cpos-s.
> >                  */
> > +#ifdef CR_DEBUG
> 
> Could we name it somehow more descriptive?
> So we wouldn't introduce large CR_DEBUG for everything,
> and say DEBUG_RST_ALLOC?
> And just add it in makefile here:
> 
> ifeq ($(DEBUG),1)
>         DEFINES        += -DCR_DEBUG -DDEBUG_RST_ALLOC

I think it is a good idea. Will send a new version. Thank you
> 
> Or maybe you could come with a better name.
> 
> Maybe even some day we could have it as a build option
> in .config along with some other.
> 
> > +               /* Move vma each time to another palce */
> > +               void *ptr = mmap(NULL, t->size + size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> > +               if (ptr == MAP_FAILED)
> > +                       return -1;
> > +               aux = mremap(t->buf, t->size,
> > +                               t->size + size, MREMAP_MAYMOVE | MREMAP_FIXED, ptr);
> > +               munmap(t->buf, t->size);
> > +#else
> >                 aux = mremap(t->buf, t->size,
> >                                 t->size + size, MREMAP_MAYMOVE);
> > +#endif
> >         }
> >         if (aux == MAP_FAILED)
> >                 return -1;
> > @@ -175,6 +190,10 @@ void *rst_mem_alloc(unsigned long size, int type)
> >                 pr_perror("Can't grow rst mem");
> >                 return NULL;
> >         }
> > +#ifdef CR_DEBUG
> > +       else if (type == RM_PRIVATE && t->grow(t, 0)) /* Move a vma to another place */
> > +               return NULL;
> > +#endif
> >
> >         ret = t->free_mem;
> >         t->free_mem += size;
> > diff --git a/scripts/travis/travis-tests b/scripts/travis/travis-tests
> > index 37117a3..3897542 100755
> > --- a/scripts/travis/travis-tests
> > +++ b/scripts/travis/travis-tests
> > @@ -41,7 +41,7 @@ ulimit -c unlimited
> >  echo "|`pwd`/test/abrt.sh %P %p %s %e" > /proc/sys/kernel/core_pattern
> >
> >  export GCOV
> > -time make CC="$CC" -j4
> > +time make CC="$CC" DEBUG="$CRIU_DEBUG" -j4
> >  time make CC="$CC" -j4 -C test/zdtm
> >
> >  [ -f "$CCACHE_LOGFILE" ] && cat $CCACHE_LOGFILE
> > --
> > 2.7.4
> >
> > _______________________________________________
> > CRIU mailing list
> > CRIU@openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
> 
> 
> 
> -- 
>              Dmitry