Peter Xu <[email protected]> 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 <[email protected]> > --- > 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 ? > + ¤t_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)
