[PATCH 0/5] migration/ram: Merge zero page handling

[PATCH 0/5] migration/ram: Merge zero page handling

[Fabiano Rosas]

Hi,

This is another small series that I extracted from my fixed-ram series
and that could be already considered for merging.

This is just code movement, no functional change. The objective is to
consolidate the zero page handling in the same routine that saves the
page header and does accounting. Then in the future I'll be able to
just return early because fixed-ram ignores zero pages.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/968300062

Fabiano Rosas (5):
  migration/ram: Remove RAMState from xbzrle_cache_zero_page
  migration/ram: Stop passing QEMUFile around in save_zero_page
  migration/ram: Move xbzrle zero page handling into save_zero_page
  migration/ram: Return early from save_zero_page
  migration/ram: Merge save_zero_page functions

 migration/ram.c | 75 ++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 45 deletions(-)

-- 
2.35.3

[PATCH 1/5] migration/ram: Remove RAMState from xbzrle_cache_zero_page

[Fabiano Rosas]

'rs' is not used in that function. It's a leftover from commit
9360447d34 ("ram: Use MigrationStats for statistics").

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/ram.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 9040d66e61..87efab72e8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -561,7 +561,6 @@ void mig_throttle_counter_reset(void)
 /**
  * xbzrle_cache_zero_page: insert a zero page in the XBZRLE cache
  *
- * @rs: current RAM state
  * @current_addr: address for the zero page
  *
  * Update the xbzrle cache to reflect a page that's been sent as all 0.
@@ -570,7 +569,7 @@ void mig_throttle_counter_reset(void)
  * As a bonus, if the page wasn't in the cache it gets added so that
  * when a small write is made into the 0'd page it gets XBZRLE sent.
  */
-static void xbzrle_cache_zero_page(RAMState *rs, ram_addr_t current_addr)
+static void xbzrle_cache_zero_page(ram_addr_t current_addr)
 {
     /* We don't care if this fails to allocate a new cache page
      * as long as it updated an old one */
@@ -2148,7 +2147,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
          */
         if (rs->xbzrle_started) {
             XBZRLE_cache_lock();
-            xbzrle_cache_zero_page(rs, block->offset + offset);
+            xbzrle_cache_zero_page(block->offset + offset);
             XBZRLE_cache_unlock();
         }
         return res;
-- 
2.35.3

Re: [PATCH 1/5] migration/ram: Remove RAMState from xbzrle_cache_zero_page

[Peter Xu]

On Tue, Aug 15, 2023 at 11:38:24AM -0300, Fabiano Rosas wrote:
> 'rs' is not used in that function. It's a leftover from commit
> 9360447d34 ("ram: Use MigrationStats for statistics").
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 2/5] migration/ram: Stop passing QEMUFile around in save_zero_page

[Fabiano Rosas]

We don't need the QEMUFile when we're already passing the
PageSearchStatus.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/ram.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 87efab72e8..761f43dc34 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1138,10 +1138,11 @@ void ram_release_page(const char *rbname, uint64_t offset)
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
-                                  RAMBlock *block, ram_addr_t offset)
+static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
+                                  ram_addr_t offset)
 {
     uint8_t *p = block->host + offset;
+    QEMUFile *file = pss->pss_channel;
     int len = 0;
 
     if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
@@ -1162,10 +1163,10 @@ static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
+static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
                           ram_addr_t offset)
 {
-    int len = save_zero_page_to_file(pss, f, block, offset);
+    int len = save_zero_page_to_file(pss, block, offset);
 
     if (len) {
         stat64_add(&mig_stats.zero_pages, 1);
@@ -2140,7 +2141,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return 1;
     }
 
-    res = save_zero_page(pss, pss->pss_channel, block, offset);
+    res = save_zero_page(pss, block, offset);
     if (res > 0) {
         /* Must let xbzrle know, otherwise a previous (now 0'd) cached
          * page would be stale
-- 
2.35.3

Re: [PATCH 2/5] migration/ram: Stop passing QEMUFile around in save_zero_page

[Peter Xu]

On Tue, Aug 15, 2023 at 11:38:25AM -0300, Fabiano Rosas wrote:
> We don't need the QEMUFile when we're already passing the
> PageSearchStatus.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 3/5] migration/ram: Move xbzrle zero page handling into save_zero_page

[Fabiano Rosas]

It makes a bit more sense to have the zero page handling of xbzrle
right where we save the zero page.

This also makes save_zero_page() follow the same format as
save_compress_page() at the top level of
ram_save_target_page_legacy().

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/ram.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 761f43dc34..a10410a1a5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1159,11 +1159,12 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
  *
  * Returns the number of pages written.
  *
+ * @rs: current RAM state
  * @pss: current PSS channel
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
+static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
                           ram_addr_t offset)
 {
     int len = save_zero_page_to_file(pss, block, offset);
@@ -1171,6 +1172,17 @@ static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
     if (len) {
         stat64_add(&mig_stats.zero_pages, 1);
         ram_transferred_add(len);
+
+        /*
+         * Must let xbzrle know, otherwise a previous (now 0'd) cached
+         * page would be stale.
+         */
+        if (rs->xbzrle_started) {
+            XBZRLE_cache_lock();
+            xbzrle_cache_zero_page(block->offset + offset);
+            XBZRLE_cache_unlock();
+        }
+
         return 1;
     }
     return -1;
@@ -2141,17 +2153,8 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return 1;
     }
 
