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

[Wei Yang <richard.weiyang@gmail.com>]

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