Re: [PATCH V2 03/13] migration: save cpr mode

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 30, 2024 at 12:40:34PM -0700, Steve Sistare wrote:
> Save the mode in CPR state, so the user does not need to explicitly specify
> it for the target.  Modify migrate_mode() so it returns the incoming mode on
> the target.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/migration/cpr.h |  7 +++++++
>  migration/cpr.c         | 23 ++++++++++++++++++++++-
>  migration/migration.c   |  1 +
>  migration/options.c     |  9 +++++++--
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> index e7b898b..ac7a63e 100644
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
> @@ -8,9 +8,16 @@
>  #ifndef MIGRATION_CPR_H
>  #define MIGRATION_CPR_H
>  
> +#include "qapi/qapi-types-migration.h"
> +
> +#define MIG_MODE_NONE           -1
> +
>  #define QEMU_CPR_FILE_MAGIC     0x51435052
>  #define QEMU_CPR_FILE_VERSION   0x00000001
>  
> +MigMode cpr_get_incoming_mode(void);
> +void cpr_set_incoming_mode(MigMode mode);
> +
>  typedef int (*cpr_walk_fd_cb)(int fd);
>  void cpr_save_fd(const char *name, int id, int fd);
>  void cpr_delete_fd(const char *name, int id);
> diff --git a/migration/cpr.c b/migration/cpr.c
> index e50fc75..7514c4e 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -21,10 +21,23 @@
>  typedef QLIST_HEAD(CprFdList, CprFd) CprFdList;
>  
>  typedef struct CprState {
> +    MigMode mode;
>      CprFdList fds;
>  } CprState;
>  
> -static CprState cpr_state;
> +static CprState cpr_state = {
> +    .mode = MIG_MODE_NONE,
> +};
> +
> +MigMode cpr_get_incoming_mode(void)
> +{
> +    return cpr_state.mode;
> +}
> +
> +void cpr_set_incoming_mode(MigMode mode)
> +{
> +    cpr_state.mode = mode;
> +}
>  
>  /****************************************************************************/
>  
> @@ -124,11 +137,19 @@ void cpr_resave_fd(const char *name, int id, int fd)
>  /*************************************************************************/
>  #define CPR_STATE "CprState"
>  
> +static int cpr_state_presave(void *opaque)
> +{
> +    cpr_state.mode = migrate_mode();
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_cpr_state = {
>      .name = CPR_STATE,
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_save = cpr_state_presave,
>      .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(mode, CprState),
>          VMSTATE_QLIST_V(fds, CprState, 1, vmstate_cpr_fd, CprFd, next),
>          VMSTATE_END_OF_LIST()
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 834b0a2..df00e5c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -416,6 +416,7 @@ 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 147cd2b..cc85a84 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qmp/qnull.h"
>  #include "sysemu/runstate.h"
>  #include "migration/colo.h"
> +#include "migration/cpr.h"
>  #include "migration/misc.h"
>  #include "migration.h"
>  #include "migration-stats.h"
> @@ -768,8 +769,12 @@ uint64_t migrate_max_postcopy_bandwidth(void)
>  
>  MigMode migrate_mode(void)
>  {
> -    MigrationState *s = migrate_get_current();
> -    MigMode mode = s->parameters.mode;
> +    MigMode mode = cpr_get_incoming_mode();
> +
> +    if (mode == MIG_MODE_NONE) {
> +        MigrationState *s = migrate_get_current();
> +        mode = s->parameters.mode;
> +    }

Is this trying to avoid interfering with what user specified?

I can kind of get the point of it, but it'll also look pretty werid in this
case that user can set the mode but then when query before cpr-transfer
incoming completes it won't read what was set previously, but what was
migrated via the cpr channel.

And IIUC it is needed to migrate this mode in cpr stream so as to avoid
another new qemu cmdline on dest qemu.  If true this needs to be mentioned
in the commit message; so far it reads like it's optional, then it's not
clear why only cpr-mode needs to be migrated not other migration parameters.

If that won't get right easily, I wonder whether we could just overwrite
parameters.mode directly by the cpr stream.  After all IIUC that's before
QMP is available, so there's no legal way to set it, then no legal way that
it overwrites an user input?

>  
>      assert(mode >= 0 && mode < MIG_MODE__MAX);
>      return mode;
> -- 
> 1.8.3.1
> 

-- 
Peter Xu