fix migration of zero pages

Not sending zero pages breaks migration if a page is zero at the source but not at the destination. This can e.g. happen if different BIOS versions are used at source and destination. It has also been reported that migration on pseries is completely broken with this patch.

This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

Conflicts:

arch_init.c

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

arch_init.c +4 -9

index 5d32ecf..08fccf6 100644
@@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             bytes_sent = -1;
             if (is_zero_page(p)) {
                 acct_info.dup_pages++;
-                if (!ram_bulk_stage) {
-                    bytes_sent = save_block_hdr(f, block, offset, cont,
-                                                RAM_SAVE_FLAG_COMPRESS);
-                    qemu_put_byte(f, 0);
-                    bytes_sent++;
-                } else {
-                    acct_info.skipped_pages++;
-                    bytes_sent = 0;
-                }
+                bytes_sent = save_block_hdr(f, block, offset, cont,
+                                            RAM_SAVE_FLAG_COMPRESS);
+                qemu_put_byte(f, 0);
+                bytes_sent++;
             } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
                 current_addr = block->offset + offset;
                 bytes_sent = save_xbzrle_page(f, p, current_addr, block,

Signature · 1 line

1.7.9.5

Not sending zero pages breaks migration if a page is zero at the source but not at the destination. This can e.g. happen if different BIOS versions are used at source and destination. It has also been reported that migration on pseries is completely broken with this patch.

This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

Conflicts:

arch_init.c

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

arch_init.c +0 -6

index 5d32ecf..08fccf6 100644
@@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             bytes_sent = -1;
             if (is_zero_page(p)) {
                 acct_info.dup_pages++;
-                if (!ram_bulk_stage) {
-                    bytes_sent = save_block_hdr(f, block, offset, cont,
-                                                RAM_SAVE_FLAG_COMPRESS);
-                    qemu_put_byte(f, 0);
-                    bytes_sent++;
-                } else {

Not sending zero pages breaks migration if a page is zero at the source but not at the destination. This can e.g. happen if different BIOS versions are used at source and destination. It has also been reported that migration on pseries is completely broken with this patch.

This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

Conflicts:

arch_init.c

Signed-off-by Peter Lieven <[email protected]>

---
  arch_init.c |   13 ++++---------
  1 file changed, 4 insertions(+), 9 deletions(-)

arch_init.c +4 -9

index 5d32ecf..08fccf6 100644
@@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
              bytes_sent = -1;
              if (is_zero_page(p)) {
                  acct_info.dup_pages++;
-                if (!ram_bulk_stage) {
-                    bytes_sent = save_block_hdr(f, block, offset, cont,
-                                                RAM_SAVE_FLAG_COMPRESS);
-                    qemu_put_byte(f, 0);
-                    bytes_sent++;
-                } else {
-                    acct_info.skipped_pages++;
-                    bytes_sent = 0;
-                }
+                bytes_sent = save_block_hdr(f, block, offset, cont,
+                                            RAM_SAVE_FLAG_COMPRESS);
+                qemu_put_byte(f, 0);
+                bytes_sent++;
              } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
                  current_addr = block->offset + offset;
                  bytes_sent = save_xbzrle_page(f, p, current_addr, block,

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +2 -1

index 08fccf6..cf4e1d5 100644
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {
+                memset(host, ch, TARGET_PAGE_SIZE);
 #ifndef _WIN32

arch_init.c +1 -0

 #endif
+            }
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
             void *host;
 

Signature · 1 line

1.7.9.5

Quoted 22 lines · Peter Lieven

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +2 -1

index 08fccf6..cf4e1d5 100644
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {
+                memset(host, ch, TARGET_PAGE_SIZE);
 #ifndef _WIN32

arch_init.c static int ram_load(QEMUFile *f, void *opaque, int version_id) · line 842

Quoted 10 lines

arch_init.c +5 -5

-            if (ch == 0 &&
-                (!kvm_enabled() || kvm_has_sync_mmu()) &&
-                getpagesize() <= TARGET_PAGE_SIZE) {
-                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-            }
+                if (ch == 0 &&
+                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
+                    getpagesize() <= TARGET_PAGE_SIZE) {
+                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+                }

Reviewed-by Michael Roth <[email protected]>

Also CC'ing qemu-stable, but from what I gather this just mitigates the

Quoted 22 lines

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
arch_init.c | 14 ++++++++------

1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +2 -1

index 08fccf6..cf4e1d5 100644
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
            }
 
            ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {
+                memset(host, ch, TARGET_PAGE_SIZE);
 #ifndef _WIN32

arch_init.c static int ram_load(QEMUFile *f, void *opaque, int version_id) · line 842

Quoted 10 lines

arch_init.c +5 -5

-            if (ch == 0 &&
-                (!kvm_enabled() || kvm_has_sync_mmu()) &&
-                getpagesize() <= TARGET_PAGE_SIZE) {
-                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-            }
+                if (ch == 0 &&
+                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
+                    getpagesize() <= TARGET_PAGE_SIZE) {
+                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+                }

Reviewed-by Michael Roth <[email protected]>

Also CC'ing qemu-stable, but from what I gather this just mitigates the performance impact of 1/2 and isn't strictly necessary to fix migration? i.e. we can still do outgoing migration to 1.5.0 guests?

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +2 -6

index 08fccf6..cf4e1d5 100644
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
             }
 
             ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {
+                memset(host, ch, TARGET_PAGE_SIZE);
 #ifndef _WIN32
-            if (ch == 0 &&
-                (!kvm_enabled() || kvm_has_sync_mmu()) &&
-                getpagesize() <= TARGET_PAGE_SIZE) {
-                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-            }

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
  arch_init.c |   14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +0 -0

index 08fccf6..cf4e1d5 100644

arch_init.c +7 -5

+                memset(host, ch, TARGET_PAGE_SIZE);
  #ifndef _WIN32
-            if (ch == 0 &&
-                (!kvm_enabled() || kvm_has_sync_mmu()) &&
-                getpagesize() <= TARGET_PAGE_SIZE) {
-                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-            }
+                if (ch == 0 &&
+                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
+                    getpagesize() <= TARGET_PAGE_SIZE) {
+                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+                }
  #endif
+            }
          } else if (flags & RAM_SAVE_FLAG_PAGE) {
              void *host;
 

Quoted 17 lines · Peter Lieven

on incoming migration do not memset pages to zero if they already read as zero. this will allocate a new zero page and consume memory unnecessarily. even if we madvise a MADV_DONTNEED later this will only deallocate the memory asynchronously.

Signed-off-by Peter Lieven <[email protected]>

---
  arch_init.c |   14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

arch_init.c +0 -0

index 08fccf6..cf4e1d5 100644
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
              }
 

arch_init.c static int ram_load(QEMUFile *f, void *opaque, int version_id) · line 835

Quoted 3 lines

arch_init.c +1 -1

              ch = qemu_get_byte(f);
-            memset(host, ch, TARGET_PAGE_SIZE);
+            if (ch != 0 || !is_zero_page(host)) {

If incoming page is not zero, always memset. If incoming page is
zero, then if on destination it is not zero, memset. Logic is OK. Only question is if the read operation in is_zero_page() consumes memory, as there are doubt in the discuss before.