| Message ID | 1424883128-9841-21-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Modify save_live_pending to return separate postcopiable and > non-postcopiable counts. > > Add 'can_postcopy' to allow a device to state if it can postcopy What's the purpose of the can_postcopy callback? There are no callers in this patch - is it still necessary with the change to save_live_pending? > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > arch_init.c | 15 +++++++++++++-- > include/migration/vmstate.h | 10 ++++++++-- > include/sysemu/sysemu.h | 4 +++- > migration/block.c | 7 +++++-- > migration/migration.c | 9 +++++++-- > savevm.c | 21 +++++++++++++++++---- > trace-events | 2 +- > 7 files changed, 54 insertions(+), 14 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index fe0df0d..7bc5fa6 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -997,7 +997,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > return 0; > } > > -static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > +static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, > + uint64_t *non_postcopiable_pending, > + uint64_t *postcopiable_pending) > { > uint64_t remaining_size; > > @@ -1009,7 +1011,9 @@ static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > qemu_mutex_unlock_iothread(); > remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE; > } > - return remaining_size; > + > + *non_postcopiable_pending = 0; > + *postcopiable_pending = remaining_size; > } > > static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > @@ -1204,6 +1208,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, > @@ -1211,6 +1221,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 18da207..c9ec74a 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -54,8 +54,14 @@ 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); > - > + /* > + * postcopiable_pending must return 0 unless the can_postcopy > + * handler returns true. > + */ > + void (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size, > + uint64_t *non_postcopiable_pending, > + uint64_t *postcopiable_pending); > + bool (*can_postcopy)(void *opaque); > LoadStateHandler *load_state; > } SaveVMHandlers; > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index e83bf80..5f518b3 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -111,7 +111,9 @@ void qemu_savevm_state_header(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_ping(QEMUFile *f, uint32_t value); > diff --git a/migration/block.c b/migration/block.c > index 0c76106..0f6f209 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -754,7 +754,9 @@ static int block_save_complete(QEMUFile *f, void *opaque) > return 0; > } > > -static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > +static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, > + uint64_t *non_postcopiable_pending, > + uint64_t *postcopiable_pending) > { > /* Estimate pending number of bytes to send */ > uint64_t pending; > @@ -773,7 +775,8 @@ static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > qemu_mutex_unlock_iothread(); > > DPRINTF("Enter save live pending %" PRIu64 "\n", pending); > - return pending; > + *non_postcopiable_pending = pending; > + *postcopiable_pending = 0; > } > > static int block_load(QEMUFile *f, void *opaque, int version_id) > diff --git a/migration/migration.c b/migration/migration.c > index 2e6adca..a4fc7d7 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -868,8 +868,13 @@ 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); > - trace_migrate_pending(pending_size, max_size); > + uint64_t pend_post, pend_nonpost; > + > + 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, > + 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 df48ba8..e301a0a 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -944,10 +944,20 @@ 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 tmp_non_postcopiable, tmp_postcopiable; > + > + *res_non_postcopiable = 0; > + *res_postcopiable = 0; > + > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > if (!se->ops || !se->ops->save_live_pending) { > @@ -958,9 +968,12 @@ uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size) > continue; > } > } > - ret += se->ops->save_live_pending(f, se->opaque, max_size); > + se->ops->save_live_pending(f, se->opaque, max_size, > + &tmp_non_postcopiable, &tmp_postcopiable); > + > + *res_postcopiable += tmp_postcopiable; > + *res_non_postcopiable += tmp_non_postcopiable; > } > - return ret; > } > > void qemu_savevm_state_cancel(void) > diff --git a/trace-events b/trace-events > index cbf995c..83312b6 100644 > --- a/trace-events > +++ b/trace-events > @@ -1400,7 +1400,7 @@ migrate_fd_cleanup(void) "" > migrate_fd_cleanup_src_rp(void) "" > migrate_fd_error(void) "" > migrate_fd_cancel(void) "" > -migrate_pending(uint64_t size, uint64_t max) "pending size %" PRIu64 " max %" PRIu64 > +migrate_pending(uint64_t size, uint64_t max, uint64_t post, uint64_t nonpost) "pending size %" PRIu64 " max %" PRIu64 " (post=%" PRIu64 " nonpost=%" PRIu64 ")" > migrate_send_rp_message(int cmd, uint16_t len) "cmd=%d, len=%d" > open_outgoing_return_path(void) "" > open_outgoing_return_path_continue(void) ""
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Modify save_live_pending to return separate postcopiable and > > non-postcopiable counts. > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > What's the purpose of the can_postcopy callback? There are no callers > in this patch - is it still necessary with the change to > save_live_pending? The patch 'qemu_savevm_state_complete: Postcopy changes' uses it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete to decide which devices must be completed at that point. Dave > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > arch_init.c | 15 +++++++++++++-- > > include/migration/vmstate.h | 10 ++++++++-- > > include/sysemu/sysemu.h | 4 +++- > > migration/block.c | 7 +++++-- > > migration/migration.c | 9 +++++++-- > > savevm.c | 21 +++++++++++++++++---- > > trace-events | 2 +- > > 7 files changed, 54 insertions(+), 14 deletions(-) > > > > diff --git a/arch_init.c b/arch_init.c > > index fe0df0d..7bc5fa6 100644 > > --- a/arch_init.c > > +++ b/arch_init.c > > @@ -997,7 +997,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > > return 0; > > } > > > > -static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > > +static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, > > + uint64_t *non_postcopiable_pending, > > + uint64_t *postcopiable_pending) > > { > > uint64_t remaining_size; > > > > @@ -1009,7 +1011,9 @@ static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > > qemu_mutex_unlock_iothread(); > > remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE; > > } > > - return remaining_size; > > + > > + *non_postcopiable_pending = 0; > > + *postcopiable_pending = remaining_size; > > } > > > > static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > > @@ -1204,6 +1208,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, > > @@ -1211,6 +1221,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 18da207..c9ec74a 100644 > > --- a/include/migration/vmstate.h > > +++ b/include/migration/vmstate.h > > @@ -54,8 +54,14 @@ 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); > > - > > + /* > > + * postcopiable_pending must return 0 unless the can_postcopy > > + * handler returns true. > > + */ > > + void (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size, > > + uint64_t *non_postcopiable_pending, > > + uint64_t *postcopiable_pending); > > + bool (*can_postcopy)(void *opaque); > > LoadStateHandler *load_state; > > } SaveVMHandlers; > > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > > index e83bf80..5f518b3 100644 > > --- a/include/sysemu/sysemu.h > > +++ b/include/sysemu/sysemu.h > > @@ -111,7 +111,9 @@ void qemu_savevm_state_header(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_ping(QEMUFile *f, uint32_t value); > > diff --git a/migration/block.c b/migration/block.c > > index 0c76106..0f6f209 100644 > > --- a/migration/block.c > > +++ b/migration/block.c > > @@ -754,7 +754,9 @@ static int block_save_complete(QEMUFile *f, void *opaque) > > return 0; > > } > > > > -static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > > +static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, > > + uint64_t *non_postcopiable_pending, > > + uint64_t *postcopiable_pending) > > { > > /* Estimate pending number of bytes to send */ > > uint64_t pending; > > @@ -773,7 +775,8 @@ static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) > > qemu_mutex_unlock_iothread(); > > > > DPRINTF("Enter save live pending %" PRIu64 "\n", pending); > > - return pending; > > + *non_postcopiable_pending = pending; > > + *postcopiable_pending = 0; > > } > > > > static int block_load(QEMUFile *f, void *opaque, int version_id) > > diff --git a/migration/migration.c b/migration/migration.c > > index 2e6adca..a4fc7d7 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -868,8 +868,13 @@ 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); > > - trace_migrate_pending(pending_size, max_size); > > + uint64_t pend_post, pend_nonpost; > > + > > + 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, > > + 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 df48ba8..e301a0a 100644 > > --- a/savevm.c > > +++ b/savevm.c > > @@ -944,10 +944,20 @@ 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 tmp_non_postcopiable, tmp_postcopiable; > > + > > + *res_non_postcopiable = 0; > > + *res_postcopiable = 0; > > + > > > > QTAILQ_FOREACH(se, &savevm_handlers, entry) { > > if (!se->ops || !se->ops->save_live_pending) { > > @@ -958,9 +968,12 @@ uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size) > > continue; > > } > > } > > - ret += se->ops->save_live_pending(f, se->opaque, max_size); > > + se->ops->save_live_pending(f, se->opaque, max_size, > > + &tmp_non_postcopiable, &tmp_postcopiable); > > + > > + *res_postcopiable += tmp_postcopiable; > > + *res_non_postcopiable += tmp_non_postcopiable; > > } > > - return ret; > > } > > > > void qemu_savevm_state_cancel(void) > > diff --git a/trace-events b/trace-events > > index cbf995c..83312b6 100644 > > --- a/trace-events > > +++ b/trace-events > > @@ -1400,7 +1400,7 @@ migrate_fd_cleanup(void) "" > > migrate_fd_cleanup_src_rp(void) "" > > migrate_fd_error(void) "" > > migrate_fd_cancel(void) "" > > -migrate_pending(uint64_t size, uint64_t max) "pending size %" PRIu64 " max %" PRIu64 > > +migrate_pending(uint64_t size, uint64_t max, uint64_t post, uint64_t nonpost) "pending size %" PRIu64 " max %" PRIu64 " (post=%" PRIu64 " nonpost=%" PRIu64 ")" > > migrate_send_rp_message(int cmd, uint16_t len) "cmd=%d, len=%d" > > open_outgoing_return_path(void) "" > > open_outgoing_return_path_continue(void) "" > > -- > 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 Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Modify save_live_pending to return separate postcopiable and > > > non-postcopiable counts. > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > What's the purpose of the can_postcopy callback? There are no callers > > in this patch - is it still necessary with the change to > > save_live_pending? > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > to decide which devices must be completed at that point. Couldn't they check for non-zero postcopiable state from save_live_pending instead?
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > non-postcopiable counts. > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > in this patch - is it still necessary with the change to > > > save_live_pending? > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > to decide which devices must be completed at that point. > > Couldn't they check for non-zero postcopiable state from > save_live_pending instead? That would be a bit weird. At the moment for each device we call the: save_live_setup method (from qemu_savevm_state_begin) 0...multiple times we call: save_live_pending save_live_iterate and then we always call save_live_complete To my mind we have to call save_live_complete for any device that we've called save_live_setup on (maybe it allocated something in _setup that it clears up in _complete). save_live_pending could perfectly well return 0 remaining at the end of the migrate for our device, and thus if we used that then we wouldn't call save_live_complete. 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 Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > non-postcopiable counts. > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > in this patch - is it still necessary with the change to > > > > save_live_pending? > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > to decide which devices must be completed at that point. > > > > Couldn't they check for non-zero postcopiable state from > > save_live_pending instead? > > That would be a bit weird. > > At the moment for each device we call the: > save_live_setup method (from qemu_savevm_state_begin) > > 0...multiple times we call: > save_live_pending > save_live_iterate > > and then we always call > save_live_complete > > > To my mind we have to call save_live_complete for any device > that we've called save_live_setup on (maybe it allocated something > in _setup that it clears up in _complete). > > save_live_pending could perfectly well return 0 remaining at the end of > the migrate for our device, and thus if we used that then we wouldn't > call save_live_complete. Um.. I don't follow. I was suggesting that at the precopy->postcopy transition point you call save_live_complete for everything that reports 0 post-copiable state. Then again, a different approach would be to split the save_live_complete hook into (possibly NULL) "complete precopy" and "complete postcopy" hooks. The core would ensure that every chunk of state has both completion hooks called (unless NULL). That might also address my concerns about the no longer entirely accurate save_live_complete function name.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > non-postcopiable counts. > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > in this patch - is it still necessary with the change to > > > > > save_live_pending? > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > to decide which devices must be completed at that point. > > > > > > Couldn't they check for non-zero postcopiable state from > > > save_live_pending instead? > > > > That would be a bit weird. > > > > At the moment for each device we call the: > > save_live_setup method (from qemu_savevm_state_begin) > > > > 0...multiple times we call: > > save_live_pending > > save_live_iterate > > > > and then we always call > > save_live_complete > > > > > > To my mind we have to call save_live_complete for any device > > that we've called save_live_setup on (maybe it allocated something > > in _setup that it clears up in _complete). > > > > save_live_pending could perfectly well return 0 remaining at the end of > > the migrate for our device, and thus if we used that then we wouldn't > > call save_live_complete. > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > transition point you call save_live_complete for everything that > reports 0 post-copiable state. > > > Then again, a different approach would be to split the > save_live_complete hook into (possibly NULL) "complete precopy" and > "complete postcopy" hooks. The core would ensure that every chunk of > state has both completion hooks called (unless NULL). That might also > address my concerns about the no longer entirely accurate > save_live_complete function name. OK, that one I prefer. Are you OK with: qemu_savevm_state_complete_precopy calls -> save_live_complete_precopy qemu_savevm_state_complete_postcopy calls -> save_live_complete_postcopy ? 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 Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > in this patch - is it still necessary with the change to > > > > > > save_live_pending? > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > to decide which devices must be completed at that point. > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > save_live_pending instead? > > > > > > That would be a bit weird. > > > > > > At the moment for each device we call the: > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > 0...multiple times we call: > > > save_live_pending > > > save_live_iterate > > > > > > and then we always call > > > save_live_complete > > > > > > > > > To my mind we have to call save_live_complete for any device > > > that we've called save_live_setup on (maybe it allocated something > > > in _setup that it clears up in _complete). > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > the migrate for our device, and thus if we used that then we wouldn't > > > call save_live_complete. > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > transition point you call save_live_complete for everything that > > reports 0 post-copiable state. > > > > > > Then again, a different approach would be to split the > > save_live_complete hook into (possibly NULL) "complete precopy" and > > "complete postcopy" hooks. The core would ensure that every chunk of > > state has both completion hooks called (unless NULL). That might also > > address my concerns about the no longer entirely accurate > > save_live_complete function name. > > OK, that one I prefer. Are you OK with: > qemu_savevm_state_complete_precopy > calls -> save_live_complete_precopy > > qemu_savevm_state_complete_postcopy > calls -> save_live_complete_postcopy > > ? Sounds ok to me. Fwiw, I was thinking that both the complete_precopy and complete_postcopy hooks should always be called. For a non-postcopy migration, the postcopy hooks would just be called immediately after the precopy hooks.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > in this patch - is it still necessary with the change to > > > > > > > save_live_pending? > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > save_live_pending instead? > > > > > > > > That would be a bit weird. > > > > > > > > At the moment for each device we call the: > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > 0...multiple times we call: > > > > save_live_pending > > > > save_live_iterate > > > > > > > > and then we always call > > > > save_live_complete > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > that we've called save_live_setup on (maybe it allocated something > > > > in _setup that it clears up in _complete). > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > call save_live_complete. > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > transition point you call save_live_complete for everything that > > > reports 0 post-copiable state. > > > > > > > > > Then again, a different approach would be to split the > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > state has both completion hooks called (unless NULL). That might also > > > address my concerns about the no longer entirely accurate > > > save_live_complete function name. > > > > OK, that one I prefer. Are you OK with: > > qemu_savevm_state_complete_precopy > > calls -> save_live_complete_precopy > > > > qemu_savevm_state_complete_postcopy > > calls -> save_live_complete_postcopy > > > > ? > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > and complete_postcopy hooks should always be called. For a > non-postcopy migration, the postcopy hooks would just be called > immediately after the precopy hooks. OK, I've made the change as described in my last mail; but I haven't called the complete_postcopy hook in the precopy case. If it was as simple as making all devices use one or the other then it would work, however there are existing (precopy) assumptions about ordering of device state on the wire that I want to be careful not to alter; for example RAM must come first is the one I know. 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
* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > > in this patch - is it still necessary with the change to > > > > > > > > save_live_pending? > > > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > save_live_pending instead? > > > > > > > > > > That would be a bit weird. > > > > > > > > > > At the moment for each device we call the: > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > > > 0...multiple times we call: > > > > > save_live_pending > > > > > save_live_iterate > > > > > > > > > > and then we always call > > > > > save_live_complete > > > > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > that we've called save_live_setup on (maybe it allocated something > > > > > in _setup that it clears up in _complete). > > > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > > call save_live_complete. > > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > > transition point you call save_live_complete for everything that > > > > reports 0 post-copiable state. > > > > > > > > > > > > Then again, a different approach would be to split the > > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > > state has both completion hooks called (unless NULL). That might also > > > > address my concerns about the no longer entirely accurate > > > > save_live_complete function name. > > > > > > OK, that one I prefer. Are you OK with: > > > qemu_savevm_state_complete_precopy > > > calls -> save_live_complete_precopy > > > > > > qemu_savevm_state_complete_postcopy > > > calls -> save_live_complete_postcopy > > > > > > ? > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > > and complete_postcopy hooks should always be called. For a > > non-postcopy migration, the postcopy hooks would just be called > > immediately after the precopy hooks. > > OK, I've made the change as described in my last mail; but I haven't called > the complete_postcopy hook in the precopy case. If it was as simple as making > all devices use one or the other then it would work, however there are > existing (precopy) assumptions about ordering of device state on the wire that > I want to be careful not to alter; for example RAM must come first is the one > I know. Actually, I spoke too soon; testing this found a bad breakage. the functions in savevm.c add the per-section headers, and then call the _complete methods on the devices. Those _complete methods can't elect to do nothing, because a header has already been planted. I've ended up with something between the two; we still have a complete_precopy and complete_postcopy method on the devices; if the complete_postcopy method exists and we're in postcopy mode, the complete_precopy method isn't called at all. A device could decide to do something different in complete_postcopy from complete_precopy but it must do something to complete the section. Effectively the presence of the complete_postcopy is now doing what can_postcopy() used to do. Dave > > 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 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Mar 25, 2015 at 04:40:11PM +0000, Dr. David Alan Gilbert wrote: > * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > > > in this patch - is it still necessary with the change to > > > > > > > > > save_live_pending? > > > > > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > > save_live_pending instead? > > > > > > > > > > > > That would be a bit weird. > > > > > > > > > > > > At the moment for each device we call the: > > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > > > > > 0...multiple times we call: > > > > > > save_live_pending > > > > > > save_live_iterate > > > > > > > > > > > > and then we always call > > > > > > save_live_complete > > > > > > > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > > that we've called save_live_setup on (maybe it allocated something > > > > > > in _setup that it clears up in _complete). > > > > > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > > > call save_live_complete. > > > > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > > > transition point you call save_live_complete for everything that > > > > > reports 0 post-copiable state. > > > > > > > > > > > > > > > Then again, a different approach would be to split the > > > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > > > state has both completion hooks called (unless NULL). That might also > > > > > address my concerns about the no longer entirely accurate > > > > > save_live_complete function name. > > > > > > > > OK, that one I prefer. Are you OK with: > > > > qemu_savevm_state_complete_precopy > > > > calls -> save_live_complete_precopy > > > > > > > > qemu_savevm_state_complete_postcopy > > > > calls -> save_live_complete_postcopy > > > > > > > > ? > > > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > > > and complete_postcopy hooks should always be called. For a > > > non-postcopy migration, the postcopy hooks would just be called > > > immediately after the precopy hooks. > > > > OK, I've made the change as described in my last mail; but I haven't called > > the complete_postcopy hook in the precopy case. If it was as simple as making > > all devices use one or the other then it would work, however there are > > existing (precopy) assumptions about ordering of device state on the wire that > > I want to be careful not to alter; for example RAM must come first is the one > > I know. > > Actually, I spoke too soon; testing this found a bad breakage. > > the functions in savevm.c add the per-section headers, and then call the _complete > methods on the devices. Those _complete methods can't elect to do nothing, because > a header has already been planted. Hrm.. couldn't you move the test for presence of the hook earlier so you don't sent the header if the hook is NULL? > I've ended up with something between the two; we still have a complete_precopy and > complete_postcopy method on the devices; if the complete_postcopy method exists and > we're in postcopy mode, the complete_precopy method isn't called at all. > A device could decide to do something different in complete_postcopy from complete_precopy > but it must do something to complete the section. > Effectively the presence of the complete_postcopy is now doing what > can_postcopy() used to do. Hmm.. but it means there's no per-device hook for the precopy to postcopy transition point. I'm not sure if that might matter.
On Wed, Mar 25, 2015 at 03:00:29PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > > in this patch - is it still necessary with the change to > > > > > > > > save_live_pending? > > > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > save_live_pending instead? > > > > > > > > > > That would be a bit weird. > > > > > > > > > > At the moment for each device we call the: > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > > > 0...multiple times we call: > > > > > save_live_pending > > > > > save_live_iterate > > > > > > > > > > and then we always call > > > > > save_live_complete > > > > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > that we've called save_live_setup on (maybe it allocated something > > > > > in _setup that it clears up in _complete). > > > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > > call save_live_complete. > > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > > transition point you call save_live_complete for everything that > > > > reports 0 post-copiable state. > > > > > > > > > > > > Then again, a different approach would be to split the > > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > > state has both completion hooks called (unless NULL). That might also > > > > address my concerns about the no longer entirely accurate > > > > save_live_complete function name. > > > > > > OK, that one I prefer. Are you OK with: > > > qemu_savevm_state_complete_precopy > > > calls -> save_live_complete_precopy > > > > > > qemu_savevm_state_complete_postcopy > > > calls -> save_live_complete_postcopy > > > > > > ? > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > > and complete_postcopy hooks should always be called. For a > > non-postcopy migration, the postcopy hooks would just be called > > immediately after the precopy hooks. > > OK, I've made the change as described in my last mail; but I haven't called > the complete_postcopy hook in the precopy case. If it was as simple as making > all devices use one or the other then it would work, however there are > existing (precopy) assumptions about ordering of device state on the wire that > I want to be careful not to alter; for example RAM must come first is the one > I know. It's not obvious to me why that matters to the hook scheme.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Wed, Mar 25, 2015 at 04:40:11PM +0000, Dr. David Alan Gilbert wrote: > > * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > > > > in this patch - is it still necessary with the change to > > > > > > > > > > save_live_pending? > > > > > > > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > > > save_live_pending instead? > > > > > > > > > > > > > > That would be a bit weird. > > > > > > > > > > > > > > At the moment for each device we call the: > > > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > > > > > > > 0...multiple times we call: > > > > > > > save_live_pending > > > > > > > save_live_iterate > > > > > > > > > > > > > > and then we always call > > > > > > > save_live_complete > > > > > > > > > > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > > > that we've called save_live_setup on (maybe it allocated something > > > > > > > in _setup that it clears up in _complete). > > > > > > > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > > > > call save_live_complete. > > > > > > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > > > > transition point you call save_live_complete for everything that > > > > > > reports 0 post-copiable state. > > > > > > > > > > > > > > > > > > Then again, a different approach would be to split the > > > > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > > > > state has both completion hooks called (unless NULL). That might also > > > > > > address my concerns about the no longer entirely accurate > > > > > > save_live_complete function name. > > > > > > > > > > OK, that one I prefer. Are you OK with: > > > > > qemu_savevm_state_complete_precopy > > > > > calls -> save_live_complete_precopy > > > > > > > > > > qemu_savevm_state_complete_postcopy > > > > > calls -> save_live_complete_postcopy > > > > > > > > > > ? > > > > > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > > > > and complete_postcopy hooks should always be called. For a > > > > non-postcopy migration, the postcopy hooks would just be called > > > > immediately after the precopy hooks. > > > > > > OK, I've made the change as described in my last mail; but I haven't called > > > the complete_postcopy hook in the precopy case. If it was as simple as making > > > all devices use one or the other then it would work, however there are > > > existing (precopy) assumptions about ordering of device state on the wire that > > > I want to be careful not to alter; for example RAM must come first is the one > > > I know. > > > > Actually, I spoke too soon; testing this found a bad breakage. > > > > the functions in savevm.c add the per-section headers, and then call the _complete > > methods on the devices. Those _complete methods can't elect to do nothing, because > > a header has already been planted. > > Hrm.. couldn't you move the test for presence of the hook earlier so > you don't sent the header if the hook is NULL? There's two tests that you have to make: a) in qemu_savevm_state_complete_precopy do you call save_live_complete_precopy b) in qemu_savevm_state_complete_postcopy do you call save_live_complete_postcopy The obvious case is if either hook is NULL you don't call it. (a) is the harder cases, if we're doing postcopy then we don't want to call the save_live_complete_precopy method on a device which isn't expecting to complete until postcopy. The code in qemu_savevm_state_complete_precopy checks for the presence of the *postcopy* hook, and doesn't emit the header or call the precopy commit if the postcopy hook is present and we're in postcopy. > > I've ended up with something between the two; we still have a complete_precopy and > > complete_postcopy method on the devices; if the complete_postcopy method exists and > > we're in postcopy mode, the complete_precopy method isn't called at all. > > A device could decide to do something different in complete_postcopy from complete_precopy > > but it must do something to complete the section. > > Effectively the presence of the complete_postcopy is now doing what > > can_postcopy() used to do. > > Hmm.. but it means there's no per-device hook for the precopy to > postcopy transition point. I'm not sure if that might matter. This is true, but if we needed a generic hook for that (which might be useful) it probably shouldn't be 'complete'. 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 Thu, Mar 26, 2015 at 11:44:36AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Wed, Mar 25, 2015 at 04:40:11PM +0000, Dr. David Alan Gilbert wrote: > > > * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert wrote: > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilbert wrote: > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan Gilbert wrote: > > > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > > > > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > > > > > > > > > > > > > > > > > Modify save_live_pending to return separate postcopiable and > > > > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > > > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it can postcopy > > > > > > > > > > > > > > > > > > > > > > What's the purpose of the can_postcopy callback? There are no callers > > > > > > > > > > > in this patch - is it still necessary with the change to > > > > > > > > > > > save_live_pending? > > > > > > > > > > > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes' uses > > > > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_savevm_state_complete > > > > > > > > > > to decide which devices must be completed at that point. > > > > > > > > > > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > > > > save_live_pending instead? > > > > > > > > > > > > > > > > That would be a bit weird. > > > > > > > > > > > > > > > > At the moment for each device we call the: > > > > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > > > > > > > > > > 0...multiple times we call: > > > > > > > > save_live_pending > > > > > > > > save_live_iterate > > > > > > > > > > > > > > > > and then we always call > > > > > > > > save_live_complete > > > > > > > > > > > > > > > > > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > > > > that we've called save_live_setup on (maybe it allocated something > > > > > > > > in _setup that it clears up in _complete). > > > > > > > > > > > > > > > > save_live_pending could perfectly well return 0 remaining at the end of > > > > > > > > the migrate for our device, and thus if we used that then we wouldn't > > > > > > > > call save_live_complete. > > > > > > > > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->postcopy > > > > > > > transition point you call save_live_complete for everything that > > > > > > > reports 0 post-copiable state. > > > > > > > > > > > > > > > > > > > > > Then again, a different approach would be to split the > > > > > > > save_live_complete hook into (possibly NULL) "complete precopy" and > > > > > > > "complete postcopy" hooks. The core would ensure that every chunk of > > > > > > > state has both completion hooks called (unless NULL). That might also > > > > > > > address my concerns about the no longer entirely accurate > > > > > > > save_live_complete function name. > > > > > > > > > > > > OK, that one I prefer. Are you OK with: > > > > > > qemu_savevm_state_complete_precopy > > > > > > calls -> save_live_complete_precopy > > > > > > > > > > > > qemu_savevm_state_complete_postcopy > > > > > > calls -> save_live_complete_postcopy > > > > > > > > > > > > ? > > > > > > > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_precopy > > > > > and complete_postcopy hooks should always be called. For a > > > > > non-postcopy migration, the postcopy hooks would just be called > > > > > immediately after the precopy hooks. > > > > > > > > OK, I've made the change as described in my last mail; but I haven't called > > > > the complete_postcopy hook in the precopy case. If it was as simple as making > > > > all devices use one or the other then it would work, however there are > > > > existing (precopy) assumptions about ordering of device state on the wire that > > > > I want to be careful not to alter; for example RAM must come first is the one > > > > I know. > > > > > > Actually, I spoke too soon; testing this found a bad breakage. > > > > > > the functions in savevm.c add the per-section headers, and then call the _complete > > > methods on the devices. Those _complete methods can't elect to do nothing, because > > > a header has already been planted. > > > > Hrm.. couldn't you move the test for presence of the hook earlier so > > you don't sent the header if the hook is NULL? > > There's two tests that you have to make: > a) in qemu_savevm_state_complete_precopy do you call save_live_complete_precopy > b) in qemu_savevm_state_complete_postcopy do you call save_live_complete_postcopy > > The obvious case is if either hook is NULL you don't call it. > (a) is the harder cases, if we're doing postcopy then we don't want to call the > save_live_complete_precopy method on a device which isn't expecting to complete until > postcopy. Uh.. no, I'm expecting the complete_precopy hook to be called every time if it's non-NULL. If it's a postcopy item that doesn't expect to finish at the end of precopy, it should put its code in the complete_postcopy hook instead. That should be fine for the non-postcopy case too, because the postcopy_complete hook will be called momentarily. > The code in qemu_savevm_state_complete_precopy checks for the presence of the *postcopy* > hook, and doesn't emit the header or call the precopy commit if the postcopy hook > is present and we're in postcopy. > > > > I've ended up with something between the two; we still have a complete_precopy and > > > complete_postcopy method on the devices; if the complete_postcopy method exists and > > > we're in postcopy mode, the complete_precopy method isn't called at all. > > > A device could decide to do something different in complete_postcopy from complete_precopy > > > but it must do something to complete the section. > > > Effectively the presence of the complete_postcopy is now doing what > > > can_postcopy() used to do. > > > > Hmm.. but it means there's no per-device hook for the precopy to > > postcopy transition point. I'm not sure if that might matter. > > This is true, but if we needed a generic hook for that (which might be useful) > it probably shouldn't be 'complete'. Well, I'm thinking of these as "state (complete precopy)" rather than "(state complete) precopy". But yeah, a different name might be less ambiguous.
On 24/03/2015 23:32, David Gibson wrote: >>> OK, that one I prefer. Are you OK with: >>> qemu_savevm_state_complete_precopy calls -> >>> save_live_complete_precopy >>> >>> qemu_savevm_state_complete_postcopy calls -> >>> save_live_complete_postcopy >>> >>> ? > Sounds ok to me. Fwiw, I was thinking that both the > complete_precopy and complete_postcopy hooks should always be > called. For a non-postcopy migration, the postcopy hooks would > just be called immediately after the precopy hooks. What about then calling them save_live_after_precopy and save_live_complete, or having save_live_before_postcopy and save_live_complete? Paolo
On Mon, Mar 30, 2015 at 10:10:05AM +0200, Paolo Bonzini wrote: > > > On 24/03/2015 23:32, David Gibson wrote: > >>> OK, that one I prefer. Are you OK with: > >>> qemu_savevm_state_complete_precopy calls -> > >>> save_live_complete_precopy > >>> > >>> qemu_savevm_state_complete_postcopy calls -> > >>> save_live_complete_postcopy > >>> > >>> ? > > Sounds ok to me. Fwiw, I was thinking that both the > > complete_precopy and complete_postcopy hooks should always be > > called. For a non-postcopy migration, the postcopy hooks would > > just be called immediately after the precopy hooks. > > What about then calling them save_live_after_precopy and > save_live_complete, or having save_live_before_postcopy and > save_live_complete? Good idea.
diff --git a/arch_init.c b/arch_init.c index fe0df0d..7bc5fa6 100644 --- a/arch_init.c +++ b/arch_init.c @@ -997,7 +997,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque) return 0; } -static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) +static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, + uint64_t *non_postcopiable_pending, + uint64_t *postcopiable_pending) { uint64_t remaining_size; @@ -1009,7 +1011,9 @@ static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) qemu_mutex_unlock_iothread(); remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE; } - return remaining_size; + + *non_postcopiable_pending = 0; + *postcopiable_pending = remaining_size; } static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) @@ -1204,6 +1208,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, @@ -1211,6 +1221,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 18da207..c9ec74a 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -54,8 +54,14 @@ 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); - + /* + * postcopiable_pending must return 0 unless the can_postcopy + * handler returns true. + */ + void (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size, + uint64_t *non_postcopiable_pending, + uint64_t *postcopiable_pending); + bool (*can_postcopy)(void *opaque); LoadStateHandler *load_state; } SaveVMHandlers; diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index e83bf80..5f518b3 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -111,7 +111,9 @@ void qemu_savevm_state_header(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_ping(QEMUFile *f, uint32_t value); diff --git a/migration/block.c b/migration/block.c index 0c76106..0f6f209 100644 --- a/migration/block.c +++ b/migration/block.c @@ -754,7 +754,9 @@ static int block_save_complete(QEMUFile *f, void *opaque) return 0; } -static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) +static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, + uint64_t *non_postcopiable_pending, + uint64_t *postcopiable_pending) { /* Estimate pending number of bytes to send */ uint64_t pending; @@ -773,7 +775,8 @@ static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size) qemu_mutex_unlock_iothread(); DPRINTF("Enter save live pending %" PRIu64 "\n", pending); - return pending; + *non_postcopiable_pending = pending; + *postcopiable_pending = 0; } static int block_load(QEMUFile *f, void *opaque, int version_id) diff --git a/migration/migration.c b/migration/migration.c index 2e6adca..a4fc7d7 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -868,8 +868,13 @@ 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); - trace_migrate_pending(pending_size, max_size); + uint64_t pend_post, pend_nonpost; + + 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, + 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 df48ba8..e301a0a 100644 --- a/savevm.c +++ b/savevm.c @@ -944,10 +944,20 @@ 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 tmp_non_postcopiable, tmp_postcopiable; + + *res_non_postcopiable = 0; + *res_postcopiable = 0; + QTAILQ_FOREACH(se, &savevm_handlers, entry) { if (!se->ops || !se->ops->save_live_pending) { @@ -958,9 +968,12 @@ uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size) continue; } } - ret += se->ops->save_live_pending(f, se->opaque, max_size); + se->ops->save_live_pending(f, se->opaque, max_size, + &tmp_non_postcopiable, &tmp_postcopiable); + + *res_postcopiable += tmp_postcopiable; + *res_non_postcopiable += tmp_non_postcopiable; } - return ret; } void qemu_savevm_state_cancel(void) diff --git a/trace-events b/trace-events index cbf995c..83312b6 100644 --- a/trace-events +++ b/trace-events @@ -1400,7 +1400,7 @@ migrate_fd_cleanup(void) "" migrate_fd_cleanup_src_rp(void) "" migrate_fd_error(void) "" migrate_fd_cancel(void) "" -migrate_pending(uint64_t size, uint64_t max) "pending size %" PRIu64 " max %" PRIu64 +migrate_pending(uint64_t size, uint64_t max, uint64_t post, uint64_t nonpost) "pending size %" PRIu64 " max %" PRIu64 " (post=%" PRIu64 " nonpost=%" PRIu64 ")" migrate_send_rp_message(int cmd, uint16_t len) "cmd=%d, len=%d" open_outgoing_return_path(void) "" open_outgoing_return_path_continue(void) ""