Re: [PATCH RFC 2/2] migration/cpr: Opt-in "mode" parameter for early boot access

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> Make "mode" to be the first parameter to opt-in for early boot access.  CPR
> will start to consume this early boot parameter.  With this, we can remove
> the special "incoming_mode" variable together with cpr_get_incoming_mode().
>

Hmm, it's now possible to invoke QEMU with:

-incoming config:mode=cpr-exec,cpr-exec-command=-incoming config:

Seems like it could be an issue.

> One thing to mention is, to make cpr_is_incoming() work like before, we
> need to do extra check on INMIGRATE runstate to make sure it only returns
> true while during the incoming migration.

...

> It used to be achieved by a
> pretty hackish "cpr_set_incoming_mode(MIG_MODE_NONE)" when incoming
> migration destroys.  Now we can remove it.

That's not it, I think. cpr_is_incoming() only returns true on incoming
migration because on outgoing there's no parsing of -incoming and
therefore cpr_state_load() doesn't call cpr_set_incoming_mode().

>
> Another good side effect is, we can finally drop MIG_MODE_NONE: it was
> never a valid value, only used by the temp global variable.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/cpr.h |  3 ---
>  migration/migration.h   |  3 +++
>  migration/cpr.c         | 18 +++++++++---------
>  migration/migration.c   | 37 ++++++++++++++++++++++++++++++++++++-
>  migration/options.c     | 10 +++-------
>  5 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> index 56fb67e6b4..27061ad629 100644
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
> @@ -12,8 +12,6 @@
>  #include "io/channel.h"
>  #include "qemu/queue.h"
>  
> -#define MIG_MODE_NONE           -1
> -
>  #define QEMU_CPR_FILE_MAGIC     0x51435052
>  #define QEMU_CPR_FILE_VERSION   0x00000001
>  #define CPR_STATE "CprState"
> @@ -38,7 +36,6 @@ int cpr_open_fd(const char *path, int flags, const char *name, int id,
>  typedef bool (*cpr_walk_fd_cb)(int fd);
>  bool cpr_walk_fd(cpr_walk_fd_cb cb);
>  
> -MigMode cpr_get_incoming_mode(void);
>  void cpr_set_incoming_mode(MigMode mode);
>  bool cpr_is_incoming(void);
>  
> diff --git a/migration/migration.h b/migration/migration.h
> index 841f49b215..9fa97f6d9a 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -609,4 +609,7 @@ void migration_bitmap_sync_precopy(bool last_stage);
>  void dirty_bitmap_mig_init(void);
>  bool should_send_vmdesc(void);
>  
> +void migration_parameters_boot_set_mode(MigMode mode);
> +MigrationParameters *migration_get_parameters(void);
> +
>  #endif
> diff --git a/migration/cpr.c b/migration/cpr.c
> index bca43e9bf3..1f49afe109 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -16,6 +16,7 @@
>  #include "migration/qemu-file.h"
>  #include "migration/savevm.h"
>  #include "migration/vmstate.h"
> +#include "migration/migration.h"
>  #include "monitor/monitor.h"
>  #include "system/runstate.h"
>  #include "trace.h"
> @@ -239,21 +240,20 @@ QIOChannel *cpr_state_ioc(void)
>      return qemu_file_get_ioc(cpr_state_file);
>  }
>  
> -static MigMode incoming_mode = MIG_MODE_NONE;
> -
> -MigMode cpr_get_incoming_mode(void)
> -{
> -    return incoming_mode;
> -}
> -
>  void cpr_set_incoming_mode(MigMode mode)
>  {
> -    incoming_mode = mode;
> +    migration_parameters_boot_set_mode(mode);
>  }
>  
>  bool cpr_is_incoming(void)
>  {
> -    return incoming_mode != MIG_MODE_NONE;
> +    MigMode mode = migrate_mode();
> +
> +    if (!runstate_check(RUN_STATE_INMIGRATE)) {
> +        return false;
> +    }
> +
> +    return mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER;
>  }
>  
>  bool cpr_state_save(MigrationChannel *channel, Error **errp)
> diff --git a/migration/migration.c b/migration/migration.c
> index d918be7a44..d7591571ed 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -145,6 +145,42 @@ static void migration_parameters_boot_apply(void)
>      }
>  }
>  
> +static void migration_parameters_boot_init(void)
> +{
> +    if (!mig_boot_params) {
> +        mig_boot_params = g_new0(MigrationParameters, 1);
> +    }
> +}
> +
> +/*
> + * This should only be used during boot by CPR.  NOTE: This is only needed
> + * to be compatible with old CPR use case, if we decide to have users
> + * switch to -incoming config:mode=cpr-* then this can be removed.

This new scheme does create some usability weirdness of allowing a
parameter to be set in -incoming and then to be changed later with
migrate-set-parameters. Hopefully it's ok.

> + */
> +void migration_parameters_boot_set_mode(MigMode mode)
> +{
> +    assert(!current_migration);
> +    migration_parameters_boot_init();
> +    mig_boot_params->has_mode = true;
> +    mig_boot_params->mode = mode;

I bet there's some visitor trick that could make this function generic
for all parameters.

> +}
> +
> +/*
> + * Get the effective migration parameter object.
> + *
> + * Three possibilities:
> + * - Migration object has been initialized, always use it, or,
> + * - Migration boot parameters are initialized, then use it, or,
> + * - return NULL

Hmm, can't we create the early parameters unconditionally so there's no
NULL case here? A bit annoying having to check for NULL for (eventually)
every parameter, even on the source side.

> + *
> + * Callers should always check non-NULL pointer first before use.
> + */
> +MigrationParameters *migration_get_parameters(void)
> +{
> +    return current_migration ?
> +        &current_migration->parameters : mig_boot_params;
> +}

Feels like time to drop the global parameters already. It's a weird
cognitive load having to grasp that there's two MigrationParameters,
both used in outgoing and incoming, but one can only be set in
incoming. The one stored in current_migration might be used
interchangeably with the one that's flying around. One lives inside and
shares lifetime with MigrationState, the other is dynamically allocated
and has a shorter lifetime.

> +
>  static void migration_downtime_start(MigrationState *s)
>  {
>      trace_vmstate_downtime_checkpoint("src-downtime-start");
> @@ -546,7 +582,6 @@ void migration_incoming_state_destroy(void)
>          mis->postcopy_qemufile_dst = NULL;
>      }
>  
> -    cpr_set_incoming_mode(MIG_MODE_NONE);
>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>  }
>  
> diff --git a/migration/options.c b/migration/options.c
> index 5cbfd29099..c1209cbec3 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -886,16 +886,12 @@ uint64_t migrate_max_postcopy_bandwidth(void)
>      return s->parameters.max_postcopy_bandwidth;
>  }
>  
> +/* Opt-in "mode" parameter to be available even during boot */
>  MigMode migrate_mode(void)
>  {
> -    MigMode mode = cpr_get_incoming_mode();
> +    MigrationParameters *params = migration_get_parameters();
>  
> -    if (mode == MIG_MODE_NONE) {
> -        mode = migrate_get_current()->parameters.mode;
> -    }
> -
> -    assert(mode >= 0 && mode < MIG_MODE__MAX);
> -    return mode;
> +    return params ? params->mode : MIG_MODE_NORMAL;
>  }
>  
>  int migrate_multifd_channels(void)