[1/2] migration/xbzrle: update cache and current_data in one place
diff mbox series

Message ID 20190606013138.13312-2-richardw.yang@linux.intel.com
State New
Headers show
Series
  • xbzrle: improve readability a little
Related show

Commit Message

Wei Yang June 6, 2019, 1:31 a.m. UTC
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(-)

Comments

Dr. David Alan Gilbert June 7, 2019, 6:57 p.m. UTC | #1
* 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
Wei Yang June 9, 2019, 7:46 p.m. UTC | #2
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

Patch
diff mbox series

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);