Re: [PATCH V1 1/4] migration: mode parameter

[Steven Sistare]

Hi Daniel, does the addition of MigMode in qdev below look OK to you?
It exactly mirrors qdev_prop_blockdev_on_error + DEFINE_PROP_BLOCKDEV_ON_ERROR.

I realize I need to add:
    QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));
and I need to delete "exec" from the .description.

I will cc you when I submit V2 of the patch.

- Steve

On 10/19/2023 4:47 PM, Steve Sistare wrote:
> Create a mode migration parameter that can be used to select alternate
> migration algorithms.  The default mode is normal, representing the
> current migration algorithm, and does not need to be explicitly set.
> 
> No functional change until a new mode is added, except that the mode is
> shown by the 'info migrate' command.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/core/qdev-properties-system.c    | 12 ++++++++++++
>  include/hw/qdev-properties-system.h |  4 ++++
>  include/migration/misc.h            |  1 +
>  migration/migration-hmp-cmds.c      |  8 ++++++++
>  migration/options.c                 | 21 +++++++++++++++++++++
>  migration/options.h                 |  1 +
>  qapi/migration.json                 | 27 ++++++++++++++++++++++++---
>  7 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 6883406..c6fd430 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -673,6 +673,18 @@ const PropertyInfo qdev_prop_multifd_compression = {
>      .set_default_value = qdev_propinfo_set_default_value_enum,
>  };
>  
> +/* --- MigMode --- */
> +
> +const PropertyInfo qdev_prop_mig_mode = {
> +    .name = "MigMode",
> +    .description = "mig_mode values, "
> +                   "normal/exec",
> +    .enum_table = &MigMode_lookup,
> +    .get = qdev_propinfo_get_enum,
> +    .set = qdev_propinfo_set_enum,
> +    .set_default_value = qdev_propinfo_set_default_value_enum,
> +};
> +
>  /* --- Reserved Region --- */
>  
>  /*
> diff --git a/include/hw/qdev-properties-system.h 
> b/include/hw/qdev-properties-system.h
> index 0ac327a..1418801 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -7,6 +7,7 @@ extern const PropertyInfo qdev_prop_chr;
>  extern const PropertyInfo qdev_prop_macaddr;
>  extern const PropertyInfo qdev_prop_reserved_region;
>  extern const PropertyInfo qdev_prop_multifd_compression;
> +extern const PropertyInfo qdev_prop_mig_mode;
>  extern const PropertyInfo qdev_prop_losttickpolicy;
>  extern const PropertyInfo qdev_prop_blockdev_on_error;
>  extern const PropertyInfo qdev_prop_bios_chs_trans;
> @@ -41,6 +42,9 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>  #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compression, \
>                         MultiFDCompression)
> +#define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
> +                       MigMode)
>  #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
>                          LostTickPolicy)
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 673ac49..1bc8902 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -15,6 +15,7 @@
>  #define MIGRATION_MISC_H
>  
>  #include "qemu/notify.h"
> +#include "qapi/qapi-types-migration.h"
>  #include "qapi/qapi-types-net.h"
>  
>  /* migration/ram.c */
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index a82597f..d8ad429 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -274,6 +274,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const 
> QDict *qdict)
>          monitor_printf(mon, "%s: %" PRIu64 " ms\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
>              params->announce_step);
> +        assert(params->has_mode);
> +        monitor_printf(mon, "%s: %s\n",
> +            MigrationParameter_str(MIGRATION_PARAMETER_MODE),
> +            qapi_enum_lookup(&MigMode_lookup, params->mode));
>          assert(params->has_compress_level);
>          monitor_printf(mon, "%s: %u\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
> @@ -514,6 +518,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
> *qdict)
>      }
>  
>      switch (val) {
> +    case MIGRATION_PARAMETER_MODE:
> +        p->has_mode = true;
> +        visit_type_MigMode(v, param, &p->mode, &err);
> +        break;
>      case MIGRATION_PARAMETER_COMPRESS_LEVEL:
>          p->has_compress_level = true;
>          visit_type_uint8(v, param, &p->compress_level, &err);
> diff --git a/migration/options.c b/migration/options.c
> index 42fb818..4f26515 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -101,6 +101,9 @@ Property migration_properties[] = {
>                       preempt_pre_7_2, false),
>  
>      /* Migration parameters */
> +    DEFINE_PROP_MIG_MODE("mode", MigrationState,
> +                      parameters.mode,
> +                      MIG_MODE_NORMAL),
>      DEFINE_PROP_UINT8("x-compress-level", MigrationState,
>                        parameters.compress_level,
>                        DEFAULT_MIGRATE_COMPRESS_LEVEL),
> @@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
>      return s->parameters.xbzrle_cache_size;
>  }
>  
> +MigMode migrate_mode(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    return s->parameters.mode;
> +}
> +
>  /* parameter setters */
>  
>  void migrate_set_block_incremental(bool value)
> @@ -911,6 +921,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
> **errp)
>  
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>      params = g_malloc0(sizeof(*params));
> +    params->has_mode = true;
> +    params->mode = s->parameters.mode;
>      params->has_compress_level = true;
>      params->compress_level = s->parameters.compress_level;
>      params->has_compress_threads = true;
> @@ -985,6 +997,7 @@ void migrate_params_init(MigrationParameters *params)
>      params->tls_creds = g_strdup("");
>  
>      /* Set has_* up only for parameter checks */
> +    params->has_mode = true;
>      params->has_compress_level = true;
>      params->has_compress_threads = true;
>      params->has_compress_wait_thread = true;
> @@ -1206,6 +1219,10 @@ static void 
> migrate_params_test_apply(MigrateSetParameters *params,
>  
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>  
> +    if (params->has_mode) {
> +        dest->mode = params->mode;
> +    }
> +
>      if (params->has_compress_level) {
>          dest->compress_level = params->compress_level;
>      }
> @@ -1315,6 +1332,10 @@ static void migrate_params_apply(MigrateSetParameters 
> *params, Error **errp)
>  
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>  
> +    if (params->has_mode) {
> +        s->parameters.mode = params->mode;
> +    }
> +
>      if (params->has_compress_level) {
>          s->parameters.compress_level = params->compress_level;
>      }
> diff --git a/migration/options.h b/migration/options.h
> index 237f2d6..d9ec873 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -92,6 +92,7 @@ const char *migrate_tls_authz(void);
>  const char *migrate_tls_creds(void);
>  const char *migrate_tls_hostname(void);
>  uint64_t migrate_xbzrle_cache_size(void);
> +MigMode migrate_mode(void);
>  
>  /* parameters setters */
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index db3df12..184fb78 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -616,6 +616,15 @@
>              { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>  
>  ##
> +# @MigMode:
> +#
> +# @normal: the original form of migration. (since 8.2)
> +#
> +##
> +{ 'enum': 'MigMode',
> +  'data': [ 'normal' ] }
> +
> +##
>  # @BitmapMigrationBitmapAliasTransform:
>  #
>  # @persistent: If present, the bitmap will be made persistent or
> @@ -675,6 +684,9 @@
>  #
>  # Migration parameters enumeration
>  #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +#        (Since 8.2)
> +#
>  # @announce-initial: Initial delay (in milliseconds) before sending
>  #     the first announce (Since 4.0)
>  #
> @@ -841,7 +853,8 @@
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> -  'data': ['announce-initial', 'announce-max',
> +  'data': ['mode',
> +           'announce-initial', 'announce-max',
>             'announce-rounds', 'announce-step',
>             'compress-level', 'compress-threads', 'decompress-threads',
>             'compress-wait-thread', 'throttle-trigger-threshold',
> @@ -862,6 +875,9 @@
>  ##
>  # @MigrateSetParameters:
>  #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +#        (Since 8.2)
> +#
>  # @announce-initial: Initial delay (in milliseconds) before sending
>  #     the first announce (Since 4.0)
>  #
> @@ -1020,7 +1036,8 @@
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrateSetParameters',
> -  'data': { '*announce-initial': 'size',
> +  'data': { '*mode': 'MigMode',
> +            '*announce-initial': 'size',
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',
> @@ -1074,6 +1091,9 @@
>  #
>  # The optional members aren't actually optional.
>  #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +#        (Since 8.2)
> +#
>  # @announce-initial: Initial delay (in milliseconds) before sending
>  #     the first announce (Since 4.0)
>  #
> @@ -1231,7 +1251,8 @@
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> -  'data': { '*announce-initial': 'size',
> +  'data': { '*mode': 'MigMode',
> +            '*announce-initial': 'size',
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',