[Qemu-devel] [PATCH] migration: initialize RAM to zero

[Qemu-devel] [PATCH] migration: initialize RAM to zero

[Paolo Bonzini]

Using qemu_memalign only leaves the RAM zero by chance, because libc
will usually use mmap to satisfy our huge requests.  But memory will
not be zero when using MALLOC_PERTURB_ with a nonzero value.  In the
case of incoming migration, this breaks a recently-introduced
invariant (commit f1c7279, migration: do not sent zero pages in
bulk stage, 2013-03-26).

To fix this, use mmap ourselves to get a well-aligned, always zero
block for the RAM.  Mmap-ed memory is easy to "trim" at the sides.

This also removes the need to do something special on valgrind
(see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/oslib-posix.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 433dd68..91f5aab 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -52,12 +52,8 @@ extern int daemon(int, int);
 #include "sysemu/sysemu.h"
 #include "trace.h"
 #include "qemu/sockets.h"
+#include <sys/mman.h>
 
-#if defined(CONFIG_VALGRIND)
-static int running_on_valgrind = -1;
-#else
-#  define running_on_valgrind 0
-#endif
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
 #endif
@@ -108,22 +104,22 @@ void *qemu_memalign(size_t alignment, size_t size)
 /* alloc shared memory pages */
 void *qemu_vmalloc(size_t size)
 {
-    void *ptr;
     size_t align = QEMU_VMALLOC_ALIGN;
+    size_t total = size + align - getpagesize();
+    void *ptr = mmap(0, total, PROT_READ | PROT_WRITE,
+                     MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+    size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
 
-#if defined(CONFIG_VALGRIND)
-    if (running_on_valgrind < 0) {
-        /* First call, test whether we are running on Valgrind.
-           This is a substitute for RUNNING_ON_VALGRIND from valgrind.h. */
-        const char *ld = getenv("LD_PRELOAD");
-        running_on_valgrind = (ld != NULL && strstr(ld, "vgpreload"));
-    }
-#endif
+    ptr += offset;
+    total -= offset;
 
-    if (size < align || running_on_valgrind) {
-        align = getpagesize();
+    if (offset > 0) {
+        munmap(ptr - offset, offset);
+    }
+    if (total > size) {
+        munmap(ptr + size, total - size);
     }
-    ptr = qemu_memalign(align, size);
+
     trace_qemu_vmalloc(size, ptr);
     return ptr;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] migration: initialize RAM to zero

[Peter Lieven]

Am 08.04.2013 um 12:47 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> Using qemu_memalign only leaves the RAM zero by chance, because libc
> will usually use mmap to satisfy our huge requests.  But memory will
> not be zero when using MALLOC_PERTURB_ with a nonzero value.  In the
> case of incoming migration, this breaks a recently-introduced
> invariant (commit f1c7279, migration: do not sent zero pages in
> bulk stage, 2013-03-26).
> 
> To fix this, use mmap ourselves to get a well-aligned, always zero
> block for the RAM.  Mmap-ed memory is easy to "trim" at the sides.
> 
> This also removes the need to do something special on valgrind
> (see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/oslib-posix.c | 30 +++++++++++++-----------------
> 1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 433dd68..91f5aab 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -52,12 +52,8 @@ extern int daemon(int, int);
> #include "sysemu/sysemu.h"
> #include "trace.h"
> #include "qemu/sockets.h"
> +#include <sys/mman.h>
> 
> -#if defined(CONFIG_VALGRIND)
> -static int running_on_valgrind = -1;
> -#else
> -#  define running_on_valgrind 0
> -#endif
> #ifdef CONFIG_LINUX
> #include <sys/syscall.h>
> #endif
> @@ -108,22 +104,22 @@ void *qemu_memalign(size_t alignment, size_t size)
> /* alloc shared memory pages */
> void *qemu_vmalloc(size_t size)
> {
> -    void *ptr;
>     size_t align = QEMU_VMALLOC_ALIGN;
> +    size_t total = size + align - getpagesize();
> +    void *ptr = mmap(0, total, PROT_READ | PROT_WRITE,
> +                     MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +    size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
> 
> -#if defined(CONFIG_VALGRIND)
> -    if (running_on_valgrind < 0) {
> -        /* First call, test whether we are running on Valgrind.
> -           This is a substitute for RUNNING_ON_VALGRIND from valgrind.h. */
> -        const char *ld = getenv("LD_PRELOAD");
> -        running_on_valgrind = (ld != NULL && strstr(ld, "vgpreload"));
> -    }
> -#endif
> +    ptr += offset;
> +    total -= offset;
> 
> -    if (size < align || running_on_valgrind) {
> -        align = getpagesize();
> +    if (offset > 0) {
> +        munmap(ptr - offset, offset);
> +    }
> +    if (total > size) {
> +        munmap(ptr + size, total - size);
>     }
> -    ptr = qemu_memalign(align, size);
> +
>     trace_qemu_vmalloc(size, ptr);
>     return ptr;
> }
> -- 
> 1.8.1.4
> 

Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH] migration: initialize RAM to zero

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Using qemu_memalign only leaves the RAM zero by chance, because libc
> will usually use mmap to satisfy our huge requests.  But memory will
> not be zero when using MALLOC_PERTURB_ with a nonzero value.  In the
> case of incoming migration, this breaks a recently-introduced
> invariant (commit f1c7279, migration: do not sent zero pages in
> bulk stage, 2013-03-26).
>
> To fix this, use mmap ourselves to get a well-aligned, always zero
> block for the RAM.  Mmap-ed memory is easy to "trim" at the sides.
>
> This also removes the need to do something special on valgrind
> (see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).

Suggest to state explicitly that you effectively revert it.

You left #define CONFIG_VALGRIND in, even though it's no longer used.
Intentional?

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/oslib-posix.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 433dd68..91f5aab 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -52,12 +52,8 @@ extern int daemon(int, int);
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "qemu/sockets.h"
> +#include <sys/mman.h>
>  
> -#if defined(CONFIG_VALGRIND)
> -static int running_on_valgrind = -1;
> -#else
> -#  define running_on_valgrind 0
> -#endif
>  #ifdef CONFIG_LINUX
>  #include <sys/syscall.h>
>  #endif
> @@ -108,22 +104,22 @@ void *qemu_memalign(size_t alignment, size_t size)
>  /* alloc shared memory pages */
>  void *qemu_vmalloc(size_t size)
>  {
> -    void *ptr;
>      size_t align = QEMU_VMALLOC_ALIGN;
> +    size_t total = size + align - getpagesize();
> +    void *ptr = mmap(0, total, PROT_READ | PROT_WRITE,
> +                     MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +    size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>  
> -#if defined(CONFIG_VALGRIND)
> -    if (running_on_valgrind < 0) {
> -        /* First call, test whether we are running on Valgrind.
> -           This is a substitute for RUNNING_ON_VALGRIND from valgrind.h. */
> -        const char *ld = getenv("LD_PRELOAD");
> -        running_on_valgrind = (ld != NULL && strstr(ld, "vgpreload"));
> -    }
> -#endif

Please check for mmap() failure.

The old code uses qemu_memalign(), which treats allocation failure as a
programming error: calls abort().  Not sure that's actually appropriate
here.

> +    ptr += offset;
> +    total -= offset;
>  
> -    if (size < align || running_on_valgrind) {
> -        align = getpagesize();
> +    if (offset > 0) {
> +        munmap(ptr - offset, offset);
> +    }
> +    if (total > size) {
> +        munmap(ptr + size, total - size);
>      }
> -    ptr = qemu_memalign(align, size);
> +
>      trace_qemu_vmalloc(size, ptr);
>      return ptr;
>  }

Re: [Qemu-devel] [PATCH] migration: initialize RAM to zero

[Paolo Bonzini]

Il 09/04/2013 10:52, Markus Armbruster ha scritto:
>> > This also removes the need to do something special on valgrind
>> > (see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).
> Suggest to state explicitly that you effectively revert it.
> 
> You left #define CONFIG_VALGRIND in, even though it's no longer used.
> Intentional?

Oh, there's both CONFIG_VALGRIND and CONFIG_VALGRIND_H.  Nice.  I'll
send v2.

Paolo

Re: [Qemu-devel] [PATCH] migration: initialize RAM to zero

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Using qemu_memalign only leaves the RAM zero by chance, because libc
> will usually use mmap to satisfy our huge requests.  But memory will
> not be zero when using MALLOC_PERTURB_ with a nonzero value.  In the
> case of incoming migration, this breaks a recently-introduced
> invariant (commit f1c7279, migration: do not sent zero pages in
> bulk stage, 2013-03-26).
>
> To fix this, use mmap ourselves to get a well-aligned, always zero
> block for the RAM.  Mmap-ed memory is easy to "trim" at the sides.
>
> This also removes the need to do something special on valgrind
> (see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Except for the mmap return check and the missing valgrind check,  the
patch looks good.

Thanks,  Juan.

Re: [Qemu-devel] [PATCH] migration: initialize RAM to zero

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 09/04/2013 10:52, Markus Armbruster ha scritto:
>>> > This also removes the need to do something special on valgrind
>>> > (see commit c2a8238a, Support running QEMU on Valgrind, 2011-10-31).
>> Suggest to state explicitly that you effectively revert it.
>> 
>> You left #define CONFIG_VALGRIND in, even though it's no longer used.
>> Intentional?
>
> Oh, there's both CONFIG_VALGRIND and CONFIG_VALGRIND_H.  Nice.  I'll
> send v2.

That confused me briefly, too :)

Make sure to address the mmap() error checking as well.