| Message ID | 1412358473-31398-25-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 03, 2014 at 06:47:30PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Use that to split the qemu_savevm_state_pending counts into postcopiable > and non-postcopiable amounts > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > arch_init.c | 7 +++++++ > include/migration/vmstate.h | 2 +- > include/sysemu/sysemu.h | 4 +++- > migration.c | 9 ++++++++- > savevm.c | 23 +++++++++++++++++++---- > 5 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 6970733..44072d8 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -1192,6 +1192,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > return ret; > } > > +/* RAM's always up for postcopying */ > +static bool ram_can_postcopy(void *opaque) > +{ > + return true; > +} > + > static SaveVMHandlers savevm_ram_handlers = { > .save_live_setup = ram_save_setup, > .save_live_iterate = ram_save_iterate, > @@ -1199,6 +1205,7 @@ static SaveVMHandlers savevm_ram_handlers = { > .save_live_pending = ram_save_pending, > .load_state = ram_load, > .cancel = ram_migration_cancel, > + .can_postcopy = ram_can_postcopy, Is there actually any plausible device for which you'd need a callback here, rather than just having a static bool? On the other hand, it does seem kind of plausible that there might be situations in which some data from a device must be pre-copied, but more can be post-copied, which would necessitate extending the per-handler callback to return quantities for both.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Fri, Oct 03, 2014 at 06:47:30PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Use that to split the qemu_savevm_state_pending counts into postcopiable > > and non-postcopiable amounts > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > arch_init.c | 7 +++++++ > > include/migration/vmstate.h | 2 +- > > include/sysemu/sysemu.h | 4 +++- > > migration.c | 9 ++++++++- > > savevm.c | 23 +++++++++++++++++++---- > > 5 files changed, 38 insertions(+), 7 deletions(-) > > > > diff --git a/arch_init.c b/arch_init.c > > index 6970733..44072d8 100644 > > --- a/arch_init.c > > +++ b/arch_init.c > > @@ -1192,6 +1192,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > return ret; > > } > > > > +/* RAM's always up for postcopying */ > > +static bool ram_can_postcopy(void *opaque) > > +{ > > + return true; > > +} > > + > > static SaveVMHandlers savevm_ram_handlers = { > > .save_live_setup = ram_save_setup, > > .save_live_iterate = ram_save_iterate, > > @@ -1199,6 +1205,7 @@ static SaveVMHandlers savevm_ram_handlers = { > > .save_live_pending = ram_save_pending, > > .load_state = ram_load, > > .cancel = ram_migration_cancel, > > + .can_postcopy = ram_can_postcopy, > > Is there actually any plausible device for which you'd need a callback > here, rather than just having a static bool? > > On the other hand, it does seem kind of plausible that there might be > situations in which some data from a device must be pre-copied, but > more can be post-copied, which would necessitate extending the > per-handler callback to return quantities for both. It's cheap enough and I couldn't make a strong argument about any possible device, so I just used the function. Dave > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Nov 19, 2014 at 05:53:54PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Fri, Oct 03, 2014 at 06:47:30PM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Use that to split the qemu_savevm_state_pending counts into postcopiable > > > and non-postcopiable amounts > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > arch_init.c | 7 +++++++ > > > include/migration/vmstate.h | 2 +- > > > include/sysemu/sysemu.h | 4 +++- > > > migration.c | 9 ++++++++- > > > savevm.c | 23 +++++++++++++++++++---- > > > 5 files changed, 38 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch_init.c b/arch_init.c > > > index 6970733..44072d8 100644 > > > --- a/arch_init.c > > > +++ b/arch_init.c > > > @@ -1192,6 +1192,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > > return ret; > > > } > > > > > > +/* RAM's always up for postcopying */ > > > +static bool ram_can_postcopy(void *opaque) > > > +{ > > > + return true; > > > +} > > > + > > > static SaveVMHandlers savevm_ram_handlers = { > > > .save_live_setup = ram_save_setup, > > > .save_live_iterate = ram_save_iterate, > > > @@ -1199,6 +1205,7 @@ static SaveVMHandlers savevm_ram_handlers = { > > > .save_live_pending = ram_save_pending, > > > .load_state = ram_load, > > > .cancel = ram_migration_cancel, > > > + .can_postcopy = ram_can_postcopy, > > > > Is there actually any plausible device for which you'd need a callback > > here, rather than just having a static bool? > > > > On the other hand, it does seem kind of plausible that there might be > > situations in which some data from a device must be pre-copied, but > > more can be post-copied, which would necessitate extending the > > per-handler callback to return quantities for both. > > It's cheap enough and I couldn't make a strong argument about > any possible device, so I just used the function. Ok. I still wonder if it might be better to instead extend the save_live_pending callback in order to return both non-postcopyable and postcopyable quantites. It allows for the case of a postcopyable device which has some non-postcopyable data - and with any postcopyable device other than RAM, it seems likely that there will need to be some precopied metadata at least. Plus it avoids adding another callback.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Wed, Nov 19, 2014 at 05:53:54PM +0000, Dr. David Alan Gilbert wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Fri, Oct 03, 2014 at 06:47:30PM +0100, Dr. David Alan Gilbert (git) wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > Use that to split the qemu_savevm_state_pending counts into postcopiable > > > > and non-postcopiable amounts > > > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > --- > > > > arch_init.c | 7 +++++++ > > > > include/migration/vmstate.h | 2 +- > > > > include/sysemu/sysemu.h | 4 +++- > > > > migration.c | 9 ++++++++- > > > > savevm.c | 23 +++++++++++++++++++---- > > > > 5 files changed, 38 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/arch_init.c b/arch_init.c > > > > index 6970733..44072d8 100644 > > > > --- a/arch_init.c > > > > +++ b/arch_init.c > > > > @@ -1192,6 +1192,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > > > return ret; > > > > } > > > > > > > > +/* RAM's always up for postcopying */ > > > > +static bool ram_can_postcopy(void *opaque) > > > > +{ > > > > + return true; > > > > +} > > > > + > > > > static SaveVMHandlers savevm_ram_handlers = { > > > > .save_live_setup = ram_save_setup, > > > > .save_live_iterate = ram_save_iterate, > > > > @@ -1199,6 +1205,7 @@ static SaveVMHandlers savevm_ram_handlers = { > > > > .save_live_pending = ram_save_pending, > > > > .load_state = ram_load, > > > > .cancel = ram_migration_cancel, > > > > + .can_postcopy = ram_can_postcopy, > > > > > > Is there actually any plausible device for which you'd need a callback > > > here, rather than just having a static bool? > > > > > > On the other hand, it does seem kind of plausible that there might be > > > situations in which some data from a device must be pre-copied, but > > > more can be post-copied, which would necessitate extending the > > > per-handler callback to return quantities for both. > > > > It's cheap enough and I couldn't make a strong argument about > > any possible device, so I just used the function. > > Ok. I still wonder if it might be better to instead extend > the save_live_pending callback in order to return both > non-postcopyable and postcopyable quantites. It allows for the case > of a postcopyable device which has some non-postcopyable data - and > with any postcopyable device other than RAM, it seems likely that > there will need to be some precopied metadata at least. Plus it > avoids adding another callback. There are two separate suggestions there - which I'll address in opposite order: 1) Extending save_live_pending callback to avoid a new callback can_postcopy is used for a few diferent decisions; not just where to add the pending value to; so I don't think extending s_l_p saves the need for the other callback 2) Allowing a device to do both pre and postcopy Yeh I can see you could theoretically have a device where that would be useful; but for reasonably small chunks of metadata it already gets the chance to send those during the _begin call. I'll make the change to save_live_pending's parameters to let this work. Dave > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/arch_init.c b/arch_init.c index 6970733..44072d8 100644 --- a/arch_init.c +++ b/arch_init.c @@ -1192,6 +1192,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) return ret; } +/* RAM's always up for postcopying */ +static bool ram_can_postcopy(void *opaque) +{ + return true; +} + static SaveVMHandlers savevm_ram_handlers = { .save_live_setup = ram_save_setup, .save_live_iterate = ram_save_iterate, @@ -1199,6 +1205,7 @@ static SaveVMHandlers savevm_ram_handlers = { .save_live_pending = ram_save_pending, .load_state = ram_load, .cancel = ram_migration_cancel, + .can_postcopy = ram_can_postcopy, }; void ram_mig_init(void) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 9a001bd..4991935 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -54,7 +54,7 @@ typedef struct SaveVMHandlers { /* This runs outside the iothread lock! */ int (*save_live_setup)(QEMUFile *f, void *opaque); uint64_t (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size); - + bool (*can_postcopy)(void *opaque); LoadStateHandler *load_state; } SaveVMHandlers; diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index ef98fa9..e7ff3d0 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -110,7 +110,9 @@ void qemu_savevm_state_begin(QEMUFile *f, int qemu_savevm_state_iterate(QEMUFile *f); void qemu_savevm_state_complete(QEMUFile *f); void qemu_savevm_state_cancel(void); -uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size); +void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size, + uint64_t *res_non_postcopiable, + uint64_t *res_postcopiable); void qemu_savevm_command_send(QEMUFile *f, enum qemu_vm_cmd command, uint16_t len, uint8_t *data); void qemu_savevm_send_reqack(QEMUFile *f, uint32_t value); diff --git a/migration.c b/migration.c index 3a45b2a..bca397d 100644 --- a/migration.c +++ b/migration.c @@ -865,8 +865,15 @@ static void *migration_thread(void *opaque) uint64_t pending_size; if (!qemu_file_rate_limit(s->file)) { - pending_size = qemu_savevm_state_pending(s->file, max_size); + uint64_t pend_post, pend_nonpost; + DPRINTF("iterate\n"); + qemu_savevm_state_pending(s->file, max_size, &pend_nonpost, + &pend_post); + pending_size = pend_nonpost + pend_post; trace_migrate_pending(pending_size, max_size); + DPRINTF("pending size %" PRIu64 " max %" PRIu64 " (post=%" PRIu64 + " nonpost=%" PRIu64 ")\n", + pending_size, max_size, pend_post, pend_nonpost); if (pending_size && pending_size >= max_size) { qemu_savevm_state_iterate(s->file); } else { diff --git a/savevm.c b/savevm.c index a368a25..1642a59 100644 --- a/savevm.c +++ b/savevm.c @@ -911,10 +911,18 @@ void qemu_savevm_state_complete(QEMUFile *f) qemu_fflush(f); } -uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size) +/* Give an estimate of the amount left to be transferred, + * the result is split into the amount for units that can and + * for units that can't do postcopy. + */ +void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size, + uint64_t *res_non_postcopiable, + uint64_t *res_postcopiable) { SaveStateEntry *se; - uint64_t ret = 0; + uint64_t res_nonpc = 0; + uint64_t res_pc = 0; + uint64_t tmp; QTAILQ_FOREACH(se, &savevm_handlers, entry) { if (!se->ops || !se->ops->save_live_pending) { @@ -925,9 +933,16 @@ uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size) continue; } } - ret += se->ops->save_live_pending(f, se->opaque, max_size); + tmp = se->ops->save_live_pending(f, se->opaque, max_size); + + if (se->ops->can_postcopy(se->opaque)) { + res_pc += tmp; + } else { + res_nonpc += tmp; + } } - return ret; + *res_non_postcopiable = res_nonpc; + *res_postcopiable = res_pc; } void qemu_savevm_state_cancel(void)