[Qemu-devel] [PATCH 0/2] xbzrle: improve readability a little

[Qemu-devel] [PATCH 0/2] xbzrle: improve readability a little

[Wei Yang]

Two trivial patches to make save_xbzrle_page() a little bit easy to
understand.

Wei Yang (2):
  migration/xbzrle: update cache and current_data in one place
  migration/xbzrle: cleanup the handling cache miss condition

 migration/ram.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] migration/xbzrle: update cache and current_data in one place

[Wei Yang]

When we are not in the last_stage, we need to update the cache if page
is not the same.

Currently this procedure is scattered in two places and mixed with
encoding status check.

This patch extract this general step out to make the code a little bit
easy to read.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index e9b40d636d..878cd8de7a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
                                        TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
                                        TARGET_PAGE_SIZE);
+
+    /*
+     * we need to update the data in the cache, in order to get the same data
+     */
+    if (!last_stage && encoded_len != 0) {
+        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
+        *current_data = prev_cached_page;
+    }
+
     if (encoded_len == 0) {
         trace_save_xbzrle_page_skipping();
         return 0;
     } else if (encoded_len == -1) {
         trace_save_xbzrle_page_overflow();
         xbzrle_counters.overflow++;
-        /* update data in the cache */
-        if (!last_stage) {
-            memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE);
-            *current_data = prev_cached_page;
-        }
         return -1;
     }
 
-    /* we need to update the data in the cache, in order to get the same data */
-    if (!last_stage) {
-        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
-    }
-
     /* Send XBZRLE based compressed page */
     bytes_xbzrle = save_page_header(rs, rs->f, block,
                                     offset | RAM_SAVE_FLAG_XBZRLE);
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/2] migration/xbzrle: update cache and current_data in one place

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> When we are not in the last_stage, we need to update the cache if page
> is not the same.
> 
> Currently this procedure is scattered in two places and mixed with
> encoding status check.
> 
> This patch extract this general step out to make the code a little bit
> easy to read.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

This function is actually quite subtle, and I think your change will
work, but it has changed the behaviour slightly.

When we enter the function the *current_data is pointing at actual guest
RAM and is changing as we're running.
It's critical that the contents of the cache match what was actually
sent, so that in the next cycle the correct delta is generated;
thus the reason for the two memcpy's is actually different.

> ---
>  migration/ram.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index e9b40d636d..878cd8de7a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
>                                         TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
>                                         TARGET_PAGE_SIZE);
> +
> +    /*
> +     * we need to update the data in the cache, in order to get the same data
> +     */
> +    if (!last_stage && encoded_len != 0) {
> +        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
> +        *current_data = prev_cached_page;
> +    }
> +
>      if (encoded_len == 0) {
>          trace_save_xbzrle_page_skipping();
>          return 0;
>      } else if (encoded_len == -1) {
>          trace_save_xbzrle_page_overflow();
>          xbzrle_counters.overflow++;
> -        /* update data in the cache */
> -        if (!last_stage) {
> -            memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE);
> -            *current_data = prev_cached_page;
> -        }
>          return -1;

In this case, we've not managed to compress, so we're going to have to
transmit the whole page; but remember the guest is still writing. So we
update the cache, and then update *current_data to point to the cache
so that the caller sends from the cache directly rather than the guest
RAM, this ensures that the thing that's sent matches the cache contents.

However, note the memcpy is from *current_data, not XBZRLE.current_buf,
so it might be slightly newer  - this is the first change you have made;
you might be sending data that's slightly older; but it's safe because
the data sent does match the cache contents.

>      }
>  
> -    /* we need to update the data in the cache, in order to get the same data */
> -    if (!last_stage) {
> -        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
> -    }
> -

This memcpy is slightly different.
Here we've managed to do the compress; so now we update the cache based
on what was compressed (current_buf).  *current_data isn't used by the
caller in this case because it's actually sending the compressed data.
So it's safe for you to update it.

So I think we need to add comments like that, how about:

       /*
        * Update the cache contents, so that it corresponds to the data
        * sent, in allc ases except where we skip the page.
        */
