Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

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

On Wed, Jul 24, 2019 at 01:10:24PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Tue, Jul 23, 2019 at 05:47:03PM +0100, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> >> After cleanup, it would be clear to audience there are two cases
>> >> ram_load:
>> >> 
>> >>   * precopy
>> >>   * postcopy
>> >> 
>> >> And it is not necessary to check postcopy_running on each iteration for
>> >> precopy.
>> >> 
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> ---
>> >>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
>> >>  1 file changed, 46 insertions(+), 27 deletions(-)
>> >> 
>> >> diff --git a/migration/ram.c b/migration/ram.c
>> >> index 6bfdfae16e..5f6f07b255 100644
>> >> --- a/migration/ram.c
>> >> +++ b/migration/ram.c
>> >> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
>> >>      trace_colo_flush_ram_cache_end();
>> >>  }
>> >>  
>> >> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
>> >> +/**
>> >> + * ram_load_precopy: load a page in precopy case
>> >
>> >This comment is wrong - although I realise you copied it from the
>> >postcopy case; they don't just load a single page; they load 'pages'
>> >
>> 
>> Thanks for pointing out.
>> 
>> Actually, I got one confusion in these two load. Compare these two cases, I
>> found precopy would handle two more cases:
>> 
>>   * precopy:  RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>>               RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE
>>   * postcopy: RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE
>> 
>> Why postcopy doesn't need to handle the other two cases? Function
>> ram_save_target_page() does the same thing in precopy and postcopy. I don't
>> find the reason the behavior differs. Would you mind giving me a hint?
>
>Because we don't support either compression or xbzrle with postcopy.
>Compression could be fixed, but it needs to make sure it uses the
>place-page function to atomically place the page.
>

Thanks for the explanation. Sounds I missed some point.

The place-page function to use is postcopy_place_page()?

>xbzrle never gets used during the postcopy stage; it gets used
>in the precopy stage in a migration that might switch to postcopy
>though.  Since xbzrle relies on optimising differences between
>passes, it's
>   1) Not needed in postcopy where there's only one final pass
>   2) Since the destination is changing RAM, you can't transmit
>      deltas relative to the old data, since that data may have
>      changed.
>
>Dave
>
>> >Other than that, I think it's OK, so:
>> >
>> >
>> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me