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

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> On Tue, Jul 23, 2019 at 05:47:03PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (address@hidden) 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 <address@hidden>
> >> ---
> >>  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.

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 <address@hidden>
> >
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK