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 reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

+++ b/qapi-schema.json

qapi-schema.json +1 -3

@@ -499,9 +499,7 @@
 #
 # @total: total amount of bytes involved in the migration process
 #
-# @duplicate: number of duplicate (zero) pages (since 1.2)
-#
-# @skipped: number of skipped zero pages (since 1.5)
+# @duplicate: number of duplicate pages (since 1.2)
 #
 # @normal : number of normal pages (since 1.2)
 #

----- Original Message -----

Quoted 24 lines

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 reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

+++ b/qapi-schema.json

qapi-schema.json +1 -3

@@ -499,9 +499,7 @@
  #
  # @total: total amount of bytes involved in the migration process
  #
-# @duplicate: number of duplicate (zero) pages (since 1.2)
-#
-# @skipped: number of skipped zero pages (since 1.5)
+# @duplicate: number of duplicate pages (since 1.2)
  #
  # @normal : number of normal pages (since 1.2)
  #

@@ -514,8 +512,8 @@
  ##
  { 'type': 'MigrationStats',
    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,

qapi-schema.json line 516

Quoted 4 lines

qapi-schema.json +2 -2

-           'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
-           'normal-bytes': 'int', 'dirty-pages-rate' : 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
+           'dirty-pages-rate' : 'int' } }

This hunk is questionable. Removing something that we have previously sent over the wire may break clients that are expecting this field to exist. Rather than reverting the entire patch, you should consider keeping this field present in QMP, even if you now always populate it with 0.

Quoted 5 lines

Quoted 5 lines

qapi-schema.json +0 -1

@@ -514,8 +512,8 @@
  ##
  { 'type': 'MigrationStats',
    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'duplicate': 'int', 'skipped': 'int', 'normal': 'int',

Quoted 9 lines

qapi-schema.json line 516

Quoted 3 lines

qapi-schema.json +2 -1

-           'normal-bytes': 'int', 'dirty-pages-rate' : 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
+           'dirty-pages-rate' : 'int' } }

This hunk is questionable. Removing something that we have previously sent
over the wire may break clients that are expecting this field to exist. Rather than reverting the entire patch, you should consider keeping this field present in QMP, even if you now always populate it with 0.

You are right. I might still account zero pages in the bulk phase to give this
field a meaning. These pages are very likely not written on the destination except
for the cornercases. What do you think?

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 reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.

Conflicts:

arch_init.c
qmp-commands.hx

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

---
 arch_init.c                   |   24 ++++--------------------
 hmp.c                         |    2 --
 include/migration/migration.h |    2 --
 migration.c                   |    3 +--
 qapi-schema.json              |    8 +++-----
 qmp-commands.hx               |    1 -
 6 files changed, 8 insertions(+), 32 deletions(-)

arch_init.c +4 -20

index 5d32ecf..283081d 100644
@@ -183,7 +183,6 @@ int64_t xbzrle_cache_resize(int64_t new_size)
 /* accounting for migration statistics */
 typedef struct AccountingInfo {
     uint64_t dup_pages;
-    uint64_t skipped_pages;
     uint64_t norm_pages;
     uint64_t iterations;
     uint64_t xbzrle_bytes;

@@ -209,16 +208,6 @@ uint64_t dup_mig_pages_transferred(void)
     return acct_info.dup_pages;
 }
 
-uint64_t skipped_mig_bytes_transferred(void)
-{
-    return acct_info.skipped_pages * TARGET_PAGE_SIZE;
-}
-
-uint64_t skipped_mig_pages_transferred(void)
-{
-    return acct_info.skipped_pages;
-}
-
 uint64_t norm_mig_bytes_transferred(void)
 {
     return acct_info.norm_pages * TARGET_PAGE_SIZE;

@@ -457,15 +446,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,

hmp.c +0 -2

index 4fb76ec..55c4423 100644
@@ -173,8 +173,6 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->ram->total >> 10);
         monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
                        info->ram->duplicate);
-        monitor_printf(mon, "skipped: %" PRIu64 " pages\n",
-                       info->ram->skipped);
         monitor_printf(mon, "normal: %" PRIu64 " pages\n",
                        info->ram->normal);
         monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",

include/migration/migration.h +0 -2

index e2acec6..bb617fd 100644
@@ -96,8 +96,6 @@ extern SaveVMHandlers savevm_ram_handlers;
 
 uint64_t dup_mig_bytes_transferred(void);
 uint64_t dup_mig_pages_transferred(void);
-uint64_t skipped_mig_bytes_transferred(void);
-uint64_t skipped_mig_pages_transferred(void);
 uint64_t norm_mig_bytes_transferred(void);
 uint64_t norm_mig_pages_transferred(void);
 uint64_t xbzrle_mig_bytes_transferred(void);

migration.c +1 -2

index 058f9e6..c305d3e 100644
@@ -197,11 +197,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->remaining = ram_bytes_remaining();
         info->ram->total = ram_bytes_total();
         info->ram->duplicate = dup_mig_pages_transferred();
-        info->ram->skipped = skipped_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
         info->ram->dirty_pages_rate = s->dirty_pages_rate;
 
+
         if (blk_mig_active()) {
             info->has_disk = true;
             info->disk = g_malloc0(sizeof(*info->disk));

@@ -227,7 +227,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->remaining = 0;
         info->ram->total = ram_bytes_total();
         info->ram->duplicate = dup_mig_pages_transferred();
-        info->ram->skipped = skipped_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
         break;

qapi-schema.json +3 -5

index ef1f657..db0986c 100644
@@ -499,9 +499,7 @@
 #
 # @total: total amount of bytes involved in the migration process
 #
-# @duplicate: number of duplicate (zero) pages (since 1.2)
-#
-# @skipped: number of skipped zero pages (since 1.5)
+# @duplicate: number of duplicate pages (since 1.2)
 #
 # @normal : number of normal pages (since 1.2)
 #

@@ -514,8 +512,8 @@
 ##
 { 'type': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
-           'normal-bytes': 'int', 'dirty-pages-rate' : 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
+           'dirty-pages-rate' : 'int' } }
 
 ##
 # @XBZRLECacheStats

qmp-commands.hx +0 -1

index ffd130e..fa5986f 100644
@@ -2515,7 +2515,6 @@ The main json-object contains the following:
          - "duplicate": number of pages filled entirely with the same
             byte (json-int)
             These are sent over the wire much more efficiently.
-         - "skipped": number of skipped zero pages (json-int)
          - "normal" : number of whole pages transferred.  I.e. they
             were not sent as duplicate or xbzrle pages (json-int)
          - "normal-bytes" : number of bytes transferred in whole

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 283081d..642f241 100644
@@ -821,14 +821,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);
-            }

Il 06/06/2013 10:36, Peter Lieven ha scritto:

Quoted 32 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 +7 -6

index 283081d..642f241 100644
@@ -821,14 +821,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);
-            }
+                if (ch == 0 &&
+                    (!kvm_enabled() || kvm_has_sync_mmu()) &&
+                    getpagesize() <= TARGET_PAGE_SIZE) {
+                    qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+                }

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

Quoted 6 lines

arch_init.c +1 -0

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

Reviewed-by Paolo Bonzini <[email protected]>