| Message ID | 20190403114958.3705-7-quintela@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | WIP: Multifd compression support | expand |
Juan Quintela <quintela@redhat.com> writes: > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > Rename it to NONE > --- > hmp.c | 17 +++++++++++++++++ > hw/core/qdev-properties.c | 11 +++++++++++ > include/hw/qdev-properties.h | 1 + > migration/migration.c | 16 ++++++++++++++++ > qapi/common.json | 13 +++++++++++++ > qapi/migration.json | 19 +++++++++++++++---- > tests/migration-test.c | 13 ++++++++++--- > 7 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/hmp.c b/hmp.c > index 8eec768088..02fbe27757 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -435,6 +435,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "%s: %u\n", > MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), > params->multifd_channels); > + monitor_printf(mon, "%s: %s\n", > + MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESS), > + MultifdCompress_str(params->multifd_compress)); > monitor_printf(mon, "%s: %" PRIu64 "\n", > MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), > params->xbzrle_cache_size); > @@ -1737,6 +1740,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); > uint64_t valuebw = 0; > uint64_t cache_size; > + int compress_type; > Error *err = NULL; > int val, ret; > > @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->has_multifd_channels = true; > visit_type_int(v, param, &p->multifd_channels, &err); > break; > + case MIGRATION_PARAMETER_MULTIFD_COMPRESS: > + p->has_multifd_compress = true; > + visit_type_enum(v, param, &compress_type, > + &MultifdCompress_lookup, &err); visit_type_MultifdCompress(), please. > + if (err) { > + break; > + } > + if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) { > + error_setg(&err, "Invalid multifd_compress option %s", valuestr); > + break; > + } > + p->multifd_compress = compress_type; > + break; > case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: > p->has_xbzrle_cache_size = true; > visit_type_size(v, param, &cache_size, &err); > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 5da1439a8b..7c8e71532f 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = { > .set_default_value = set_default_value_enum, > }; > > +/* --- MultifdCompress --- */ > + > +const PropertyInfo qdev_prop_multifd_compress = { > + .name = "MultifdCompress", > + .description = "multifd_compress values", Similar property declarations list the valid values in .description. > + .enum_table = &MultifdCompress_lookup, > + .get = get_enum, > + .set = set_enum, > + .set_default_value = set_default_value_enum, > +}; > + > /* --- pci address --- */ > > /* > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index b6758c852e..ac452d8f2c 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_tpm; > extern const PropertyInfo qdev_prop_ptr; > extern const PropertyInfo qdev_prop_macaddr; > extern const PropertyInfo qdev_prop_on_off_auto; > +extern const PropertyInfo qdev_prop_multifd_compress; > extern const PropertyInfo qdev_prop_losttickpolicy; > extern const PropertyInfo qdev_prop_blockdev_on_error; > extern const PropertyInfo qdev_prop_bios_chs_trans; > diff --git a/migration/migration.c b/migration/migration.c > index 609e0df5d0..d6f8ef342a 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -82,6 +82,7 @@ > /* The delay time (in ms) between two COLO checkpoints */ > #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100) > #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2 > +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS MULTIFD_COMPRESS_NONE > > /* Background transfer rate for postcopy, 0 means unlimited, note > * that page requests can still exceed this limit. > @@ -769,6 +770,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > params->block_incremental = s->parameters.block_incremental; > params->has_multifd_channels = true; > params->multifd_channels = s->parameters.multifd_channels; > + params->has_multifd_compress = true; > + params->multifd_compress = s->parameters.multifd_compress; > params->has_xbzrle_cache_size = true; > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > params->has_max_postcopy_bandwidth = true; > @@ -1268,6 +1271,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, > if (params->has_multifd_channels) { > dest->multifd_channels = params->multifd_channels; > } > + if (params->has_multifd_compress) { > + dest->multifd_compress = params->multifd_compress; > + } > if (params->has_xbzrle_cache_size) { > dest->xbzrle_cache_size = params->xbzrle_cache_size; > } > @@ -1364,6 +1370,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_multifd_channels) { > s->parameters.multifd_channels = params->multifd_channels; > } > + if (params->has_multifd_compress) { > + s->parameters.multifd_compress = params->multifd_compress; > + } > if (params->has_xbzrle_cache_size) { > s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; > xbzrle_cache_resize(params->xbzrle_cache_size, errp); > @@ -3353,6 +3362,9 @@ void migration_global_dump(Monitor *mon) > #define DEFINE_PROP_MIG_CAP(name, x) \ > DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false) > > +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \ > + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress) > + As long as qdev_prop_multifd_compress is exposed in qdev-properties.h, hiding the macro using it doesn't make sense to me. > static Property migration_properties[] = { > DEFINE_PROP_BOOL("store-global-state", MigrationState, > store_global_state, true), > @@ -3392,6 +3404,9 @@ static Property migration_properties[] = { > DEFINE_PROP_UINT8("multifd-channels", MigrationState, > parameters.multifd_channels, > DEFAULT_MIGRATE_MULTIFD_CHANNELS), > + DEFINE_PROP_MULTIFD_COMPRESS("multifd-compress", MigrationState, > + parameters.multifd_compress, > + DEFAULT_MIGRATE_MULTIFD_COMPRESS), > DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, > parameters.xbzrle_cache_size, > DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), > @@ -3481,6 +3496,7 @@ static void migration_instance_init(Object *obj) > params->has_x_checkpoint_delay = true; > params->has_block_incremental = true; > params->has_multifd_channels = true; > + params->has_multifd_compress = true; > params->has_xbzrle_cache_size = true; > params->has_max_postcopy_bandwidth = true; > params->has_max_cpu_throttle = true; > diff --git a/qapi/common.json b/qapi/common.json > index 99d313ef3b..7248172792 100644 > --- a/qapi/common.json > +++ b/qapi/common.json > @@ -193,3 +193,16 @@ > 'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4', > 'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32', > 'x86_64', 'xtensa', 'xtensaeb' ] } > + > +## > +# @MultifdCompress: > +# > +# An enumeration of multifd compression. > +# > +# @none: no compression. > +# > +# Since: 4.1 > +# > +## > +{ 'enum': 'MultifdCompress', > + 'data': [ 'none' ] } Any particular reason for putting this in common.json? As is, it looks rather migration-specific to me... > diff --git a/qapi/migration.json b/qapi/migration.json > index 9cfbaf8c6c..629795fd30 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -580,6 +580,9 @@ > # @max-cpu-throttle: maximum cpu throttle percentage. > # Defaults to 99. (Since 3.1) > # > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > +# > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > @@ -592,7 +595,7 @@ > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > 'multifd-channels', > 'xbzrle-cache-size', 'max-postcopy-bandwidth', > - 'max-cpu-throttle' ] } > + 'max-cpu-throttle', 'multifd-compress' ] } > > ## > # @MigrateSetParameters: > @@ -680,7 +683,10 @@ > # (Since 3.0) > # > # @max-cpu-throttle: maximum cpu throttle percentage. > -# The default value is 99. (Since 3.1) > +# The default value is 99. (Since 4.0) > +# > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > # > # Since: 2.4 > ## > @@ -707,7 +713,8 @@ > '*multifd-channels': 'int', > '*xbzrle-cache-size': 'size', > '*max-postcopy-bandwidth': 'size', > - '*max-cpu-throttle': 'int' } } > + '*max-cpu-throttle': 'int', > + '*multifd-compress': 'MultifdCompress' } } > > ## > # @migrate-set-parameters: > @@ -817,6 +824,9 @@ > # Defaults to 99. > # (Since 3.1) > # > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -840,7 +850,8 @@ > '*multifd-channels': 'uint8', > '*xbzrle-cache-size': 'size', > '*max-postcopy-bandwidth': 'size', > - '*max-cpu-throttle':'uint8'} } > + '*max-cpu-throttle': 'uint8', > + '*multifd-compress': 'MultifdCompress' } } > > ## > # @query-migrate-parameters: > diff --git a/tests/migration-test.c b/tests/migration-test.c > index 65d5e256a7..8a1ccc2516 100644 > --- a/tests/migration-test.c > +++ b/tests/migration-test.c > @@ -449,7 +449,6 @@ static void migrate_check_parameter_str(QTestState *who, const char *parameter, > g_free(result); > } > > -__attribute__((unused)) > static void migrate_set_parameter_str(QTestState *who, const char *parameter, > const char *value) > { > @@ -1065,7 +1064,7 @@ static void test_precopy_tcp(void) > g_free(uri); > } > > -static void test_multifd_tcp(void) > +static void test_multifd_tcp(const char *method) > { > char *uri; > QTestState *from, *to; > @@ -1087,6 +1086,9 @@ static void test_multifd_tcp(void) > migrate_set_parameter_int(from, "multifd-channels", 2); > migrate_set_parameter_int(to, "multifd-channels", 2); > > + migrate_set_parameter_str(from, "multifd-compress", method); > + migrate_set_parameter_str(to, "multifd-compress", method); > + > migrate_set_capability(from, "multifd", "true"); > migrate_set_capability(to, "multifd", "true"); > /* Wait for the first serial output from the source */ > @@ -1112,6 +1114,11 @@ static void test_multifd_tcp(void) > test_migrate_end(from, to, true); > } > > +static void test_multifd_tcp_none(void) > +{ > + test_multifd_tcp("none"); > +} > + > int main(int argc, char **argv) > { > char template[] = "/tmp/migration-test-XXXXXX"; > @@ -1166,7 +1173,7 @@ int main(int argc, char **argv) > qtest_add_func("/migration/precopy/tcp", test_precopy_tcp); > /* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */ > qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix); > - qtest_add_func("/migration/multifd/tcp", test_multifd_tcp); > + qtest_add_func("/migration/multifd/tcp/none", test_multifd_tcp_none); > > ret = g_test_run();
* Juan Quintela (quintela@redhat.com) wrote: > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > Rename it to NONE > --- > hmp.c | 17 +++++++++++++++++ > hw/core/qdev-properties.c | 11 +++++++++++ > include/hw/qdev-properties.h | 1 + > migration/migration.c | 16 ++++++++++++++++ > qapi/common.json | 13 +++++++++++++ > qapi/migration.json | 19 +++++++++++++++---- > tests/migration-test.c | 13 ++++++++++--- > 7 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/hmp.c b/hmp.c > index 8eec768088..02fbe27757 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -435,6 +435,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "%s: %u\n", > MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), > params->multifd_channels); > + monitor_printf(mon, "%s: %s\n", > + MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESS), > + MultifdCompress_str(params->multifd_compress)); > monitor_printf(mon, "%s: %" PRIu64 "\n", > MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), > params->xbzrle_cache_size); > @@ -1737,6 +1740,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); > uint64_t valuebw = 0; > uint64_t cache_size; > + int compress_type; > Error *err = NULL; > int val, ret; > > @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->has_multifd_channels = true; > visit_type_int(v, param, &p->multifd_channels, &err); > break; > + case MIGRATION_PARAMETER_MULTIFD_COMPRESS: > + p->has_multifd_compress = true; > + visit_type_enum(v, param, &compress_type, > + &MultifdCompress_lookup, &err); > + if (err) { > + break; > + } > + if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) { I still think that needs to be >= rather than > ? Dave > + error_setg(&err, "Invalid multifd_compress option %s", valuestr); > + break; > + } > + p->multifd_compress = compress_type; > + break; > case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: > p->has_xbzrle_cache_size = true; > visit_type_size(v, param, &cache_size, &err); > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 5da1439a8b..7c8e71532f 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = { > .set_default_value = set_default_value_enum, > }; > > +/* --- MultifdCompress --- */ > + > +const PropertyInfo qdev_prop_multifd_compress = { > + .name = "MultifdCompress", > + .description = "multifd_compress values", > + .enum_table = &MultifdCompress_lookup, > + .get = get_enum, > + .set = set_enum, > + .set_default_value = set_default_value_enum, > +}; > + > /* --- pci address --- */ > > /* > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index b6758c852e..ac452d8f2c 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_tpm; > extern const PropertyInfo qdev_prop_ptr; > extern const PropertyInfo qdev_prop_macaddr; > extern const PropertyInfo qdev_prop_on_off_auto; > +extern const PropertyInfo qdev_prop_multifd_compress; > extern const PropertyInfo qdev_prop_losttickpolicy; > extern const PropertyInfo qdev_prop_blockdev_on_error; > extern const PropertyInfo qdev_prop_bios_chs_trans; > diff --git a/migration/migration.c b/migration/migration.c > index 609e0df5d0..d6f8ef342a 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -82,6 +82,7 @@ > /* The delay time (in ms) between two COLO checkpoints */ > #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100) > #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2 > +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS MULTIFD_COMPRESS_NONE > > /* Background transfer rate for postcopy, 0 means unlimited, note > * that page requests can still exceed this limit. > @@ -769,6 +770,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > params->block_incremental = s->parameters.block_incremental; > params->has_multifd_channels = true; > params->multifd_channels = s->parameters.multifd_channels; > + params->has_multifd_compress = true; > + params->multifd_compress = s->parameters.multifd_compress; > params->has_xbzrle_cache_size = true; > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > params->has_max_postcopy_bandwidth = true; > @@ -1268,6 +1271,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, > if (params->has_multifd_channels) { > dest->multifd_channels = params->multifd_channels; > } > + if (params->has_multifd_compress) { > + dest->multifd_compress = params->multifd_compress; > + } > if (params->has_xbzrle_cache_size) { > dest->xbzrle_cache_size = params->xbzrle_cache_size; > } > @@ -1364,6 +1370,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_multifd_channels) { > s->parameters.multifd_channels = params->multifd_channels; > } > + if (params->has_multifd_compress) { > + s->parameters.multifd_compress = params->multifd_compress; > + } > if (params->has_xbzrle_cache_size) { > s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; > xbzrle_cache_resize(params->xbzrle_cache_size, errp); > @@ -3353,6 +3362,9 @@ void migration_global_dump(Monitor *mon) > #define DEFINE_PROP_MIG_CAP(name, x) \ > DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false) > > +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \ > + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress) > + > static Property migration_properties[] = { > DEFINE_PROP_BOOL("store-global-state", MigrationState, > store_global_state, true), > @@ -3392,6 +3404,9 @@ static Property migration_properties[] = { > DEFINE_PROP_UINT8("multifd-channels", MigrationState, > parameters.multifd_channels, > DEFAULT_MIGRATE_MULTIFD_CHANNELS), > + DEFINE_PROP_MULTIFD_COMPRESS("multifd-compress", MigrationState, > + parameters.multifd_compress, > + DEFAULT_MIGRATE_MULTIFD_COMPRESS), > DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, > parameters.xbzrle_cache_size, > DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), > @@ -3481,6 +3496,7 @@ static void migration_instance_init(Object *obj) > params->has_x_checkpoint_delay = true; > params->has_block_incremental = true; > params->has_multifd_channels = true; > + params->has_multifd_compress = true; > params->has_xbzrle_cache_size = true; > params->has_max_postcopy_bandwidth = true; > params->has_max_cpu_throttle = true; > diff --git a/qapi/common.json b/qapi/common.json > index 99d313ef3b..7248172792 100644 > --- a/qapi/common.json > +++ b/qapi/common.json > @@ -193,3 +193,16 @@ > 'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4', > 'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32', > 'x86_64', 'xtensa', 'xtensaeb' ] } > + > +## > +# @MultifdCompress: > +# > +# An enumeration of multifd compression. > +# > +# @none: no compression. > +# > +# Since: 4.1 > +# > +## > +{ 'enum': 'MultifdCompress', > + 'data': [ 'none' ] } > diff --git a/qapi/migration.json b/qapi/migration.json > index 9cfbaf8c6c..629795fd30 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -580,6 +580,9 @@ > # @max-cpu-throttle: maximum cpu throttle percentage. > # Defaults to 99. (Since 3.1) > # > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > +# > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > @@ -592,7 +595,7 @@ > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > 'multifd-channels', > 'xbzrle-cache-size', 'max-postcopy-bandwidth', > - 'max-cpu-throttle' ] } > + 'max-cpu-throttle', 'multifd-compress' ] } > > ## > # @MigrateSetParameters: > @@ -680,7 +683,10 @@ > # (Since 3.0) > # > # @max-cpu-throttle: maximum cpu throttle percentage. > -# The default value is 99. (Since 3.1) > +# The default value is 99. (Since 4.0) > +# > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > # > # Since: 2.4 > ## > @@ -707,7 +713,8 @@ > '*multifd-channels': 'int', > '*xbzrle-cache-size': 'size', > '*max-postcopy-bandwidth': 'size', > - '*max-cpu-throttle': 'int' } } > + '*max-cpu-throttle': 'int', > + '*multifd-compress': 'MultifdCompress' } } > > ## > # @migrate-set-parameters: > @@ -817,6 +824,9 @@ > # Defaults to 99. > # (Since 3.1) > # > +# @multifd-compress: What compression method to use. > +# Defaults to none. (Since 4.1) > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -840,7 +850,8 @@ > '*multifd-channels': 'uint8', > '*xbzrle-cache-size': 'size', > '*max-postcopy-bandwidth': 'size', > - '*max-cpu-throttle':'uint8'} } > + '*max-cpu-throttle': 'uint8', > + '*multifd-compress': 'MultifdCompress' } } > > ## > # @query-migrate-parameters: > diff --git a/tests/migration-test.c b/tests/migration-test.c > index 65d5e256a7..8a1ccc2516 100644 > --- a/tests/migration-test.c > +++ b/tests/migration-test.c > @@ -449,7 +449,6 @@ static void migrate_check_parameter_str(QTestState *who, const char *parameter, > g_free(result); > } > > -__attribute__((unused)) > static void migrate_set_parameter_str(QTestState *who, const char *parameter, > const char *value) > { > @@ -1065,7 +1064,7 @@ static void test_precopy_tcp(void) > g_free(uri); > } > > -static void test_multifd_tcp(void) > +static void test_multifd_tcp(const char *method) > { > char *uri; > QTestState *from, *to; > @@ -1087,6 +1086,9 @@ static void test_multifd_tcp(void) > migrate_set_parameter_int(from, "multifd-channels", 2); > migrate_set_parameter_int(to, "multifd-channels", 2); > > + migrate_set_parameter_str(from, "multifd-compress", method); > + migrate_set_parameter_str(to, "multifd-compress", method); > + > migrate_set_capability(from, "multifd", "true"); > migrate_set_capability(to, "multifd", "true"); > /* Wait for the first serial output from the source */ > @@ -1112,6 +1114,11 @@ static void test_multifd_tcp(void) > test_migrate_end(from, to, true); > } > > +static void test_multifd_tcp_none(void) > +{ > + test_multifd_tcp("none"); > +} > + > int main(int argc, char **argv) > { > char template[] = "/tmp/migration-test-XXXXXX"; > @@ -1166,7 +1173,7 @@ int main(int argc, char **argv) > qtest_add_func("/migration/precopy/tcp", test_precopy_tcp); > /* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */ > qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix); > - qtest_add_func("/migration/multifd/tcp", test_multifd_tcp); > + qtest_add_func("/migration/multifd/tcp/none", test_multifd_tcp_none); > > ret = g_test_run(); > > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> --- >> Rename it to NONE >> @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >> p->has_multifd_channels = true; >> visit_type_int(v, param, &p->multifd_channels, &err); >> break; >> + case MIGRATION_PARAMETER_MULTIFD_COMPRESS: >> + p->has_multifd_compress = true; >> + visit_type_enum(v, param, &compress_type, >> + &MultifdCompress_lookup, &err); > > visit_type_MultifdCompress(), please. done. Interesting that I can #include "qapi/qapi-visit-common.h" but not what I would expect/want: #include "qapi/qapi-visit.h" Perhaps we should remove #include "qapi-visit-target.h" from there? Anyways, independent of this patch. >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index 5da1439a8b..7c8e71532f 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = { >> .set_default_value = set_default_value_enum, >> }; >> >> +/* --- MultifdCompress --- */ >> + >> +const PropertyInfo qdev_prop_multifd_compress = { >> + .name = "MultifdCompress", >> + .description = "multifd_compress values", > > Similar property declarations list the valid values in .description. Fixed, thanks. >> >> +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \ >> + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress) >> + > > As long as qdev_prop_multifd_compress is exposed in qdev-properties.h, > hiding the macro using it doesn't make sense to me. Where do you want it to be? This should only be used here, but if you want it anywhere else, told me where. >> +## >> +# @MultifdCompress: >> +# >> +# An enumeration of multifd compression. >> +# >> +# @none: no compression. >> +# >> +# Since: 4.1 >> +# >> +## >> +{ 'enum': 'MultifdCompress', >> + 'data': [ 'none' ] } > > Any particular reason for putting this in common.json? As is, it looks > rather migration-specific to me... Not sure if with new "qapi" compiler it works, it used to be that it failed if you declared an enum anywhere else. See how I have to put property info into qdev-properties.c instead of any migration file. Thanks, Juan.
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> >>> --- >>> Rename it to NONE > >>> @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >>> p->has_multifd_channels = true; >>> visit_type_int(v, param, &p->multifd_channels, &err); >>> break; >>> + case MIGRATION_PARAMETER_MULTIFD_COMPRESS: >>> + p->has_multifd_compress = true; >>> + visit_type_enum(v, param, &compress_type, >>> + &MultifdCompress_lookup, &err); >> >> visit_type_MultifdCompress(), please. > > done. > > Interesting that I can > > #include "qapi/qapi-visit-common.h" > > but not what I would expect/want: > > #include "qapi/qapi-visit.h" I guess you tried to include it into target-independent code, which doesn't work since we added target conditionals like { 'command': 'rtc-reset-reinjection', 'if': 'defined(TARGET_I386)' } Adding these (merge commit a0430dd8abb) was only possible because the code generated from the QAPI schema mirrors the QAPI schema's modular structure: just like qapi-schema.json includes its sub-modules such as common.json, the generated qapi-visit.h includes the headers generated for its sub-modules such as qapi-visit-common.h. See commit 252dc3105f "qapi: Generate separate .h, .c for each module". The original motivation for this modularization was actually compile time. Before modularization, touching the QAPI schema typically recompiled everything using QAPI, which nowadays means pretty much everything. Modularization let me replace "include all generated QAPI stuff" by "include just the generated QAPI stuff I actually need", for *massive* compile time improvements. Back to target conditionals. Right now, we confine them to sub-module target.json. This means the headers generated for this sub-module, such as qapi-visit-target.h, can only be included into target-dependent code. Since qapi-visit.h includes all sub-modules, it can also only be included there. > Perhaps we should remove > > #include "qapi-visit-target.h" > > from there? No for two reasons: 1. The code generating qapi-visit.h has no idea which sub-modules are target-specific. 2. You shouldn't include qapi-visit.h anyway, it's bad for compile time. > Anyways, independent of this patch. Yes. >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >>> index 5da1439a8b..7c8e71532f 100644 >>> --- a/hw/core/qdev-properties.c >>> +++ b/hw/core/qdev-properties.c >>> @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = { >>> .set_default_value = set_default_value_enum, >>> }; >>> >>> +/* --- MultifdCompress --- */ >>> + >>> +const PropertyInfo qdev_prop_multifd_compress = { >>> + .name = "MultifdCompress", >>> + .description = "multifd_compress values", >> >> Similar property declarations list the valid values in .description. > > Fixed, thanks. > >>> >>> +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \ >>> + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress) >>> + >> >> As long as qdev_prop_multifd_compress is exposed in qdev-properties.h, >> hiding the macro using it doesn't make sense to me. > > Where do you want it to be? This should only be used here, but if you > want it anywhere else, told me where. Put the declaration of qdev_prop_multifd_compress and the macro in the same source file. Simplest: qdev-properties.h. >>> +## >>> +# @MultifdCompress: >>> +# >>> +# An enumeration of multifd compression. >>> +# >>> +# @none: no compression. >>> +# >>> +# Since: 4.1 >>> +# >>> +## >>> +{ 'enum': 'MultifdCompress', >>> + 'data': [ 'none' ] } >> >> Any particular reason for putting this in common.json? As is, it looks >> rather migration-specific to me... > > Not sure if with new "qapi" compiler it works, it used to be that it > failed if you declared an enum anywhere else. See how I have to put > property info into qdev-properties.c instead of any migration file. You can certainly declare enums anywhere, you just have to make sure to include the right headers in the right places, just like for manually written enums in C headers. Rules for modular QAPI schema: 1. If module A uses an entity defined in module B, A must include B Example: since migration.json uses MultifdCompress defined in common.json, migration.json must include common.json (it does). 2. No circular includes, ever. 3. Try to avoid cross-module dependencies Example: migration.json uses MultifdCompress defined in common.json. This is a cross-module dependency. Can we avoid it by moving MultifdCompress into migration.json? The QAPI schema would be just fine with such a move, but C code using the generated headers might be unhappy, because it now has to include qapi-types-migration.h instead of just qapi-types-common.h. My question was: is C code really unhappy? Please find out and tell me.
diff --git a/hmp.c b/hmp.c index 8eec768088..02fbe27757 100644 --- a/hmp.c +++ b/hmp.c @@ -435,6 +435,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: %u\n", MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), params->multifd_channels); + monitor_printf(mon, "%s: %s\n", + MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESS), + MultifdCompress_str(params->multifd_compress)); monitor_printf(mon, "%s: %" PRIu64 "\n", MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), params->xbzrle_cache_size); @@ -1737,6 +1740,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); uint64_t valuebw = 0; uint64_t cache_size; + int compress_type; Error *err = NULL; int val, ret; @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_multifd_channels = true; visit_type_int(v, param, &p->multifd_channels, &err); break; + case MIGRATION_PARAMETER_MULTIFD_COMPRESS: + p->has_multifd_compress = true; + visit_type_enum(v, param, &compress_type, + &MultifdCompress_lookup, &err); + if (err) { + break; + } + if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) { + error_setg(&err, "Invalid multifd_compress option %s", valuestr); + break; + } + p->multifd_compress = compress_type; + break; case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: p->has_xbzrle_cache_size = true; visit_type_size(v, param, &cache_size, &err); diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 5da1439a8b..7c8e71532f 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = { .set_default_value = set_default_value_enum, }; +/* --- MultifdCompress --- */ + +const PropertyInfo qdev_prop_multifd_compress = { + .name = "MultifdCompress", + .description = "multifd_compress values", + .enum_table = &MultifdCompress_lookup, + .get = get_enum, + .set = set_enum, + .set_default_value = set_default_value_enum, +}; + /* --- pci address --- */ /* diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index b6758c852e..ac452d8f2c 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_tpm; extern const PropertyInfo qdev_prop_ptr; extern const PropertyInfo qdev_prop_macaddr; extern const PropertyInfo qdev_prop_on_off_auto; +extern const PropertyInfo qdev_prop_multifd_compress; extern const PropertyInfo qdev_prop_losttickpolicy; extern const PropertyInfo qdev_prop_blockdev_on_error; extern const PropertyInfo qdev_prop_bios_chs_trans; diff --git a/migration/migration.c b/migration/migration.c index 609e0df5d0..d6f8ef342a 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -82,6 +82,7 @@ /* The delay time (in ms) between two COLO checkpoints */ #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100) #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2 +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS MULTIFD_COMPRESS_NONE /* Background transfer rate for postcopy, 0 means unlimited, note * that page requests can still exceed this limit. @@ -769,6 +770,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->block_incremental = s->parameters.block_incremental; params->has_multifd_channels = true; params->multifd_channels = s->parameters.multifd_channels; + params->has_multifd_compress = true; + params->multifd_compress = s->parameters.multifd_compress; params->has_xbzrle_cache_size = true; params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; params->has_max_postcopy_bandwidth = true; @@ -1268,6 +1271,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, if (params->has_multifd_channels) { dest->multifd_channels = params->multifd_channels; } + if (params->has_multifd_compress) { + dest->multifd_compress = params->multifd_compress; + } if (params->has_xbzrle_cache_size) { dest->xbzrle_cache_size = params->xbzrle_cache_size; } @@ -1364,6 +1370,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) if (params->has_multifd_channels) { s->parameters.multifd_channels = params->multifd_channels; } + if (params->has_multifd_compress) { + s->parameters.multifd_compress = params->multifd_compress; + } if (params->has_xbzrle_cache_size) { s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; xbzrle_cache_resize(params->xbzrle_cache_size, errp); @@ -3353,6 +3362,9 @@ void migration_global_dump(Monitor *mon) #define DEFINE_PROP_MIG_CAP(name, x) \ DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false) +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \ + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress) + static Property migration_properties[] = { DEFINE_PROP_BOOL("store-global-state", MigrationState, store_global_state, true), @@ -3392,6 +3404,9 @@ static Property migration_properties[] = { DEFINE_PROP_UINT8("multifd-channels", MigrationState, parameters.multifd_channels, DEFAULT_MIGRATE_MULTIFD_CHANNELS), + DEFINE_PROP_MULTIFD_COMPRESS("multifd-compress", MigrationState, + parameters.multifd_compress, + DEFAULT_MIGRATE_MULTIFD_COMPRESS), DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, parameters.xbzrle_cache_size, DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), @@ -3481,6 +3496,7 @@ static void migration_instance_init(Object *obj) params->has_x_checkpoint_delay = true; params->has_block_incremental = true; params->has_multifd_channels = true; + params->has_multifd_compress = true; params->has_xbzrle_cache_size = true; params->has_max_postcopy_bandwidth = true; params->has_max_cpu_throttle = true; diff --git a/qapi/common.json b/qapi/common.json index 99d313ef3b..7248172792 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -193,3 +193,16 @@ 'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4', 'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32', 'x86_64', 'xtensa', 'xtensaeb' ] } + +## +# @MultifdCompress: +# +# An enumeration of multifd compression. +# +# @none: no compression. +# +# Since: 4.1 +# +## +{ 'enum': 'MultifdCompress', + 'data': [ 'none' ] } diff --git a/qapi/migration.json b/qapi/migration.json index 9cfbaf8c6c..629795fd30 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -580,6 +580,9 @@ # @max-cpu-throttle: maximum cpu throttle percentage. # Defaults to 99. (Since 3.1) # +# @multifd-compress: What compression method to use. +# Defaults to none. (Since 4.1) +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -592,7 +595,7 @@ 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', - 'max-cpu-throttle' ] } + 'max-cpu-throttle', 'multifd-compress' ] } ## # @MigrateSetParameters: @@ -680,7 +683,10 @@ # (Since 3.0) # # @max-cpu-throttle: maximum cpu throttle percentage. -# The default value is 99. (Since 3.1) +# The default value is 99. (Since 4.0) +# +# @multifd-compress: What compression method to use. +# Defaults to none. (Since 4.1) # # Since: 2.4 ## @@ -707,7 +713,8 @@ '*multifd-channels': 'int', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', - '*max-cpu-throttle': 'int' } } + '*max-cpu-throttle': 'int', + '*multifd-compress': 'MultifdCompress' } } ## # @migrate-set-parameters: @@ -817,6 +824,9 @@ # Defaults to 99. # (Since 3.1) # +# @multifd-compress: What compression method to use. +# Defaults to none. (Since 4.1) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -840,7 +850,8 @@ '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', - '*max-cpu-throttle':'uint8'} } + '*max-cpu-throttle': 'uint8', + '*multifd-compress': 'MultifdCompress' } } ## # @query-migrate-parameters: diff --git a/tests/migration-test.c b/tests/migration-test.c index 65d5e256a7..8a1ccc2516 100644 --- a/tests/migration-test.c +++ b/tests/migration-test.c @@ -449,7 +449,6 @@ static void migrate_check_parameter_str(QTestState *who, const char *parameter, g_free(result); } -__attribute__((unused)) static void migrate_set_parameter_str(QTestState *who, const char *parameter, const char *value) { @@ -1065,7 +1064,7 @@ static void test_precopy_tcp(void) g_free(uri); } -static void test_multifd_tcp(void) +static void test_multifd_tcp(const char *method) { char *uri; QTestState *from, *to; @@ -1087,6 +1086,9 @@ static void test_multifd_tcp(void) migrate_set_parameter_int(from, "multifd-channels", 2); migrate_set_parameter_int(to, "multifd-channels", 2); + migrate_set_parameter_str(from, "multifd-compress", method); + migrate_set_parameter_str(to, "multifd-compress", method); + migrate_set_capability(from, "multifd", "true"); migrate_set_capability(to, "multifd", "true"); /* Wait for the first serial output from the source */ @@ -1112,6 +1114,11 @@ static void test_multifd_tcp(void) test_migrate_end(from, to, true); } +static void test_multifd_tcp_none(void) +{ + test_multifd_tcp("none"); +} + int main(int argc, char **argv) { char template[] = "/tmp/migration-test-XXXXXX"; @@ -1166,7 +1173,7 @@ int main(int argc, char **argv) qtest_add_func("/migration/precopy/tcp", test_precopy_tcp); /* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */ qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix); - qtest_add_func("/migration/multifd/tcp", test_multifd_tcp); + qtest_add_func("/migration/multifd/tcp/none", test_multifd_tcp_none); ret = g_test_run();
Signed-off-by: Juan Quintela <quintela@redhat.com> --- Rename it to NONE --- hmp.c | 17 +++++++++++++++++ hw/core/qdev-properties.c | 11 +++++++++++ include/hw/qdev-properties.h | 1 + migration/migration.c | 16 ++++++++++++++++ qapi/common.json | 13 +++++++++++++ qapi/migration.json | 19 +++++++++++++++---- tests/migration-test.c | 13 ++++++++++--- 7 files changed, 83 insertions(+), 7 deletions(-)