| Message ID | 20190722075339.25121-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: cleanup ram_load | expand |
* 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' Other than that, I think it's OK, so: Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > + * Returns 0 for success or -errno in case of error > + * > + * Called in precopy mode by ram_load(). > + * rcu_read_lock is taken prior to this being called. > + * > + * @f: QEMUFile where to send the data > + */ > +static int ram_load_precopy(QEMUFile *f) > { > - int flags = 0, ret = 0, invalid_flags = 0; > - static uint64_t seq_iter; > - int len = 0; > - /* > - * If system is running in postcopy mode, page inserts to host memory must > - * be atomic > - */ > - bool postcopy_running = postcopy_is_running(); > + int flags = 0, ret = 0, invalid_flags = 0, len = 0; > /* ADVISE is earlier, it shows the source has the postcopy capability on */ > bool postcopy_advised = postcopy_is_advised(); > - > - seq_iter++; > - > - if (version_id != 4) { > - return -EINVAL; > - } > - > if (!migrate_use_compression()) { > invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE; > } > - /* This RCU critical section can be very long running. > - * When RCU reclaims in the code start to become numerous, > - * it will be necessary to reduce the granularity of this > - * critical section. > - */ > - rcu_read_lock(); > - > - if (postcopy_running) { > - ret = ram_load_postcopy(f); > - } > > - while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) { > + while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { > ram_addr_t addr, total_ram_bytes; > void *host = NULL; > uint8_t ch; > @@ -4390,6 +4376,39 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > } > } > > + return ret; > +} > + > +static int ram_load(QEMUFile *f, void *opaque, int version_id) > +{ > + int ret = 0; > + static uint64_t seq_iter; > + /* > + * If system is running in postcopy mode, page inserts to host memory must > + * be atomic > + */ > + bool postcopy_running = postcopy_is_running(); > + > + seq_iter++; > + > + if (version_id != 4) { > + return -EINVAL; > + } > + > + /* > + * This RCU critical section can be very long running. > + * When RCU reclaims in the code start to become numerous, > + * it will be necessary to reduce the granularity of this > + * critical section. > + */ > + rcu_read_lock(); > + > + if (postcopy_running) { > + ret = ram_load_postcopy(f); > + } else { > + ret = ram_load_precopy(f); > + } > + > ret |= wait_for_decompress_done(); > rcu_read_unlock(); > trace_ram_load_complete(ret, seq_iter); > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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? >Other than that, I think it's OK, so: > > >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >
* 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. 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
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
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 + * + * Returns 0 for success or -errno in case of error + * + * Called in precopy mode by ram_load(). + * rcu_read_lock is taken prior to this being called. + * + * @f: QEMUFile where to send the data + */ +static int ram_load_precopy(QEMUFile *f) { - int flags = 0, ret = 0, invalid_flags = 0; - static uint64_t seq_iter; - int len = 0; - /* - * If system is running in postcopy mode, page inserts to host memory must - * be atomic - */ - bool postcopy_running = postcopy_is_running(); + int flags = 0, ret = 0, invalid_flags = 0, len = 0; /* ADVISE is earlier, it shows the source has the postcopy capability on */ bool postcopy_advised = postcopy_is_advised(); - - seq_iter++; - - if (version_id != 4) { - return -EINVAL; - } - if (!migrate_use_compression()) { invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE; } - /* This RCU critical section can be very long running. - * When RCU reclaims in the code start to become numerous, - * it will be necessary to reduce the granularity of this - * critical section. - */ - rcu_read_lock(); - - if (postcopy_running) { - ret = ram_load_postcopy(f); - } - while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) { + while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { ram_addr_t addr, total_ram_bytes; void *host = NULL; uint8_t ch; @@ -4390,6 +4376,39 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) } } + return ret; +} + +static int ram_load(QEMUFile *f, void *opaque, int version_id) +{ + int ret = 0; + static uint64_t seq_iter; + /* + * If system is running in postcopy mode, page inserts to host memory must + * be atomic + */ + bool postcopy_running = postcopy_is_running(); + + seq_iter++; + + if (version_id != 4) { + return -EINVAL; + } + + /* + * This RCU critical section can be very long running. + * When RCU reclaims in the code start to become numerous, + * it will be necessary to reduce the granularity of this + * critical section. + */ + rcu_read_lock(); + + if (postcopy_running) { + ret = ram_load_postcopy(f); + } else { + ret = ram_load_precopy(f); + } + ret |= wait_for_decompress_done(); rcu_read_unlock(); trace_ram_load_complete(ret, seq_iter);
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(-)