On Wed, Feb 21, 2024 at 5:58 AM Elena Ufimtseva <ufimts...@gmail.com> wrote:
>
>
>
> On Fri, Feb 16, 2024 at 2:41 PM Hao Xiang <hao.xi...@bytedance.com> wrote:
>>
>> This new parameter controls where the zero page checking is running.
>> 1. If this parameter is set to 'legacy', zero page checking is
>> done in the migration main thread.
>> 2. If this parameter is set to 'none', zero page checking is disabled.
>>
>
> Hello Hao
>
> Few questions and comments.
>
> First the commit message states that the parameter control where the checking
> is done, but it also controls
> if sending of zero pages is done by multifd threads or not.
>
>
>>
>> Signed-off-by: Hao Xiang <hao.xi...@bytedance.com>
>> ---
>> hw/core/qdev-properties-system.c | 10 ++++++++++
>> include/hw/qdev-properties-system.h | 4 ++++
>> migration/migration-hmp-cmds.c | 9 +++++++++
>> migration/options.c | 21 ++++++++++++++++++++
>> migration/options.h | 1 +
>> migration/ram.c | 4 ++++
>> qapi/migration.json | 30 ++++++++++++++++++++++++++---
>> 7 files changed, 76 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/core/qdev-properties-system.c
>> b/hw/core/qdev-properties-system.c
>> index 1a396521d5..63843f18b5 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -679,6 +679,16 @@ const PropertyInfo qdev_prop_mig_mode = {
>> .set_default_value = qdev_propinfo_set_default_value_enum,
>> };
>>
>> +const PropertyInfo qdev_prop_zero_page_detection = {
>> + .name = "ZeroPageDetection",
>> + .description = "zero_page_detection values, "
>> + "multifd,legacy,none",
>> + .enum_table = &ZeroPageDetection_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 06c359c190..839b170235 100644
>> --- a/include/hw/qdev-properties-system.h
>> +++ b/include/hw/qdev-properties-system.h
>> @@ -8,6 +8,7 @@ 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_zero_page_detection;
>> extern const PropertyInfo qdev_prop_losttickpolicy;
>> extern const PropertyInfo qdev_prop_blockdev_on_error;
>> extern const PropertyInfo qdev_prop_bios_chs_trans;
>> @@ -47,6 +48,9 @@ extern const PropertyInfo
>> qdev_prop_iothread_vq_mapping_list;
>> #define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
>> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
>> MigMode)
>> +#define DEFINE_PROP_ZERO_PAGE_DETECTION(_n, _s, _f, _d) \
>> + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_zero_page_detection, \
>> + ZeroPageDetection)
>> #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
>> LostTickPolicy)
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index 99b49df5dd..7e96ae6ffd 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -344,6 +344,11 @@ void hmp_info_migrate_parameters(Monitor *mon, const
>> QDict *qdict)
>> monitor_printf(mon, "%s: %s\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION),
>> MultiFDCompression_str(params->multifd_compression));
>> + assert(params->has_zero_page_detection);
>
>
> What is the reason to have assert here?
It's just to verify that the option is initialized properly before we
reach here. Same things are done for other options.
>
>>
>> + monitor_printf(mon, "%s: %s\n",
>> + MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION),
>> + qapi_enum_lookup(&ZeroPageDetection_lookup,
>> + params->zero_page_detection));
>> monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
>> params->xbzrle_cache_size);
>> @@ -634,6 +639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const
>> QDict *qdict)
>> p->has_multifd_zstd_level = true;
>> visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>> break;
>> + case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
>> + p->has_zero_page_detection = true;
>> + visit_type_ZeroPageDetection(v, param, &p->zero_page_detection,
>> &err);
>> + break;
>> case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>> p->has_xbzrle_cache_size = true;
>> if (!visit_type_size(v, param, &cache_size, &err)) {
>> diff --git a/migration/options.c b/migration/options.c
>> index 3e3e0b93b4..3c603391b0 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -179,6 +179,9 @@ Property migration_properties[] = {
>> DEFINE_PROP_MIG_MODE("mode", MigrationState,
>> parameters.mode,
>> MIG_MODE_NORMAL),
>> + DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>> + parameters.zero_page_detection,
>> + ZERO_PAGE_DETECTION_LEGACY),
>>
>> /* Migration capabilities */
>> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>> @@ -903,6 +906,13 @@ uint64_t migrate_xbzrle_cache_size(void)
>> return s->parameters.xbzrle_cache_size;
>> }
>>
>> +ZeroPageDetection migrate_zero_page_detection(void)
>> +{
>> + MigrationState *s = migrate_get_current();
>> +
>> + return s->parameters.zero_page_detection;
>> +}
>> +
>> /* parameter setters */
>>
>> void migrate_set_block_incremental(bool value)
>> @@ -1013,6 +1023,8 @@ MigrationParameters
>> *qmp_query_migrate_parameters(Error **errp)
>> params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
>> params->has_mode = true;
>> params->mode = s->parameters.mode;
>> + params->has_zero_page_detection = true;
>> + params->zero_page_detection = s->parameters.zero_page_detection;
>>
>> return params;
>> }
>> @@ -1049,6 +1061,7 @@ void migrate_params_init(MigrationParameters *params)
>> params->has_x_vcpu_dirty_limit_period = true;
>> params->has_vcpu_dirty_limit = true;
>> params->has_mode = true;
>> + params->has_zero_page_detection = true;
>> }
>>
>> /*
>> @@ -1350,6 +1363,10 @@ static void
>> migrate_params_test_apply(MigrateSetParameters *params,
>> if (params->has_mode) {
>> dest->mode = params->mode;
>> }
>> +
>> + if (params->has_zero_page_detection) {
>> + dest->zero_page_detection = params->zero_page_detection;
>> + }
>> }
>>
>> static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>> @@ -1494,6 +1511,10 @@ static void migrate_params_apply(MigrateSetParameters
>> *params, Error **errp)
>> if (params->has_mode) {
>> s->parameters.mode = params->mode;
>> }
>> +
>> + if (params->has_zero_page_detection) {
>> + s->parameters.zero_page_detection = params->zero_page_detection;
>> + }
>> }
>>
>> void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>> diff --git a/migration/options.h b/migration/options.h
>> index 246c160aee..b7c4fb3861 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -93,6 +93,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);
>> +ZeroPageDetection migrate_zero_page_detection(void);
>>
>> /* parameters setters */
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 4649a81204..556725c30f 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1123,6 +1123,10 @@ static int save_zero_page(RAMState *rs,
>> PageSearchStatus *pss,
>> QEMUFile *file = pss->pss_channel;
>> int len = 0;
>>
>> + if (migrate_zero_page_detection() != ZERO_PAGE_DETECTION_LEGACY) {
>> + return 0;
>> + }
>> +
>> if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>> return 0;
>> }
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 5a565d9b8d..99843a8e95 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -653,6 +653,17 @@
>> { 'enum': 'MigMode',
>> 'data': [ 'normal', 'cpr-reboot' ] }
>>
>> +##
>> +# @ZeroPageDetection:
>> +#
>> +# @legacy: Perform zero page checking from main migration thread. (since
>> 9.0)
>> +#
>> +# @none: Do not perform zero page checking.
>> +#
>> +##
>> +{ 'enum': 'ZeroPageDetection',
>> + 'data': [ 'legacy', 'none' ] }
>> +
>
>
> Above you have introduced the qdev property qdev_prop_zero_page_detection
> with multifd, but it is not present in the scheme.
> Perhaps 'mulitfd' in qdev_prop_zero_page_detection belongs to another patch?
You are right. I will fix that.
>
>
>>
>> ##
>> # @BitmapMigrationBitmapAliasTransform:
>> #
>> @@ -874,6 +885,9 @@
>> # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>> # (Since 8.2)
>> #
>> +# @zero-page-detection: See description in @ZeroPageDetection.
>> +# Default is 'legacy'. (Since 9.0)
>> +#
>> # Features:
>> #
>> # @deprecated: Member @block-incremental is deprecated. Use
>> @@ -907,7 +921,8 @@
>> 'block-bitmap-mapping',
>> { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable']
>> },
>> 'vcpu-dirty-limit',
>> - 'mode'] }
>> + 'mode',
>> + 'zero-page-detection'] }
>>
>> ##
>> # @MigrateSetParameters:
>> @@ -1066,6 +1081,10 @@
>> # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>> # (Since 8.2)
>> #
>> +# @zero-page-detection: See description in @ZeroPageDetection.
>> +# Default is 'legacy'. (Since 9.0)
>> +#
>> +#
>> # Features:
>> #
>> # @deprecated: Member @block-incremental is deprecated. Use
>> @@ -1119,7 +1138,8 @@
>> '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> 'features': [ 'unstable' ] },
>> '*vcpu-dirty-limit': 'uint64',
>> - '*mode': 'MigMode'} }
>> + '*mode': 'MigMode',
>> + '*zero-page-detection': 'ZeroPageDetection'} }
>>
>> ##
>> # @migrate-set-parameters:
>> @@ -1294,6 +1314,9 @@
>> # @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>> # (Since 8.2)
>> #
>> +# @zero-page-detection: See description in @ZeroPageDetection.
>> +# Default is 'legacy'. (Since 9.0)
>> +#
>> # Features:
>> #
>> # @deprecated: Member @block-incremental is deprecated. Use
>> @@ -1344,7 +1367,8 @@
>> '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> 'features': [ 'unstable' ] },
>> '*vcpu-dirty-limit': 'uint64',
>> - '*mode': 'MigMode'} }
>> + '*mode': 'MigMode',
>> + '*zero-page-detection': 'ZeroPageDetection'} }
>>
>> ##
>> # @query-migrate-parameters:
>> --
>> 2.30.2
>>
>>
>
>
> --
> Elena