> +    if (!last_stage && encoded_len != 0) {
> +        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
       /*
        * In the case where we couldn't compress, ensure that the caller
        * sends the data from the cache, since the guest might have
        * changed the RAM since we copied it.
        */

> +        *current_data = prev_cached_page;
> +    }
>      /* Send XBZRLE based compressed page */
>      bytes_xbzrle = save_page_header(rs, rs->f, block,
>                                      offset | RAM_SAVE_FLAG_XBZRLE);

Dave

> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] migration/xbzrle: update cache and current_data in one place

[Wei Yang]

On Fri, Jun 07, 2019 at 07:57:34PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> When we are not in the last_stage, we need to update the cache if page
>> is not the same.
>> 
>> Currently this procedure is scattered in two places and mixed with
>> encoding status check.
>> 
>> This patch extract this general step out to make the code a little bit
>> easy to read.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>This function is actually quite subtle, and I think your change will
>work, but it has changed the behaviour slightly.
>
>When we enter the function the *current_data is pointing at actual guest
>RAM and is changing as we're running.
>It's critical that the contents of the cache match what was actually
>sent, so that in the next cycle the correct delta is generated;
>thus the reason for the two memcpy's is actually different.
>
>> ---
>>  migration/ram.c | 19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index e9b40d636d..878cd8de7a 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1594,25 +1594,24 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>>      encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
>>                                         TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
>>                                         TARGET_PAGE_SIZE);
>> +
>> +    /*
>> +     * we need to update the data in the cache, in order to get the same data
>> +     */
>> +    if (!last_stage && encoded_len != 0) {
>> +        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
>> +        *current_data = prev_cached_page;
>> +    }
>> +
>>      if (encoded_len == 0) {
>>          trace_save_xbzrle_page_skipping();
>>          return 0;
>>      } else if (encoded_len == -1) {
>>          trace_save_xbzrle_page_overflow();
>>          xbzrle_counters.overflow++;
>> -        /* update data in the cache */
>> -        if (!last_stage) {
>> -            memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE);
>> -            *current_data = prev_cached_page;
>> -        }
>>          return -1;
>
>In this case, we've not managed to compress, so we're going to have to
>transmit the whole page; but remember the guest is still writing. So we
>update the cache, and then update *current_data to point to the cache
>so that the caller sends from the cache directly rather than the guest
>RAM, this ensures that the thing that's sent matches the cache contents.
>
>However, note the memcpy is from *current_data, not XBZRLE.current_buf,
>so it might be slightly newer  - this is the first change you have made;
>you might be sending data that's slightly older; but it's safe because
>the data sent does match the cache contents.
>
>>      }
>>  
>> -    /* we need to update the data in the cache, in order to get the same data */
>> -    if (!last_stage) {
>> -        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
>> -    }
>> -
>
>This memcpy is slightly different.
>Here we've managed to do the compress; so now we update the cache based
>on what was compressed (current_buf).  *current_data isn't used by the
>caller in this case because it's actually sending the compressed data.
>So it's safe for you to update it.

Yes, you are right. My change will alter the behavior a little. To be
specific, when we didn't manage to compress content, the content we sent will
be a little *old*.

>
>So I think we need to add comments like that, how about:
>
>       /*
>        * Update the cache contents, so that it corresponds to the data
>        * sent, in allc ases except where we skip the page.
>        */
>> +    if (!last_stage && encoded_len != 0) {
>> +        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
>       /*
>        * In the case where we couldn't compress, ensure that the caller
>        * sends the data from the cache, since the guest might have
>        * changed the RAM since we copied it.
>        */
>
>> +        *current_data = prev_cached_page;
>> +    }
>>      /* Send XBZRLE based compressed page */
>>      bytes_xbzrle = save_page_header(rs, rs->f, block,
>>                                      offset | RAM_SAVE_FLAG_XBZRLE);

Yep, I agree with this comment.

Let me add this in v2.

Thanks :-)

>
>Dave
>
>> -- 
>> 2.19.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [PATCH 2/2] migration/xbzrle: cleanup the handling cache miss condition

[Wei Yang]

For cache miss condition not in last_stage, we need to insert data into
cache. When this step succeed, current_data should be updated. While no
matter these checks pass or not, -1 is returned.

