| Message ID | 20230521151808.24804-3-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Add switchover ack capability and VFIO precopy support | expand |
On Sun, May 21, 2023 at 06:18:03PM +0300, Avihai Horon wrote: > Implement switchover ack logic. This prevents the source from stopping > the VM and completing the migration until an ACK is received from the > destination that it's OK to do so. > > To achieve this, a new SaveVMHandlers handler switchover_ack_needed() > and a new return path message MIG_RP_MSG_SWITCHOVER_ACK are added. > > The switchover_ack_needed() handler is called during migration setup > both in the source and the destination to check if switchover ack is > used by the migrated device. > > When switchover is approved by all migrated devices in the destination > that support this capability, the MIG_RP_MSG_INITIAL_DATA_LOADED_ACK s/MIG_RP_MSG_INITIAL_DATA_LOADED_ACK/MIG_RP_MSG_SWITCHOVER_ACK/ > return path message is sent to the source to notify it that it's OK to > do switchover. > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > --- > include/migration/register.h | 3 ++ > migration/migration.h | 16 +++++++++++ > migration/savevm.h | 2 ++ > migration/migration.c | 42 +++++++++++++++++++++++++-- > migration/savevm.c | 56 ++++++++++++++++++++++++++++++++++++ > migration/trace-events | 4 +++ > 6 files changed, 121 insertions(+), 2 deletions(-) > > diff --git a/include/migration/register.h b/include/migration/register.h > index a8dfd8fefd..cda36d377b 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -71,6 +71,9 @@ typedef struct SaveVMHandlers { > int (*load_cleanup)(void *opaque); > /* Called when postcopy migration wants to resume from failure */ > int (*resume_prepare)(MigrationState *s, void *opaque); > + > + /* Checks if switchover ack should be used. Called both in src and dest */ > + bool (*switchover_ack_needed)(void *opaque); Sorry if I'm going to suggest something that overwrites what I suggested.. :( Luckily, not so much. When I read the new series I just noticed maybe it's still better to only use this on dst, and always do the ACK. The problem is based on your patch 1 description, the RP ACK message will be sent if the switchover-ack cap is set, but actually it's conditionally in the current impl just to handle num==0 case, so either the impl needs change or the desc needs change. It turns out it'll be even cleaner to always send it. If so.. > } SaveVMHandlers; > > int register_savevm_live(const char *idstr, > diff --git a/migration/migration.h b/migration/migration.h > index 48a46123a0..e209860cce 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -209,6 +209,13 @@ struct MigrationIncomingState { > * contains valid information. > */ > QemuMutex page_request_mutex; > + > + /* > + * Number of devices that have yet to approve switchover. When this reaches > + * zero an ACK that it's OK to do switchover is sent to the source. No lock > + * is needed as this field is updated serially. > + */ > + unsigned int switchover_ack_pending_num; > }; > > MigrationIncomingState *migration_incoming_get_current(void); > @@ -437,6 +444,14 @@ struct MigrationState { > > /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */ > JSONWriter *vmdesc; > + > + /* Number of devices that use switchover ack capability */ > + unsigned int switchover_ack_user_num; ... we save this field, then... > + /* > + * Indicates whether an ACK from the destination that it's OK to do > + * switchover has been received. > + */ > + bool switchover_acked; > }; > > void migrate_set_state(int *state, int old_state, int new_state); > @@ -477,6 +492,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis, > void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis, > char *block_name); > void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value); > +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis); > > void dirty_bitmap_mig_before_vm_start(void); > void dirty_bitmap_mig_cancel_outgoing(void); > diff --git a/migration/savevm.h b/migration/savevm.h > index fb636735f0..5c3e1a026b 100644 > --- a/migration/savevm.h > +++ b/migration/savevm.h > @@ -32,6 +32,7 @@ > bool qemu_savevm_state_blocked(Error **errp); > void qemu_savevm_non_migratable_list(strList **reasons); > void qemu_savevm_state_setup(QEMUFile *f); > +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms); > bool qemu_savevm_state_guest_unplug_pending(void); > int qemu_savevm_state_resume_prepare(MigrationState *s); > void qemu_savevm_state_header(QEMUFile *f); > @@ -65,6 +66,7 @@ int qemu_loadvm_state(QEMUFile *f); > void qemu_loadvm_state_cleanup(void); > int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis); > int qemu_load_device_state(QEMUFile *f); > +int qemu_loadvm_approve_switchover(void); > int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, > bool in_postcopy, bool inactivate_disks); > > diff --git a/migration/migration.c b/migration/migration.c > index 5de7f734b9..87423ec30c 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -78,6 +78,7 @@ enum mig_rp_message_type { > MIG_RP_MSG_REQ_PAGES, /* data (start: be64, len: be32) */ > MIG_RP_MSG_RECV_BITMAP, /* send recved_bitmap back to source */ > MIG_RP_MSG_RESUME_ACK, /* tell source that we are ready to resume */ > + MIG_RP_MSG_SWITCHOVER_ACK, /* Tell source it's OK to do switchover */ > > MIG_RP_MSG_MAX > }; > @@ -760,6 +761,11 @@ bool migration_has_all_channels(void) > return true; > } > > +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis) > +{ > + return migrate_send_rp_message(mis, MIG_RP_MSG_SWITCHOVER_ACK, 0, NULL); > +} > + > /* > * Send a 'SHUT' message on the return channel with the given value > * to indicate that we've finished with the RP. Non-0 value indicates > @@ -1405,6 +1411,8 @@ void migrate_init(MigrationState *s) > s->vm_was_running = false; > s->iteration_initial_bytes = 0; > s->threshold_size = 0; > + s->switchover_ack_user_num = 0; > + s->switchover_acked = false; > } > > int migrate_add_blocker_internal(Error *reason, Error **errp) > @@ -1721,6 +1729,7 @@ static struct rp_cmd_args { > [MIG_RP_MSG_REQ_PAGES_ID] = { .len = -1, .name = "REQ_PAGES_ID" }, > [MIG_RP_MSG_RECV_BITMAP] = { .len = -1, .name = "RECV_BITMAP" }, > [MIG_RP_MSG_RESUME_ACK] = { .len = 4, .name = "RESUME_ACK" }, > + [MIG_RP_MSG_SWITCHOVER_ACK] = { .len = 0, .name = "SWITCHOVER_ACK" }, > [MIG_RP_MSG_MAX] = { .len = -1, .name = "MAX" }, > }; > > @@ -1959,6 +1968,11 @@ retry: > } > break; > > + case MIG_RP_MSG_SWITCHOVER_ACK: > + ms->switchover_acked = true; > + trace_source_return_path_thread_switchover_acked(); > + break; > + > default: > break; > } > @@ -2700,6 +2714,25 @@ static void migration_update_counters(MigrationState *s, > bandwidth, s->threshold_size); > } > > +static bool migration_can_switchover(MigrationState *s) > +{ > + if (!migrate_switchover_ack()) { > + return true; > + } > + > + /* Switchover ack was enabled but no device uses it */ > + if (!s->switchover_ack_user_num) { > + return true; > + } ... we drop this, then... > + > + /* No reason to wait for switchover ACK if VM is stopped */ > + if (!runstate_is_running()) { > + return true; > + } > + > + return s->switchover_acked; > +} > + > /* Migration thread iteration status */ > typedef enum { > MIG_ITERATE_RESUME, /* Resume current iteration */ > @@ -2715,6 +2748,7 @@ static MigIterateState migration_iteration_run(MigrationState *s) > { > uint64_t must_precopy, can_postcopy; > bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; > + bool can_switchover = migration_can_switchover(s); > > qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); > uint64_t pending_size = must_precopy + can_postcopy; > @@ -2727,14 +2761,14 @@ static MigIterateState migration_iteration_run(MigrationState *s) > trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy); > } > > - if (!pending_size || pending_size < s->threshold_size) { > + if ((!pending_size || pending_size < s->threshold_size) && can_switchover) { > trace_migration_thread_low_pending(pending_size); > migration_completion(s); > return MIG_ITERATE_BREAK; > } > > /* Still a significant amount to transfer */ > - if (!in_postcopy && must_precopy <= s->threshold_size && > + if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && > qatomic_read(&s->start_postcopy)) { > if (postcopy_start(s)) { > error_report("%s: postcopy failed to start", __func__); > @@ -2959,6 +2993,10 @@ static void *migration_thread(void *opaque) > > qemu_savevm_state_setup(s->to_dst_file); > > + if (migrate_switchover_ack()) { > + qemu_savevm_state_switchover_ack_needed(s); > + } ... we also drop this, then... > + > qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, > MIGRATION_STATUS_ACTIVE); > > diff --git a/migration/savevm.c b/migration/savevm.c > index 03795ce8dc..9482b1ff27 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1233,6 +1233,23 @@ bool qemu_savevm_state_guest_unplug_pending(void) > return false; > } > > +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms) > +{ > + SaveStateEntry *se; > + > + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > + if (!se->ops || !se->ops->switchover_ack_needed) { > + continue; > + } > + > + if (se->ops->switchover_ack_needed(se->opaque)) { > + ms->switchover_ack_user_num++; > + } > + } > + > + trace_savevm_state_switchover_ack_needed(ms->switchover_ack_user_num); > +} ... we also drop this, then... > + > void qemu_savevm_state_setup(QEMUFile *f) > { > MigrationState *ms = migrate_get_current(); > @@ -2586,6 +2603,23 @@ static int qemu_loadvm_state_header(QEMUFile *f) > return 0; > } > > +static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) > +{ > + SaveStateEntry *se; > + > + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > + if (!se->ops || !se->ops->switchover_ack_needed) { > + continue; > + } > + > + if (se->ops->switchover_ack_needed(se->opaque)) { > + mis->switchover_ack_pending_num++; > + } > + } > + > + trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); > +} > + > static int qemu_loadvm_state_setup(QEMUFile *f) > { > SaveStateEntry *se; > @@ -2789,6 +2823,10 @@ int qemu_loadvm_state(QEMUFile *f) > return -EINVAL; > } > > + if (migrate_switchover_ack()) { > + qemu_loadvm_state_switchover_ack_needed(mis); ... here, we send ACK msg if num==0. So we (1) make the wire protocol clearer (ACK must be there if cap set), then (2) drop a bunch of code too. Actually we also make the code clearer too on src. What do you think? Other than that this looks very good to me. > + } > + > cpu_synchronize_all_pre_loadvm(); > > ret = qemu_loadvm_state_main(f, mis); > @@ -2862,6 +2900,24 @@ int qemu_load_device_state(QEMUFile *f) > return 0; > } > > +int qemu_loadvm_approve_switchover(void) > +{ > + MigrationIncomingState *mis = migration_incoming_get_current(); > + > + if (!mis->switchover_ack_pending_num) { > + return -EINVAL; > + } > + > + mis->switchover_ack_pending_num--; > + trace_loadvm_approve_switchover(mis->switchover_ack_pending_num); > + > + if (mis->switchover_ack_pending_num) { > + return 0; > + } > + > + return migrate_send_rp_switchover_ack(mis); > +} > + > bool save_snapshot(const char *name, bool overwrite, const char *vmstate, > bool has_devices, strList *devices, Error **errp) > { > diff --git a/migration/trace-events b/migration/trace-events > index cdaef7a1ea..c52b429b28 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -7,6 +7,7 @@ qemu_loadvm_state_section_partend(uint32_t section_id) "%u" > qemu_loadvm_state_post_main(int ret) "%d" > qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u" > qemu_savevm_send_packaged(void) "" > +loadvm_state_switchover_ack_needed(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" > loadvm_state_setup(void) "" > loadvm_state_cleanup(void) "" > loadvm_handle_cmd_packaged(unsigned int length) "%u" > @@ -23,6 +24,7 @@ loadvm_postcopy_ram_handle_discard_end(void) "" > loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud" > loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d" > loadvm_process_command_ping(uint32_t val) "0x%x" > +loadvm_approve_switchover(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" > postcopy_ram_listen_thread_exit(void) "" > postcopy_ram_listen_thread_start(void) "" > qemu_savevm_send_postcopy_advise(void) "" > @@ -39,6 +41,7 @@ savevm_send_postcopy_resume(void) "" > savevm_send_colo_enable(void) "" > savevm_send_recv_bitmap(char *name) "%s" > savevm_state_setup(void) "" > +savevm_state_switchover_ack_needed(unsigned int switchover_ack_user_num) "Switchover ack user num=%u" > savevm_state_resume_prepare(void) "" > savevm_state_header(void) "" > savevm_state_iterate(void) "" > @@ -180,6 +183,7 @@ source_return_path_thread_loop_top(void) "" > source_return_path_thread_pong(uint32_t val) "0x%x" > source_return_path_thread_shut(uint32_t val) "0x%x" > source_return_path_thread_resume_ack(uint32_t v) "%"PRIu32 > +source_return_path_thread_switchover_acked(void) "" > migration_thread_low_pending(uint64_t pending) "%" PRIu64 > migrate_transferred(uint64_t tranferred, uint64_t time_spent, uint64_t bandwidth, uint64_t size) "transferred %" PRIu64 " time_spent %" PRIu64 " bandwidth %" PRIu64 " max_size %" PRId64 > process_incoming_migration_co_end(int ret, int ps) "ret=%d postcopy-state=%d" > -- > 2.26.3 >
On 24/05/2023 22:32, Peter Xu wrote: > External email: Use caution opening links or attachments > > > On Sun, May 21, 2023 at 06:18:03PM +0300, Avihai Horon wrote: >> Implement switchover ack logic. This prevents the source from stopping >> the VM and completing the migration until an ACK is received from the >> destination that it's OK to do so. >> >> To achieve this, a new SaveVMHandlers handler switchover_ack_needed() >> and a new return path message MIG_RP_MSG_SWITCHOVER_ACK are added. >> >> The switchover_ack_needed() handler is called during migration setup >> both in the source and the destination to check if switchover ack is >> used by the migrated device. >> >> When switchover is approved by all migrated devices in the destination >> that support this capability, the MIG_RP_MSG_INITIAL_DATA_LOADED_ACK > s/MIG_RP_MSG_INITIAL_DATA_LOADED_ACK/MIG_RP_MSG_SWITCHOVER_ACK/ Will change. > >> return path message is sent to the source to notify it that it's OK to >> do switchover. >> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> --- >> include/migration/register.h | 3 ++ >> migration/migration.h | 16 +++++++++++ >> migration/savevm.h | 2 ++ >> migration/migration.c | 42 +++++++++++++++++++++++++-- >> migration/savevm.c | 56 ++++++++++++++++++++++++++++++++++++ >> migration/trace-events | 4 +++ >> 6 files changed, 121 insertions(+), 2 deletions(-) >> >> diff --git a/include/migration/register.h b/include/migration/register.h >> index a8dfd8fefd..cda36d377b 100644 >> --- a/include/migration/register.h >> +++ b/include/migration/register.h >> @@ -71,6 +71,9 @@ typedef struct SaveVMHandlers { >> int (*load_cleanup)(void *opaque); >> /* Called when postcopy migration wants to resume from failure */ >> int (*resume_prepare)(MigrationState *s, void *opaque); >> + >> + /* Checks if switchover ack should be used. Called both in src and dest */ >> + bool (*switchover_ack_needed)(void *opaque); > Sorry if I'm going to suggest something that overwrites what I > suggested.. :( Luckily, not so much. > > When I read the new series I just noticed maybe it's still better to only > use this on dst, and always do the ACK. > > The problem is based on your patch 1 description, the RP ACK message will > be sent if the switchover-ack cap is set, but actually it's conditionally > in the current impl just to handle num==0 case, so either the impl needs > change or the desc needs change. > > It turns out it'll be even cleaner to always send it. If so.. > >> } SaveVMHandlers; >> >> int register_savevm_live(const char *idstr, >> diff --git a/migration/migration.h b/migration/migration.h >> index 48a46123a0..e209860cce 100644 >> --- a/migration/migration.h >> +++ b/migration/migration.h >> @@ -209,6 +209,13 @@ struct MigrationIncomingState { >> * contains valid information. >> */ >> QemuMutex page_request_mutex; >> + >> + /* >> + * Number of devices that have yet to approve switchover. When this reaches >> + * zero an ACK that it's OK to do switchover is sent to the source. No lock >> + * is needed as this field is updated serially. >> + */ >> + unsigned int switchover_ack_pending_num; >> }; >> >> MigrationIncomingState *migration_incoming_get_current(void); >> @@ -437,6 +444,14 @@ struct MigrationState { >> >> /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */ >> JSONWriter *vmdesc; >> + >> + /* Number of devices that use switchover ack capability */ >> + unsigned int switchover_ack_user_num; > ... we save this field, then... > >> + /* >> + * Indicates whether an ACK from the destination that it's OK to do >> + * switchover has been received. >> + */ >> + bool switchover_acked; >> }; >> >> void migrate_set_state(int *state, int old_state, int new_state); >> @@ -477,6 +492,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis, >> void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis, >> char *block_name); >> void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value); >> +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis); >> >> void dirty_bitmap_mig_before_vm_start(void); >> void dirty_bitmap_mig_cancel_outgoing(void); >> diff --git a/migration/savevm.h b/migration/savevm.h >> index fb636735f0..5c3e1a026b 100644 >> --- a/migration/savevm.h >> +++ b/migration/savevm.h >> @@ -32,6 +32,7 @@ >> bool qemu_savevm_state_blocked(Error **errp); >> void qemu_savevm_non_migratable_list(strList **reasons); >> void qemu_savevm_state_setup(QEMUFile *f); >> +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms); >> bool qemu_savevm_state_guest_unplug_pending(void); >> int qemu_savevm_state_resume_prepare(MigrationState *s); >> void qemu_savevm_state_header(QEMUFile *f); >> @@ -65,6 +66,7 @@ int qemu_loadvm_state(QEMUFile *f); >> void qemu_loadvm_state_cleanup(void); >> int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis); >> int qemu_load_device_state(QEMUFile *f); >> +int qemu_loadvm_approve_switchover(void); >> int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, >> bool in_postcopy, bool inactivate_disks); >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 5de7f734b9..87423ec30c 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -78,6 +78,7 @@ enum mig_rp_message_type { >> MIG_RP_MSG_REQ_PAGES, /* data (start: be64, len: be32) */ >> MIG_RP_MSG_RECV_BITMAP, /* send recved_bitmap back to source */ >> MIG_RP_MSG_RESUME_ACK, /* tell source that we are ready to resume */ >> + MIG_RP_MSG_SWITCHOVER_ACK, /* Tell source it's OK to do switchover */ >> >> MIG_RP_MSG_MAX >> }; >> @@ -760,6 +761,11 @@ bool migration_has_all_channels(void) >> return true; >> } >> >> +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis) >> +{ >> + return migrate_send_rp_message(mis, MIG_RP_MSG_SWITCHOVER_ACK, 0, NULL); >> +} >> + >> /* >> * Send a 'SHUT' message on the return channel with the given value >> * to indicate that we've finished with the RP. Non-0 value indicates >> @@ -1405,6 +1411,8 @@ void migrate_init(MigrationState *s) >> s->vm_was_running = false; >> s->iteration_initial_bytes = 0; >> s->threshold_size = 0; >> + s->switchover_ack_user_num = 0; >> + s->switchover_acked = false; >> } >> >> int migrate_add_blocker_internal(Error *reason, Error **errp) >> @@ -1721,6 +1729,7 @@ static struct rp_cmd_args { >> [MIG_RP_MSG_REQ_PAGES_ID] = { .len = -1, .name = "REQ_PAGES_ID" }, >> [MIG_RP_MSG_RECV_BITMAP] = { .len = -1, .name = "RECV_BITMAP" }, >> [MIG_RP_MSG_RESUME_ACK] = { .len = 4, .name = "RESUME_ACK" }, >> + [MIG_RP_MSG_SWITCHOVER_ACK] = { .len = 0, .name = "SWITCHOVER_ACK" }, >> [MIG_RP_MSG_MAX] = { .len = -1, .name = "MAX" }, >> }; >> >> @@ -1959,6 +1968,11 @@ retry: >> } >> break; >> >> + case MIG_RP_MSG_SWITCHOVER_ACK: >> + ms->switchover_acked = true; >> + trace_source_return_path_thread_switchover_acked(); >> + break; >> + >> default: >> break; >> } >> @@ -2700,6 +2714,25 @@ static void migration_update_counters(MigrationState *s, >> bandwidth, s->threshold_size); >> } >> >> +static bool migration_can_switchover(MigrationState *s) >> +{ >> + if (!migrate_switchover_ack()) { >> + return true; >> + } >> + >> + /* Switchover ack was enabled but no device uses it */ >> + if (!s->switchover_ack_user_num) { >> + return true; >> + } > ... we drop this, then... > >> + >> + /* No reason to wait for switchover ACK if VM is stopped */ >> + if (!runstate_is_running()) { >> + return true; >> + } >> + >> + return s->switchover_acked; >> +} >> + >> /* Migration thread iteration status */ >> typedef enum { >> MIG_ITERATE_RESUME, /* Resume current iteration */ >> @@ -2715,6 +2748,7 @@ static MigIterateState migration_iteration_run(MigrationState *s) >> { >> uint64_t must_precopy, can_postcopy; >> bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; >> + bool can_switchover = migration_can_switchover(s); >> >> qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); >> uint64_t pending_size = must_precopy + can_postcopy; >> @@ -2727,14 +2761,14 @@ static MigIterateState migration_iteration_run(MigrationState *s) >> trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy); >> } >> >> - if (!pending_size || pending_size < s->threshold_size) { >> + if ((!pending_size || pending_size < s->threshold_size) && can_switchover) { >> trace_migration_thread_low_pending(pending_size); >> migration_completion(s); >> return MIG_ITERATE_BREAK; >> } >> >> /* Still a significant amount to transfer */ >> - if (!in_postcopy && must_precopy <= s->threshold_size && >> + if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && >> qatomic_read(&s->start_postcopy)) { >> if (postcopy_start(s)) { >> error_report("%s: postcopy failed to start", __func__); >> @@ -2959,6 +2993,10 @@ static void *migration_thread(void *opaque) >> >> qemu_savevm_state_setup(s->to_dst_file); >> >> + if (migrate_switchover_ack()) { >> + qemu_savevm_state_switchover_ack_needed(s); >> + } > ... we also drop this, then... > >> + >> qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, >> MIGRATION_STATUS_ACTIVE); >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 03795ce8dc..9482b1ff27 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1233,6 +1233,23 @@ bool qemu_savevm_state_guest_unplug_pending(void) >> return false; >> } >> >> +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms) >> +{ >> + SaveStateEntry *se; >> + >> + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >> + if (!se->ops || !se->ops->switchover_ack_needed) { >> + continue; >> + } >> + >> + if (se->ops->switchover_ack_needed(se->opaque)) { >> + ms->switchover_ack_user_num++; >> + } >> + } >> + >> + trace_savevm_state_switchover_ack_needed(ms->switchover_ack_user_num); >> +} > ... we also drop this, then... > >> + >> void qemu_savevm_state_setup(QEMUFile *f) >> { >> MigrationState *ms = migrate_get_current(); >> @@ -2586,6 +2603,23 @@ static int qemu_loadvm_state_header(QEMUFile *f) >> return 0; >> } >> >> +static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) >> +{ >> + SaveStateEntry *se; >> + >> + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >> + if (!se->ops || !se->ops->switchover_ack_needed) { >> + continue; >> + } >> + >> + if (se->ops->switchover_ack_needed(se->opaque)) { >> + mis->switchover_ack_pending_num++; >> + } >> + } >> + >> + trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); >> +} >> + >> static int qemu_loadvm_state_setup(QEMUFile *f) >> { >> SaveStateEntry *se; >> @@ -2789,6 +2823,10 @@ int qemu_loadvm_state(QEMUFile *f) >> return -EINVAL; >> } >> >> + if (migrate_switchover_ack()) { >> + qemu_loadvm_state_switchover_ack_needed(mis); [1] > ... here, we send ACK msg if num==0. > > So we (1) make the wire protocol clearer (ACK must be there if cap set), > then (2) drop a bunch of code too. Actually we also make the code clearer > too on src. > > What do you think? I really like this idea! It simplifies things even more. However, there is one issue -- we can't send the ACK up here [1], as at that point the return path has not been created yet. A possible solution is to check for mis->switchover_ack_pending_num == 0 when we create the return path and send the ACK there. It's not as clean as checking the number of users and ACKing here in the same place, but it works. Do you think it's OK? or do you have another idea? Thanks! > > Other than that this looks very good to me. > >> + } >> + >> cpu_synchronize_all_pre_loadvm(); >> >> ret = qemu_loadvm_state_main(f, mis); >> @@ -2862,6 +2900,24 @@ int qemu_load_device_state(QEMUFile *f) >> return 0; >> } >> >> +int qemu_loadvm_approve_switchover(void) >> +{ >> + MigrationIncomingState *mis = migration_incoming_get_current(); >> + >> + if (!mis->switchover_ack_pending_num) { >> + return -EINVAL; >> + } >> + >> + mis->switchover_ack_pending_num--; >> + trace_loadvm_approve_switchover(mis->switchover_ack_pending_num); >> + >> + if (mis->switchover_ack_pending_num) { >> + return 0; >> + } >> + >> + return migrate_send_rp_switchover_ack(mis); >> +} >> + >> bool save_snapshot(const char *name, bool overwrite, const char *vmstate, >> bool has_devices, strList *devices, Error **errp) >> { >> diff --git a/migration/trace-events b/migration/trace-events >> index cdaef7a1ea..c52b429b28 100644 >> --- a/migration/trace-events >> +++ b/migration/trace-events >> @@ -7,6 +7,7 @@ qemu_loadvm_state_section_partend(uint32_t section_id) "%u" >> qemu_loadvm_state_post_main(int ret) "%d" >> qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u" >> qemu_savevm_send_packaged(void) "" >> +loadvm_state_switchover_ack_needed(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" >> loadvm_state_setup(void) "" >> loadvm_state_cleanup(void) "" >> loadvm_handle_cmd_packaged(unsigned int length) "%u" >> @@ -23,6 +24,7 @@ loadvm_postcopy_ram_handle_discard_end(void) "" >> loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud" >> loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d" >> loadvm_process_command_ping(uint32_t val) "0x%x" >> +loadvm_approve_switchover(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" >> postcopy_ram_listen_thread_exit(void) "" >> postcopy_ram_listen_thread_start(void) "" >> qemu_savevm_send_postcopy_advise(void) "" >> @@ -39,6 +41,7 @@ savevm_send_postcopy_resume(void) "" >> savevm_send_colo_enable(void) "" >> savevm_send_recv_bitmap(char *name) "%s" >> savevm_state_setup(void) "" >> +savevm_state_switchover_ack_needed(unsigned int switchover_ack_user_num) "Switchover ack user num=%u" >> savevm_state_resume_prepare(void) "" >> savevm_state_header(void) "" >> savevm_state_iterate(void) "" >> @@ -180,6 +183,7 @@ source_return_path_thread_loop_top(void) "" >> source_return_path_thread_pong(uint32_t val) "0x%x" >> source_return_path_thread_shut(uint32_t val) "0x%x" >> source_return_path_thread_resume_ack(uint32_t v) "%"PRIu32 >> +source_return_path_thread_switchover_acked(void) "" >> migration_thread_low_pending(uint64_t pending) "%" PRIu64 >> migrate_transferred(uint64_t tranferred, uint64_t time_spent, uint64_t bandwidth, uint64_t size) "transferred %" PRIu64 " time_spent %" PRIu64 " bandwidth %" PRIu64 " max_size %" PRId64 >> process_incoming_migration_co_end(int ret, int ps) "ret=%d postcopy-state=%d" >> -- >> 2.26.3 >> > -- > Peter Xu >
On Thu, May 25, 2023 at 12:51:46PM +0300, Avihai Horon wrote: > However, there is one issue -- we can't send the ACK up here [1], as at that > point the return path has not been created yet. > A possible solution is to check for mis->switchover_ack_pending_num == 0 > when we create the return path and send the ACK there. > It's not as clean as checking the number of users and ACKing here in the > same place, but it works. > > Do you think it's OK? or do you have another idea? Good point. It looks fine to me as we create the return path mostly at the very early stage right after the migration headers. Let's just add a comment describing the ordering fact? IMHO it's about switchover_ack_pending_num won't be used before setup of the return path due to current wire protocol (we create return path before sending mostly anything else), so setup it there is fine.
On 25/05/2023 15:44, Peter Xu wrote: > External email: Use caution opening links or attachments > > > On Thu, May 25, 2023 at 12:51:46PM +0300, Avihai Horon wrote: >> However, there is one issue -- we can't send the ACK up here [1], as at that >> point the return path has not been created yet. >> A possible solution is to check for mis->switchover_ack_pending_num == 0 >> when we create the return path and send the ACK there. >> It's not as clean as checking the number of users and ACKing here in the >> same place, but it works. >> >> Do you think it's OK? or do you have another idea? > Good point. It looks fine to me as we create the return path mostly at the > very early stage right after the migration headers. > > Let's just add a comment describing the ordering fact? IMHO it's about > switchover_ack_pending_num won't be used before setup of the return path > due to current wire protocol (we create return path before sending mostly > anything else), so setup it there is fine. Sure, I will also add a comment. Thanks.
diff --git a/include/migration/register.h b/include/migration/register.h index a8dfd8fefd..cda36d377b 100644 --- a/include/migration/register.h +++ b/include/migration/register.h @@ -71,6 +71,9 @@ typedef struct SaveVMHandlers { int (*load_cleanup)(void *opaque); /* Called when postcopy migration wants to resume from failure */ int (*resume_prepare)(MigrationState *s, void *opaque); + + /* Checks if switchover ack should be used. Called both in src and dest */ + bool (*switchover_ack_needed)(void *opaque); } SaveVMHandlers; int register_savevm_live(const char *idstr, diff --git a/migration/migration.h b/migration/migration.h index 48a46123a0..e209860cce 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -209,6 +209,13 @@ struct MigrationIncomingState { * contains valid information. */ QemuMutex page_request_mutex; + + /* + * Number of devices that have yet to approve switchover. When this reaches + * zero an ACK that it's OK to do switchover is sent to the source. No lock + * is needed as this field is updated serially. + */ + unsigned int switchover_ack_pending_num; }; MigrationIncomingState *migration_incoming_get_current(void); @@ -437,6 +444,14 @@ struct MigrationState { /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */ JSONWriter *vmdesc; + + /* Number of devices that use switchover ack capability */ + unsigned int switchover_ack_user_num; + /* + * Indicates whether an ACK from the destination that it's OK to do + * switchover has been received. + */ + bool switchover_acked; }; void migrate_set_state(int *state, int old_state, int new_state); @@ -477,6 +492,7 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis, void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis, char *block_name); void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value); +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis); void dirty_bitmap_mig_before_vm_start(void); void dirty_bitmap_mig_cancel_outgoing(void); diff --git a/migration/savevm.h b/migration/savevm.h index fb636735f0..5c3e1a026b 100644 --- a/migration/savevm.h +++ b/migration/savevm.h @@ -32,6 +32,7 @@ bool qemu_savevm_state_blocked(Error **errp); void qemu_savevm_non_migratable_list(strList **reasons); void qemu_savevm_state_setup(QEMUFile *f); +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms); bool qemu_savevm_state_guest_unplug_pending(void); int qemu_savevm_state_resume_prepare(MigrationState *s); void qemu_savevm_state_header(QEMUFile *f); @@ -65,6 +66,7 @@ int qemu_loadvm_state(QEMUFile *f); void qemu_loadvm_state_cleanup(void); int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis); int qemu_load_device_state(QEMUFile *f); +int qemu_loadvm_approve_switchover(void); int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, bool in_postcopy, bool inactivate_disks); diff --git a/migration/migration.c b/migration/migration.c index 5de7f734b9..87423ec30c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -78,6 +78,7 @@ enum mig_rp_message_type { MIG_RP_MSG_REQ_PAGES, /* data (start: be64, len: be32) */ MIG_RP_MSG_RECV_BITMAP, /* send recved_bitmap back to source */ MIG_RP_MSG_RESUME_ACK, /* tell source that we are ready to resume */ + MIG_RP_MSG_SWITCHOVER_ACK, /* Tell source it's OK to do switchover */ MIG_RP_MSG_MAX }; @@ -760,6 +761,11 @@ bool migration_has_all_channels(void) return true; } +int migrate_send_rp_switchover_ack(MigrationIncomingState *mis) +{ + return migrate_send_rp_message(mis, MIG_RP_MSG_SWITCHOVER_ACK, 0, NULL); +} + /* * Send a 'SHUT' message on the return channel with the given value * to indicate that we've finished with the RP. Non-0 value indicates @@ -1405,6 +1411,8 @@ void migrate_init(MigrationState *s) s->vm_was_running = false; s->iteration_initial_bytes = 0; s->threshold_size = 0; + s->switchover_ack_user_num = 0; + s->switchover_acked = false; } int migrate_add_blocker_internal(Error *reason, Error **errp) @@ -1721,6 +1729,7 @@ static struct rp_cmd_args { [MIG_RP_MSG_REQ_PAGES_ID] = { .len = -1, .name = "REQ_PAGES_ID" }, [MIG_RP_MSG_RECV_BITMAP] = { .len = -1, .name = "RECV_BITMAP" }, [MIG_RP_MSG_RESUME_ACK] = { .len = 4, .name = "RESUME_ACK" }, + [MIG_RP_MSG_SWITCHOVER_ACK] = { .len = 0, .name = "SWITCHOVER_ACK" }, [MIG_RP_MSG_MAX] = { .len = -1, .name = "MAX" }, }; @@ -1959,6 +1968,11 @@ retry: } break; + case MIG_RP_MSG_SWITCHOVER_ACK: + ms->switchover_acked = true; + trace_source_return_path_thread_switchover_acked(); + break; + default: break; } @@ -2700,6 +2714,25 @@ static void migration_update_counters(MigrationState *s, bandwidth, s->threshold_size); } +static bool migration_can_switchover(MigrationState *s) +{ + if (!migrate_switchover_ack()) { + return true; + } + + /* Switchover ack was enabled but no device uses it */ + if (!s->switchover_ack_user_num) { + return true; + } + + /* No reason to wait for switchover ACK if VM is stopped */ + if (!runstate_is_running()) { + return true; + } + + return s->switchover_acked; +} + /* Migration thread iteration status */ typedef enum { MIG_ITERATE_RESUME, /* Resume current iteration */ @@ -2715,6 +2748,7 @@ static MigIterateState migration_iteration_run(MigrationState *s) { uint64_t must_precopy, can_postcopy; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; + bool can_switchover = migration_can_switchover(s); qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); uint64_t pending_size = must_precopy + can_postcopy; @@ -2727,14 +2761,14 @@ static MigIterateState migration_iteration_run(MigrationState *s) trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy); } - if (!pending_size || pending_size < s->threshold_size) { + if ((!pending_size || pending_size < s->threshold_size) && can_switchover) { trace_migration_thread_low_pending(pending_size); migration_completion(s); return MIG_ITERATE_BREAK; } /* Still a significant amount to transfer */ - if (!in_postcopy && must_precopy <= s->threshold_size && + if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && qatomic_read(&s->start_postcopy)) { if (postcopy_start(s)) { error_report("%s: postcopy failed to start", __func__); @@ -2959,6 +2993,10 @@ static void *migration_thread(void *opaque) qemu_savevm_state_setup(s->to_dst_file); + if (migrate_switchover_ack()) { + qemu_savevm_state_switchover_ack_needed(s); + } + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_ACTIVE); diff --git a/migration/savevm.c b/migration/savevm.c index 03795ce8dc..9482b1ff27 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1233,6 +1233,23 @@ bool qemu_savevm_state_guest_unplug_pending(void) return false; } +void qemu_savevm_state_switchover_ack_needed(MigrationState *ms) +{ + SaveStateEntry *se; + + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { + if (!se->ops || !se->ops->switchover_ack_needed) { + continue; + } + + if (se->ops->switchover_ack_needed(se->opaque)) { + ms->switchover_ack_user_num++; + } + } + + trace_savevm_state_switchover_ack_needed(ms->switchover_ack_user_num); +} + void qemu_savevm_state_setup(QEMUFile *f) { MigrationState *ms = migrate_get_current(); @@ -2586,6 +2603,23 @@ static int qemu_loadvm_state_header(QEMUFile *f) return 0; } +static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) +{ + SaveStateEntry *se; + + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { + if (!se->ops || !se->ops->switchover_ack_needed) { + continue; + } + + if (se->ops->switchover_ack_needed(se->opaque)) { + mis->switchover_ack_pending_num++; + } + } + + trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); +} + static int qemu_loadvm_state_setup(QEMUFile *f) { SaveStateEntry *se; @@ -2789,6 +2823,10 @@ int qemu_loadvm_state(QEMUFile *f) return -EINVAL; } + if (migrate_switchover_ack()) { + qemu_loadvm_state_switchover_ack_needed(mis); + } + cpu_synchronize_all_pre_loadvm(); ret = qemu_loadvm_state_main(f, mis); @@ -2862,6 +2900,24 @@ int qemu_load_device_state(QEMUFile *f) return 0; } +int qemu_loadvm_approve_switchover(void) +{ + MigrationIncomingState *mis = migration_incoming_get_current(); + + if (!mis->switchover_ack_pending_num) { + return -EINVAL; + } + + mis->switchover_ack_pending_num--; + trace_loadvm_approve_switchover(mis->switchover_ack_pending_num); + + if (mis->switchover_ack_pending_num) { + return 0; + } + + return migrate_send_rp_switchover_ack(mis); +} + bool save_snapshot(const char *name, bool overwrite, const char *vmstate, bool has_devices, strList *devices, Error **errp) { diff --git a/migration/trace-events b/migration/trace-events index cdaef7a1ea..c52b429b28 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -7,6 +7,7 @@ qemu_loadvm_state_section_partend(uint32_t section_id) "%u" qemu_loadvm_state_post_main(int ret) "%d" qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u" qemu_savevm_send_packaged(void) "" +loadvm_state_switchover_ack_needed(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" loadvm_state_setup(void) "" loadvm_state_cleanup(void) "" loadvm_handle_cmd_packaged(unsigned int length) "%u" @@ -23,6 +24,7 @@ loadvm_postcopy_ram_handle_discard_end(void) "" loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud" loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d" loadvm_process_command_ping(uint32_t val) "0x%x" +loadvm_approve_switchover(unsigned int switchover_ack_pending_num) "Switchover ack pending num=%u" postcopy_ram_listen_thread_exit(void) "" postcopy_ram_listen_thread_start(void) "" qemu_savevm_send_postcopy_advise(void) "" @@ -39,6 +41,7 @@ savevm_send_postcopy_resume(void) "" savevm_send_colo_enable(void) "" savevm_send_recv_bitmap(char *name) "%s" savevm_state_setup(void) "" +savevm_state_switchover_ack_needed(unsigned int switchover_ack_user_num) "Switchover ack user num=%u" savevm_state_resume_prepare(void) "" savevm_state_header(void) "" savevm_state_iterate(void) "" @@ -180,6 +183,7 @@ source_return_path_thread_loop_top(void) "" source_return_path_thread_pong(uint32_t val) "0x%x" source_return_path_thread_shut(uint32_t val) "0x%x" source_return_path_thread_resume_ack(uint32_t v) "%"PRIu32 +source_return_path_thread_switchover_acked(void) "" migration_thread_low_pending(uint64_t pending) "%" PRIu64 migrate_transferred(uint64_t tranferred, uint64_t time_spent, uint64_t bandwidth, uint64_t size) "transferred %" PRIu64 " time_spent %" PRIu64 " bandwidth %" PRIu64 " max_size %" PRId64 process_incoming_migration_co_end(int ret, int ps) "ret=%d postcopy-state=%d"
Implement switchover ack logic. This prevents the source from stopping the VM and completing the migration until an ACK is received from the destination that it's OK to do so. To achieve this, a new SaveVMHandlers handler switchover_ack_needed() and a new return path message MIG_RP_MSG_SWITCHOVER_ACK are added. The switchover_ack_needed() handler is called during migration setup both in the source and the destination to check if switchover ack is used by the migrated device. When switchover is approved by all migrated devices in the destination that support this capability, the MIG_RP_MSG_INITIAL_DATA_LOADED_ACK return path message is sent to the source to notify it that it's OK to do switchover. Signed-off-by: Avihai Horon <avihaih@nvidia.com> --- include/migration/register.h | 3 ++ migration/migration.h | 16 +++++++++++ migration/savevm.h | 2 ++ migration/migration.c | 42 +++++++++++++++++++++++++-- migration/savevm.c | 56 ++++++++++++++++++++++++++++++++++++ migration/trace-events | 4 +++ 6 files changed, 121 insertions(+), 2 deletions(-)