-    res = save_zero_page(pss, block, offset);
-    if (res > 0) {
-        /* Must let xbzrle know, otherwise a previous (now 0'd) cached
-         * page would be stale
-         */
-        if (rs->xbzrle_started) {
-            XBZRLE_cache_lock();
-            xbzrle_cache_zero_page(block->offset + offset);
-            XBZRLE_cache_unlock();
-        }
-        return res;
+    if (save_zero_page(rs, pss, block, offset)) {
+        return 1;
     }
 
     /*
-- 
2.35.3

Re: [PATCH 3/5] migration/ram: Move xbzrle zero page handling into save_zero_page

[Peter Xu]

On Tue, Aug 15, 2023 at 11:38:26AM -0300, Fabiano Rosas wrote:
> It makes a bit more sense to have the zero page handling of xbzrle
> right where we save the zero page.
> 
> This also makes save_zero_page() follow the same format as
> save_compress_page() at the top level of
> ram_save_target_page_legacy().
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 3/5] migration/ram: Move xbzrle zero page handling into save_zero_page

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> It makes a bit more sense to have the zero page handling of xbzrle
> right where we save the zero page.
>
> This also makes save_zero_page() follow the same format as
> save_compress_page() at the top level of
> ram_save_target_page_legacy().
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/ram.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 761f43dc34..a10410a1a5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1159,11 +1159,12 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
>   *
>   * Returns the number of pages written.
>   *
> + * @rs: current RAM state
>   * @pss: current PSS channel
>   * @block: block that contains the page we want to send
>   * @offset: offset inside the block for the page
>   */
> -static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
> +static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>                            ram_addr_t offset)
>  {
>      int len = save_zero_page_to_file(pss, block, offset);
> @@ -1171,6 +1172,17 @@ static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
>      if (len) {
>          stat64_add(&mig_stats.zero_pages, 1);
>          ram_transferred_add(len);
> +
> +        /*
> +         * Must let xbzrle know, otherwise a previous (now 0'd) cached
> +         * page would be stale.
> +         */
> +        if (rs->xbzrle_started) {
> +            XBZRLE_cache_lock();
> +            xbzrle_cache_zero_page(block->offset + offset);
> +            XBZRLE_cache_unlock();
> +        }
> +
>          return 1;
>      }
>      return -1;
> @@ -2141,17 +2153,8 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return 1;
>      }
>  
> -    res = save_zero_page(pss, block, offset);
> -    if (res > 0) {

These two subtractions should be in patch 5.

> -        /* Must let xbzrle know, otherwise a previous (now 0'd) cached
> -         * page would be stale
> -         */
> -        if (rs->xbzrle_started) {
> -            XBZRLE_cache_lock();
> -            xbzrle_cache_zero_page(block->offset + offset);
> -            XBZRLE_cache_unlock();
> -        }
> -        return res;
> +    if (save_zero_page(rs, pss, block, offset)) {
> +        return 1;

These two additions should be in patch 5.

>      }
>  
>      /*

[PATCH 4/5] migration/ram: Return early from save_zero_page

[Fabiano Rosas]

Invert the first conditional so we return early when len == 0. This is
merely to make the next patch easier to read.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/ram.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index a10410a1a5..8ec38f69e8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1169,23 +1169,24 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
 {
     int len = save_zero_page_to_file(pss, block, offset);
 
-    if (len) {
-        stat64_add(&mig_stats.zero_pages, 1);
-        ram_transferred_add(len);
+    if (!len) {
+        return -1;
+    }
 
-        /*
-         * Must let xbzrle know, otherwise a previous (now 0'd) cached
-         * page would be stale.
-         */
-        if (rs->xbzrle_started) {
-            XBZRLE_cache_lock();
-            xbzrle_cache_zero_page(block->offset + offset);
-            XBZRLE_cache_unlock();
-        }
+    stat64_add(&mig_stats.zero_pages, 1);
+    ram_transferred_add(len);
 
-        return 1;
+    /*
+     * Must let xbzrle know, otherwise a previous (now 0'd) cached
+     * page would be stale.
+     */
+    if (rs->xbzrle_started) {
+        XBZRLE_cache_lock();
+        xbzrle_cache_zero_page(block->offset + offset);
+        XBZRLE_cache_unlock();
     }
-    return -1;
+
+    return 1;
 }
 
 /*
-- 
2.35.3

Re: [PATCH 4/5] migration/ram: Return early from save_zero_page

[Peter Xu]

On Tue, Aug 15, 2023 at 11:38:27AM -0300, Fabiano Rosas wrote:
> Invert the first conditional so we return early when len == 0. This is
> merely to make the next patch easier to read.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/ram.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index a10410a1a5..8ec38f69e8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1169,23 +1169,24 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>  {
>      int len = save_zero_page_to_file(pss, block, offset);
>  
> -    if (len) {
> -        stat64_add(&mig_stats.zero_pages, 1);
> -        ram_transferred_add(len);
> +    if (!len) {
> +        return -1;
> +    }
>  
> -        /*
> -         * Must let xbzrle know, otherwise a previous (now 0'd) cached
> -         * page would be stale.
> -         */
> -        if (rs->xbzrle_started) {
> -            XBZRLE_cache_lock();
> -            xbzrle_cache_zero_page(block->offset + offset);
> -            XBZRLE_cache_unlock();
> -        }
> +    stat64_add(&mig_stats.zero_pages, 1);
> +    ram_transferred_add(len);
>  
> -        return 1;
> +    /*
> +     * Must let xbzrle know, otherwise a previous (now 0'd) cached
> +     * page would be stale.
> +     */
> +    if (rs->xbzrle_started) {
> +        XBZRLE_cache_lock();
> +        xbzrle_cache_zero_page(block->offset + offset);
> +        XBZRLE_cache_unlock();
>      }
> -    return -1;
> +
> +    return 1;
>  }

