Re: [PATCH v3 2/7] migration: Implement switchover ack logic

[Peter Xu]

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
> 

-- 
Peter Xu