Re: [PATCH v4 11/14] migration/multifd: Add migration option set packet size.

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 25, 2024 at 02:21:14AM +0000, Hao Xiang wrote:
> The current multifd packet size is 128 * 4kb. This change adds
> an option to set the packet size. Both sender and receiver needs
> to set the same packet size for things to work.
> 
> Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> ---
>  migration/options.c | 36 ++++++++++++++++++++++++++++++++++++
>  migration/options.h |  1 +
>  qapi/migration.json | 21 ++++++++++++++++++---
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index dc8642df81..a9deb079eb 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -79,6 +79,12 @@
>  #define DEFAULT_MIGRATE_ANNOUNCE_ROUNDS    5
>  #define DEFAULT_MIGRATE_ANNOUNCE_STEP    100
>  
> +/*
> + * Parameter for multifd packet size.
> + */
> +#define DEFAULT_MIGRATE_MULTIFD_PACKET_SIZE (128 * 4 * 1024)
> +#define MAX_MIGRATE_MULTIFD_PACKET_SIZE (1023 * 4 * 1024)
> +
>  #define DEFINE_PROP_MIG_CAP(name, x)             \
>      DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
>  
> @@ -184,6 +190,9 @@ Property migration_properties[] = {
>                         ZERO_PAGE_DETECTION_MULTIFD),
>      DEFINE_PROP_STRING("multifd-dsa-accel", MigrationState,
>                         parameters.multifd_dsa_accel),
> +    DEFINE_PROP_SIZE("multifd-packet-size", MigrationState,
> +                     parameters.multifd_packet_size,
> +                     DEFAULT_MIGRATE_MULTIFD_PACKET_SIZE),

Having such knob looks all fine, but I feel like this patch is half-baked,
no?  There seems to have another part in the next patch.  Maybe they need
to be squashed together.

>  
>      /* Migration capabilities */
>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> @@ -879,6 +888,13 @@ int migrate_multifd_channels(void)
>      return s->parameters.multifd_channels;
>  }
>  
> +uint64_t migrate_multifd_packet_size(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    return s->parameters.multifd_packet_size;
> +}
> +
>  MultiFDCompression migrate_multifd_compression(void)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -1031,6 +1047,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
>      params->has_block_incremental = true;
>      params->block_incremental = s->parameters.block_incremental;
> +    params->has_multifd_packet_size = true;
> +    params->multifd_packet_size = s->parameters.multifd_packet_size;
>      params->has_multifd_channels = true;
>      params->multifd_channels = s->parameters.multifd_channels;
>      params->has_multifd_compression = true;
> @@ -1094,6 +1112,7 @@ void migrate_params_init(MigrationParameters *params)
>      params->has_downtime_limit = true;
>      params->has_x_checkpoint_delay = true;
>      params->has_block_incremental = true;
> +    params->has_multifd_packet_size = true;
>      params->has_multifd_channels = true;
>      params->has_multifd_compression = true;
>      params->has_multifd_zlib_level = true;
> @@ -1195,6 +1214,17 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>  
>      /* x_checkpoint_delay is now always positive */
>  
> +    if (params->has_multifd_packet_size &&
> +        ((params->multifd_packet_size < DEFAULT_MIGRATE_MULTIFD_PACKET_SIZE) ||
> +            (params->multifd_packet_size >  MAX_MIGRATE_MULTIFD_PACKET_SIZE) ||
> +            (params->multifd_packet_size % qemu_target_page_size() != 0))) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                    "multifd_packet_size",
> +                    "a value between 524288 and 4190208, "

We should reference the macros here.

> +                    "must be a multiple of guest VM's page size.");
> +        return false;
> +    }
> +
>      if (params->has_multifd_channels && (params->multifd_channels < 1)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "multifd_channels",
> @@ -1374,6 +1404,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_block_incremental) {
>          dest->block_incremental = params->block_incremental;
>      }
> +    if (params->has_multifd_packet_size) {
> +        dest->multifd_packet_size = params->multifd_packet_size;
> +    }
>      if (params->has_multifd_channels) {
>          dest->multifd_channels = params->multifd_channels;
>      }
> @@ -1524,6 +1557,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>                      " use blockdev-mirror with NBD instead");
>          s->parameters.block_incremental = params->block_incremental;
>      }
> +    if (params->has_multifd_packet_size) {
> +        s->parameters.multifd_packet_size = params->multifd_packet_size;
> +    }
>      if (params->has_multifd_channels) {
>          s->parameters.multifd_channels = params->multifd_channels;
>      }
> diff --git a/migration/options.h b/migration/options.h
> index 1cb3393be9..23995e6608 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -92,6 +92,7 @@ const char *migrate_tls_hostname(void);
>  uint64_t migrate_xbzrle_cache_size(void);
>  ZeroPageDetection migrate_zero_page_detection(void);
>  const char *migrate_multifd_dsa_accel(void);
> +uint64_t migrate_multifd_packet_size(void);
>  
>  /* parameters setters */
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 934fa8839e..39d609c394 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -920,6 +920,10 @@
>  #     characters. Setting this string to an empty string means disabling
>  #     DSA accelerator offloading. Defaults to an empty string. (since 9.2)
>  #
> +# @multifd-packet-size: Packet size in bytes used to migrate data.
> +#     The value needs to be a multiple of guest VM's page size.

Maybe just call it "guest page size".

> +#     The default value is 524288 and max value is 4190208. (Since 9.2)

IMHO we can avoid mentioning these in QAPI.  This will be a very, very,
developer oriented value: if the default isn't the best to the majority of
people, we should change the default.  Not easy for an admin to understand
what is this about.

I'm even thinking whether we should only expose it via one migration debug
option (-global migration.multifd-packet-size only), rather exporting it in
QMP or even HMP.  Or do you want this actually to be tunable for real?

> +#
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated.  Use
> @@ -954,7 +958,8 @@
>             { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] },
>             'vcpu-dirty-limit',
>             'mode',
> -           'zero-page-detection'] }
> +           'zero-page-detection',
> +           'multifd-packet-size'] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -1134,6 +1139,10 @@
>  #     characters. Setting this string to an empty string means disabling
>  #     DSA accelerator offloading. Defaults to an empty string. (since 9.2)
>  #
> +# @multifd-packet-size: Packet size in bytes used to migrate data.
> +#     The value needs to be a multiple of guest VM's page size.
> +#     The default value is 524288 and max value is 4190208. (Since 9.2)
> +#
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated.  Use
> @@ -1189,7 +1198,8 @@
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
>              '*zero-page-detection': 'ZeroPageDetection',
> -            '*multifd-dsa-accel': 'StrOrNull'} }
> +            '*multifd-dsa-accel': 'StrOrNull',
> +            '*multifd-packet-size' : 'uint64'} }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -1373,6 +1383,10 @@
>  #     characters. Setting this string to an empty string means disabling
>  #     DSA accelerator offloading. Defaults to an empty string. (since 9.2)
>  #
> +# @multifd-packet-size: Packet size in bytes used to migrate data.
> +#     The value needs to be a multiple of guest VM's page size.
> +#     The default value is 524288 and max value is 4190208. (Since 9.2)
> +#
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated.  Use
> @@ -1425,7 +1439,8 @@
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
>              '*zero-page-detection': 'ZeroPageDetection',
> -            '*multifd-dsa-accel': 'str'} }
> +            '*multifd-dsa-accel': 'str',
> +            '*multifd-packet-size': 'uint64'} }
>  
>  ##
>  # @query-migrate-parameters:
> -- 
> 2.30.2
> 
> 

-- 
Peter Xu