Based on this, the logic in cache miss handling could be simplified a
little.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 878cd8de7a..67ba075cc4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1572,15 +1572,14 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     if (!cache_is_cached(XBZRLE.cache, current_addr,
                          ram_counters.dirty_sync_count)) {
         xbzrle_counters.cache_miss++;
-        if (!last_stage) {
-            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
-                             ram_counters.dirty_sync_count) == -1) {
-                return -1;
-            } else {
-                /* update *current_data when the page has been
-                   inserted into cache */
-                *current_data = get_cached_data(XBZRLE.cache, current_addr);
-            }
+        if (!last_stage &&
+            !cache_insert(XBZRLE.cache, current_addr, *current_data,
+                          ram_counters.dirty_sync_count)) {
+            /*
+             * update *current_data when the page has been inserted into
+             * cache
+             */
+            *current_data = get_cached_data(XBZRLE.cache, current_addr);
         }
         return -1;
     }
-- 
2.19.1

Re: [Qemu-devel] [PATCH 2/2] migration/xbzrle: cleanup the handling cache miss condition

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> For cache miss condition not in last_stage, we need to insert data into
> cache. When this step succeed, current_data should be updated. While no
> matter these checks pass or not, -1 is returned.
> 
> Based on this, the logic in cache miss handling could be simplified a
> little.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 878cd8de7a..67ba075cc4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1572,15 +1572,14 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      if (!cache_is_cached(XBZRLE.cache, current_addr,
>                           ram_counters.dirty_sync_count)) {
>          xbzrle_counters.cache_miss++;
> -        if (!last_stage) {
> -            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
> -                             ram_counters.dirty_sync_count) == -1) {
> -                return -1;
> -            } else {
> -                /* update *current_data when the page has been
> -                   inserted into cache */
> -                *current_data = get_cached_data(XBZRLE.cache, current_addr);
> -            }
> +        if (!last_stage &&
> +            !cache_insert(XBZRLE.cache, current_addr, *current_data,
> +                          ram_counters.dirty_sync_count)) {
> +            /*
> +             * update *current_data when the page has been inserted into
> +             * cache
> +             */
> +            *current_data = get_cached_data(XBZRLE.cache, current_addr);

No this change doesn't gain anything and I find the original easier to
read.

This function is really subtle, every time I do anything with it I have
to think really hard about it, so ease of reading is more important.

Dave

>          }
>          return -1;
>      }
> -- 
> 2.19.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] migration/xbzrle: cleanup the handling cache miss condition

[Wei Yang]

On Fri, Jun 07, 2019 at 08:01:14PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> For cache miss condition not in last_stage, we need to insert data into
>> cache. When this step succeed, current_data should be updated. While no
>> matter these checks pass or not, -1 is returned.
>> 
>> Based on this, the logic in cache miss handling could be simplified a
>> little.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 878cd8de7a..67ba075cc4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1572,15 +1572,14 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>>      if (!cache_is_cached(XBZRLE.cache, current_addr,
>>                           ram_counters.dirty_sync_count)) {
>>          xbzrle_counters.cache_miss++;
>> -        if (!last_stage) {
>> -            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
>> -                             ram_counters.dirty_sync_count) == -1) {
>> -                return -1;
>> -            } else {
>> -                /* update *current_data when the page has been
>> -                   inserted into cache */
>> -                *current_data = get_cached_data(XBZRLE.cache, current_addr);
>> -            }
>> +        if (!last_stage &&
>> +            !cache_insert(XBZRLE.cache, current_addr, *current_data,
>> +                          ram_counters.dirty_sync_count)) {
>> +            /*
>> +             * update *current_data when the page has been inserted into
>> +             * cache
>> +             */
>> +            *current_data = get_cached_data(XBZRLE.cache, current_addr);
>
>No this change doesn't gain anything and I find the original easier to
>read.
>
>This function is really subtle, every time I do anything with it I have
>to think really hard about it, so ease of reading is more important.
>

Yep, I agree ease of reading is more important.

Since the original version looks better, I will keep current code. :-)

>Dave
>
>>          }
>>          return -1;
>>      }
>> -- 
>> 2.19.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me