Can we squash this into previous patch?

-- 
Peter Xu

[PATCH 5/5] migration/ram: Merge save_zero_page functions

[Fabiano Rosas]

We don't need to do this in two pieces. One single function makes it
easier to grasp, specially since it removes the indirection on the
return value handling.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/ram.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 8ec38f69e8..13935ead1c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset)
     ram_discard_range(rbname, offset, TARGET_PAGE_SIZE);
 }
 
-/**
- * save_zero_page_to_file: send the zero page to the file
- *
- * Returns the size of data written to the file, 0 means the page is not
- * a zero page
- *
- * @pss: current PSS channel
- * @block: block that contains the page we want to send
- * @offset: offset inside the block for the page
- */
-static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
-                                  ram_addr_t offset)
-{
-    uint8_t *p = block->host + offset;
-    QEMUFile *file = pss->pss_channel;
-    int len = 0;
-
-    if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
-        len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
-        qemu_put_byte(file, 0);
-        len += 1;
-        ram_release_page(block->idstr, offset);
-    }
-    return len;
-}
-
 /**
  * save_zero_page: send the zero page to the stream
  *
@@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
 static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
                           ram_addr_t offset)
 {
-    int len = save_zero_page_to_file(pss, block, offset);
+    uint8_t *p = block->host + offset;
+    QEMUFile *file = pss->pss_channel;
+    int len = 0;
 
-    if (!len) {
-        return -1;
+    if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
+        return 0;
     }
 
+    len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
+    qemu_put_byte(file, 0);
+    len += 1;
+    ram_release_page(block->idstr, offset);
+
     stat64_add(&mig_stats.zero_pages, 1);
     ram_transferred_add(len);
 
@@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
         XBZRLE_cache_unlock();
     }
 
-    return 1;
+    return len;
 }
 
 /*
-- 
2.35.3

Re: [PATCH 5/5] migration/ram: Merge save_zero_page functions

[Peter Xu]

On Tue, Aug 15, 2023 at 11:38:28AM -0300, Fabiano Rosas wrote:
> We don't need to do this in two pieces. One single function makes it
> easier to grasp, specially since it removes the indirection on the
> return value handling.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/ram.c | 41 +++++++++++------------------------------
>  1 file changed, 11 insertions(+), 30 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 8ec38f69e8..13935ead1c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset)
>      ram_discard_range(rbname, offset, TARGET_PAGE_SIZE);
>  }
>  
> -/**
> - * save_zero_page_to_file: send the zero page to the file
> - *
> - * Returns the size of data written to the file, 0 means the page is not
> - * a zero page
> - *
> - * @pss: current PSS channel
> - * @block: block that contains the page we want to send
> - * @offset: offset inside the block for the page
> - */
> -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
> -                                  ram_addr_t offset)
> -{
> -    uint8_t *p = block->host + offset;
> -    QEMUFile *file = pss->pss_channel;
> -    int len = 0;
> -
> -    if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> -        len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
> -        qemu_put_byte(file, 0);
> -        len += 1;
> -        ram_release_page(block->idstr, offset);
> -    }
> -    return len;
> -}
> -
>  /**
>   * save_zero_page: send the zero page to the stream
>   *
> @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
>  static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>                            ram_addr_t offset)
>  {
> -    int len = save_zero_page_to_file(pss, block, offset);
> +    uint8_t *p = block->host + offset;
> +    QEMUFile *file = pss->pss_channel;
> +    int len = 0;
>  
> -    if (!len) {
> -        return -1;
> +    if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> +        return 0;
>      }
>  
> +    len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
> +    qemu_put_byte(file, 0);
> +    len += 1;
> +    ram_release_page(block->idstr, offset);
> +
>      stat64_add(&mig_stats.zero_pages, 1);
>      ram_transferred_add(len);
>  
> @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>          XBZRLE_cache_unlock();
>      }
>  
> -    return 1;
> +    return len;

I don't think it's correct.. We need to keep the retval definition (how
many pages were sent) rather than returning num of bytes, I think.

I'm curious how did this pass any form of test.. because I think we did
assert that:

            /* Be strict to return code; it must be 1, or what else? */
            if (migration_ops->ram_save_target_page(rs, pss) != 1) {
                error_report_once("%s: ram_save_target_page failed", __func__);
                ret = -1;
                goto out;
            }

