[V1,1/4] migration: mode parameter

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(-)

Steve Sistare <steven.sistare@oracle.com> 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>

[... qdev definition ...]

Looks legit, but I am not an expert here.


> @@ -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;
> +}
> +

Inside parameters, I try to get the functions sorted by name.  the same
for options.h

> 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' ] }
> +
> +##

Here you only have normal, but in qdev you also have exec.


>  # @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)
> +#

You normally put comments and values at the end of the comments and
sections. Your sshould be last.

Feel free to use a single line in the json.  More than one value for
line make it a bit more compress, but makes changes more complicated.

Rest of the patch feels right.

Thanks, Juan.

On 10/20/2023 5:29 AM, Juan Quintela wrote:
> Steve Sistare <steven.sistare@oracle.com> 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>
> 
> [... qdev definition ...]
> 
> Looks legit, but I am not an expert here.

Nor I, but I copied a similar definition line for line, see
  qdev_prop_blockdev_on_error
  DEFINE_PROP_BLOCKDEV_ON_ERROR

However, I now see I am missing:
  QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));

I can ask Daniel Berrange to review this part if you prefer.

>> @@ -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;
>> +}
>> +
> 
> Inside parameters, I try to get the functions sorted by name.  the same
> for options.h

Sure, will do.

>> 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' ] }
>> +
>> +##
> 
> Here you only have normal, but in qdev you also have exec.

Good eye.  I will remove exec from .description in this patch, and add
cpr-reboot to it in patch 4.

>>  # @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)
>> +#
> 
> You normally put comments and values at the end of the comments and
> sections. Your sshould be last.

Do you mean, add the mode parameter at the end of the existing parameters, 
after vcpu-dirty-limit?

> Feel free to use a single line in the json.  More than one value for
> line make it a bit more compress, but makes changes more complicated.

Like this?

{ 'enum': 'MigrationParameter',
  'data': ['announce-initial', 'announce-max',
          ...
          'vcpu-dirty-limit',
          'mode'] }

- Steve

Steven Sistare <steven.sistare@oracle.com> wrote:
> On 10/20/2023 5:29 AM, Juan Quintela wrote:
>> Steve Sistare <steven.sistare@oracle.com> 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>
>> 
>> [... qdev definition ...]
>> 
>> Looks legit, but I am not an expert here.
>
> Nor I, but I copied a similar definition line for line, see
>   qdev_prop_blockdev_on_error
>   DEFINE_PROP_BLOCKDEV_ON_ERROR
>
> However, I now see I am missing:
>   QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));
>
> I can ask Daniel Berrange to review this part if you prefer.

I reviewed it, but I agree that looks legit to me O:-)

>>> 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' ] }
>>> +
>>> +##
>> 
>> Here you only have normal, but in qdev you also have exec.
>
> Good eye.  I will remove exec from .description in this patch, and add
> cpr-reboot to it in patch 4.

Thanks.

>>>  # @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)
>>> +#
>> 
>> You normally put comments and values at the end of the comments and
>> sections. Your sshould be last.
>
> Do you mean, add the mode parameter at the end of the existing parameters, 
> after vcpu-dirty-limit?
>
>> Feel free to use a single line in the json.  More than one value for
>> line make it a bit more compress, but makes changes more complicated.
>
> Like this?
>
> { 'enum': 'MigrationParameter',
>   'data': ['announce-initial', 'announce-max',
>           ...
>           'vcpu-dirty-limit',
>           'mode'] }

Exactwly.  Same for the comments at the end of the list.

> - Steve

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',