On Fri, Jun 07, 2019 at 07:57:34PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@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.y...@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 / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