Did I miss something?

-- 
Peter Xu

Re: [PATCH 5/5] migration/ram: Merge save_zero_page functions

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Aug 15, 2023 at 11:38:28AM -0300, Fabiano Rosas wrote:
>> We don't need to do this in two pieces. One single function makes it
>> easier to grasp, specially since it removes the indirection on the
>> return value handling.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/ram.c | 41 +++++++++++------------------------------
>>  1 file changed, 11 insertions(+), 30 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 8ec38f69e8..13935ead1c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t offset)
>>      ram_discard_range(rbname, offset, TARGET_PAGE_SIZE);
>>  }
>>  
>> -/**
>> - * save_zero_page_to_file: send the zero page to the file
>> - *
>> - * Returns the size of data written to the file, 0 means the page is not
>> - * a zero page
>> - *
>> - * @pss: current PSS channel
>> - * @block: block that contains the page we want to send
>> - * @offset: offset inside the block for the page
>> - */
>> -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
>> -                                  ram_addr_t offset)
>> -{
>> -    uint8_t *p = block->host + offset;
>> -    QEMUFile *file = pss->pss_channel;
>> -    int len = 0;
>> -
>> -    if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> -        len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
>> -        qemu_put_byte(file, 0);
>> -        len += 1;
>> -        ram_release_page(block->idstr, offset);
>> -    }
>> -    return len;
>> -}
>> -
>>  /**
>>   * save_zero_page: send the zero page to the stream
>>   *
>> @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
>>  static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>>                            ram_addr_t offset)
>>  {
>> -    int len = save_zero_page_to_file(pss, block, offset);
>> +    uint8_t *p = block->host + offset;
>> +    QEMUFile *file = pss->pss_channel;
>> +    int len = 0;
>>  
>> -    if (!len) {
>> -        return -1;
>> +    if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> +        return 0;
>>      }
>>  
>> +    len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
>> +    qemu_put_byte(file, 0);
>> +    len += 1;
>> +    ram_release_page(block->idstr, offset);
>> +
>>      stat64_add(&mig_stats.zero_pages, 1);
>>      ram_transferred_add(len);
>>  
>> @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock *block,
>>          XBZRLE_cache_unlock();
>>      }
>>  
>> -    return 1;
>> +    return len;
>
> I don't think it's correct.. We need to keep the retval definition (how
> many pages were sent) rather than returning num of bytes, I think.
>
> I'm curious how did this pass any form of test.. because I think we did
> assert that:
>
>             /* Be strict to return code; it must be 1, or what else? */
>             if (migration_ops->ram_save_target_page(rs, pss) != 1) {
>                 error_report_once("%s: ram_save_target_page failed", __func__);
>                 ret = -1;
>                 goto out;
>             }
>
> Did I miss something?

Kind of, this code is correct. It's just that I made save_zero_page()
return bytes like save_zero_page_to_file() used to do and made
ram_save_target_page() return 1 instead of passing the value from
save_zero_page() along.

But there's a bug in patch 3 because what I described above should only
happen in this patch 5.