| Message ID | 1533128420-2745-1-git-send-email-liq3ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migrate/cpu-throttle: Add max-cpu-throttle migration parameter | expand |
* Li Qiang (liq3ea@gmail.com) wrote: > Currently, the default maximum CPU throttle for migration is > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > performance effect for the guest. We see a lot of packets latency > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > adds a new max-cpu-throttle parameter to limit the CPU throttle. I think this is OK, so Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> but I do have one comment below which made me think > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > hmp.c | 8 ++++++++ > migration/migration.c | 23 ++++++++++++++++++++++- > migration/migration.h | 1 + > migration/ram.c | 4 +++- > qapi/migration.json | 21 ++++++++++++++++++--- > 5 files changed, 52 insertions(+), 5 deletions(-) > > diff --git a/hmp.c b/hmp.c > index 2aafb50e8e..c38e8b1f78 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "%s: %u\n", > MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT), > params->cpu_throttle_increment); > + assert(params->has_max_cpu_throttle); > + monitor_printf(mon, "%s: %u\n", > + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE), > + params->max_cpu_throttle); > assert(params->has_tls_creds); > monitor_printf(mon, "%s: '%s'\n", > MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->has_cpu_throttle_increment = true; > visit_type_int(v, param, &p->cpu_throttle_increment, &err); > break; > + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: > + p->has_max_cpu_throttle = true; > + visit_type_int(v, param, &p->max_cpu_throttle, &err); > + break; > case MIGRATION_PARAMETER_TLS_CREDS: > p->has_tls_creds = true; > p->tls_creds = g_new0(StrOrNull, 1); > diff --git a/migration/migration.c b/migration/migration.c > index b7d9854bda..570da6c0e7 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -71,6 +71,7 @@ > /* Define default autoconverge cpu throttle migration parameters */ > #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 > #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 > +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 > > /* Migration XBZRLE default cache size */ > #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) > @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > params->has_max_postcopy_bandwidth = true; > params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth; > + params->has_max_cpu_throttle = true; > + params->max_cpu_throttle = s->parameters.max_cpu_throttle; > > return params; > } > @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) > return false; > } > > + if (params->has_max_cpu_throttle && > + (params->max_cpu_throttle < params->cpu_throttle_initial || > + params->max_cpu_throttle > 99)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > + "max_cpu_throttle", > + "an integer in the range of cpu_throttle_initial to 99"); > + return false; > + } > + > return true; > } > > @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, > if (params->has_max_postcopy_bandwidth) { > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > } > + if (params->has_max_cpu_throttle) { > + dest->max_cpu_throttle = params->max_cpu_throttle; > + } > } > > static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_max_postcopy_bandwidth) { > s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth; > } > + if (params->has_max_cpu_throttle) { > + s->parameters.max_cpu_throttle = params->max_cpu_throttle; > + } > } > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) > @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_bandwidth(void) > return s->parameters.max_postcopy_bandwidth; > } > > - > bool migrate_use_block(void) > { > MigrationState *s; > @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { > DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, > parameters.max_postcopy_bandwidth, > DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), > + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, > + parameters.max_cpu_throttle, > + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) > params->has_x_multifd_page_count = true; > params->has_xbzrle_cache_size = true; > params->has_max_postcopy_bandwidth = true; > + params->has_max_cpu_throttle = true; > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > diff --git a/migration/migration.h b/migration/migration.h > index 64a7b33735..eb0c04a7b4 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); > > bool migrate_use_block(void); > bool migrate_use_block_incremental(void); > +int migrate_max_cpu_throttle(void); > bool migrate_use_return_path(void); > > bool migrate_use_compression(void); > diff --git a/migration/ram.c b/migration/ram.c > index 24dea2730c..cdd4349177 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) > MigrationState *s = migrate_get_current(); > uint64_t pct_initial = s->parameters.cpu_throttle_initial; > uint64_t pct_icrement = s->parameters.cpu_throttle_increment; > + int pct_max = s->parameters.max_cpu_throttle; > > /* We have not started throttling yet. Let's start it. */ > if (!cpu_throttle_active()) { > cpu_throttle_set(pct_initial); > } else { > /* Throttling already on, just increase the rate */ > - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); > + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + pct_icrement, > + pct_max)); I wondered whether we should just make this change inside cpu_throttle_set and remove the CPU_THROTTLE_PCT_MAX define; but in the end I decided you're code is probably better, because the cpu_throttle_set code is generic, where as this limitation is specific to why we're throttling it for migration. Dave > } > } > > diff --git a/qapi/migration.json b/qapi/migration.json > index 186e8a7303..865064e18c 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -523,6 +523,9 @@ > # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. > # Defaults to 0 (unlimited). In bytes per second. > # (Since 3.0) > +# > +# @max-cpu-throttle: maximum cpu throttle percentage. > +# Defaults to 99. (Since 3.1) > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > @@ -531,7 +534,8 @@ > 'tls-creds', 'tls-hostname', 'max-bandwidth', > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > 'x-multifd-channels', 'x-multifd-page-count', > - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } > + 'xbzrle-cache-size', 'max-postcopy-bandwidth', > + 'max-cpu-throttle' ] } > > ## > # @MigrateSetParameters: > @@ -603,6 +607,10 @@ > # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. > # Defaults to 0 (unlimited). In bytes per second. > # (Since 3.0) > +# > +# @max-cpu-throttle: maximum cpu throttle percentage. > +# The default value is 99. (Since 3.1) > +# > # Since: 2.4 > ## > # TODO either fuse back into MigrationParameters, or make > @@ -622,7 +630,8 @@ > '*x-multifd-channels': 'int', > '*x-multifd-page-count': 'int', > '*xbzrle-cache-size': 'size', > - '*max-postcopy-bandwidth': 'size' } } > + '*max-postcopy-bandwidth': 'size', > + '*max-cpu-throttle': 'int' } } > > ## > # @migrate-set-parameters: > @@ -709,6 +718,11 @@ > # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. > # Defaults to 0 (unlimited). In bytes per second. > # (Since 3.0) > +# > +# @max-cpu-throttle: maximum cpu throttle percentage. > +# Defaults to 99. > +# (Since 3.1) > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -726,7 +740,8 @@ > '*x-multifd-channels': 'uint8', > '*x-multifd-page-count': 'uint32', > '*xbzrle-cache-size': 'size', > - '*max-postcopy-bandwidth': 'size' } } > + '*max-postcopy-bandwidth': 'size', > + '*max-cpu-throttle':'uint8'} } > > ## > # @query-migrate-parameters: > -- > 2.11.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
2018-08-02 18:47 GMT+08:00 Dr. David Alan Gilbert <dgilbert@redhat.com>: > * Li Qiang (liq3ea@gmail.com) wrote: > > Currently, the default maximum CPU throttle for migration is > > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > > performance effect for the guest. We see a lot of packets latency > > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > > adds a new max-cpu-throttle parameter to limit the CPU throttle. > > I think this is OK, so > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > but I do have one comment below which made me think > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > hmp.c | 8 ++++++++ > > migration/migration.c | 23 ++++++++++++++++++++++- > > migration/migration.h | 1 + > > migration/ram.c | 4 +++- > > qapi/migration.json | 21 ++++++++++++++++++--- > > 5 files changed, 52 insertions(+), 5 deletions(-) > > > > diff --git a/hmp.c b/hmp.c > > index 2aafb50e8e..c38e8b1f78 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, > const QDict *qdict) > > monitor_printf(mon, "%s: %u\n", > > MigrationParameter_str(MIGRATION_PARAMETER_CPU_ > THROTTLE_INCREMENT), > > params->cpu_throttle_increment); > > + assert(params->has_max_cpu_throttle); > > + monitor_printf(mon, "%s: %u\n", > > + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_ > THROTTLE), > > + params->max_cpu_throttle); > > assert(params->has_tls_creds); > > monitor_printf(mon, "%s: '%s'\n", > > MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > > @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, > const QDict *qdict) > > p->has_cpu_throttle_increment = true; > > visit_type_int(v, param, &p->cpu_throttle_increment, &err); > > break; > > + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: > > + p->has_max_cpu_throttle = true; > > + visit_type_int(v, param, &p->max_cpu_throttle, &err); > > + break; > > case MIGRATION_PARAMETER_TLS_CREDS: > > p->has_tls_creds = true; > > p->tls_creds = g_new0(StrOrNull, 1); > > diff --git a/migration/migration.c b/migration/migration.c > > index b7d9854bda..570da6c0e7 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -71,6 +71,7 @@ > > /* Define default autoconverge cpu throttle migration parameters */ > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 > > +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 > > > > /* Migration XBZRLE default cache size */ > > #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) > > @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error > **errp) > > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > > params->has_max_postcopy_bandwidth = true; > > params->max_postcopy_bandwidth = s->parameters.max_postcopy_ > bandwidth; > > + params->has_max_cpu_throttle = true; > > + params->max_cpu_throttle = s->parameters.max_cpu_throttle; > > > > return params; > > } > > @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters > *params, Error **errp) > > return false; > > } > > > > + if (params->has_max_cpu_throttle && > > + (params->max_cpu_throttle < params->cpu_throttle_initial || > > + params->max_cpu_throttle > 99)) { > > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > > + "max_cpu_throttle", > > + "an integer in the range of cpu_throttle_initial to > 99"); > > + return false; > > + } > > + > > return true; > > } > > > > @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters > *params, > > if (params->has_max_postcopy_bandwidth) { > > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > > } > > + if (params->has_max_cpu_throttle) { > > + dest->max_cpu_throttle = params->max_cpu_throttle; > > + } > > } > > > > static void migrate_params_apply(MigrateSetParameters *params, Error > **errp) > > @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters > *params, Error **errp) > > if (params->has_max_postcopy_bandwidth) { > > s->parameters.max_postcopy_bandwidth = params->max_postcopy_ > bandwidth; > > } > > + if (params->has_max_cpu_throttle) { > > + s->parameters.max_cpu_throttle = params->max_cpu_throttle; > > + } > > } > > > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error > **errp) > > @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_ > bandwidth(void) > > return s->parameters.max_postcopy_bandwidth; > > } > > > > - > > bool migrate_use_block(void) > > { > > MigrationState *s; > > @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { > > DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, > > parameters.max_postcopy_bandwidth, > > DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), > > + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, > > + parameters.max_cpu_throttle, > > + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), > > > > /* Migration capabilities */ > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) > > params->has_x_multifd_page_count = true; > > params->has_xbzrle_cache_size = true; > > params->has_max_postcopy_bandwidth = true; > > + params->has_max_cpu_throttle = true; > > > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > > diff --git a/migration/migration.h b/migration/migration.h > > index 64a7b33735..eb0c04a7b4 100644 > > --- a/migration/migration.h > > +++ b/migration/migration.h > > @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); > > > > bool migrate_use_block(void); > > bool migrate_use_block_incremental(void); > > +int migrate_max_cpu_throttle(void); > > bool migrate_use_return_path(void); > > > > bool migrate_use_compression(void); > > diff --git a/migration/ram.c b/migration/ram.c > > index 24dea2730c..cdd4349177 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) > > MigrationState *s = migrate_get_current(); > > uint64_t pct_initial = s->parameters.cpu_throttle_initial; > > uint64_t pct_icrement = s->parameters.cpu_throttle_increment; > > + int pct_max = s->parameters.max_cpu_throttle; > > > > /* We have not started throttling yet. Let's start it. */ > > if (!cpu_throttle_active()) { > > cpu_throttle_set(pct_initial); > > } else { > > /* Throttling already on, just increase the rate */ > > - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); > > + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + > pct_icrement, > > + pct_max)); > > I wondered whether we should just make this change inside > cpu_throttle_set and remove the CPU_THROTTLE_PCT_MAX define; but in the > end I decided you're code is probably better, because the > cpu_throttle_set code is generic, where as this limitation is specific > to why we're throttling it for migration. > > Exactly. I also have a more think here. I made the change in ' cpu_throttle_set ' in the qmp version. But here I choice this as it is more clear. Also I noticed the ' cpu_throttle_set' is not used only in 'mig_throttle_guest_down' but also in ui/cocoa.m file(Though I'm not sure what this is for. Thanks, Li Qiang > Dave > > > } > > } > > > > diff --git a/qapi/migration.json b/qapi/migration.json > > index 186e8a7303..865064e18c 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -523,6 +523,9 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# Defaults to 99. (Since 3.1) > > # Since: 2.4 > > ## > > { 'enum': 'MigrationParameter', > > @@ -531,7 +534,8 @@ > > 'tls-creds', 'tls-hostname', 'max-bandwidth', > > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > > 'x-multifd-channels', 'x-multifd-page-count', > > - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } > > + 'xbzrle-cache-size', 'max-postcopy-bandwidth', > > + 'max-cpu-throttle' ] } > > > > ## > > # @MigrateSetParameters: > > @@ -603,6 +607,10 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# The default value is 99. (Since 3.1) > > +# > > # Since: 2.4 > > ## > > # TODO either fuse back into MigrationParameters, or make > > @@ -622,7 +630,8 @@ > > '*x-multifd-channels': 'int', > > '*x-multifd-page-count': 'int', > > '*xbzrle-cache-size': 'size', > > - '*max-postcopy-bandwidth': 'size' } } > > + '*max-postcopy-bandwidth': 'size', > > + '*max-cpu-throttle': 'int' } } > > > > ## > > # @migrate-set-parameters: > > @@ -709,6 +718,11 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# Defaults to 99. > > +# (Since 3.1) > > +# > > # Since: 2.4 > > ## > > { 'struct': 'MigrationParameters', > > @@ -726,7 +740,8 @@ > > '*x-multifd-channels': 'uint8', > > '*x-multifd-page-count': 'uint32', > > '*xbzrle-cache-size': 'size', > > - '*max-postcopy-bandwidth': 'size' } } > > + '*max-postcopy-bandwidth': 'size', > > + '*max-cpu-throttle':'uint8'} } > > > > ## > > # @query-migrate-parameters: > > -- > > 2.11.0 > > > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Li Qiang (liq3ea@gmail.com) wrote: > 2018-08-02 18:47 GMT+08:00 Dr. David Alan Gilbert <dgilbert@redhat.com>: > > > * Li Qiang (liq3ea@gmail.com) wrote: > > > Currently, the default maximum CPU throttle for migration is > > > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > > > performance effect for the guest. We see a lot of packets latency > > > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > > > adds a new max-cpu-throttle parameter to limit the CPU throttle. > > > > I think this is OK, so > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > but I do have one comment below which made me think > > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > --- > > > hmp.c | 8 ++++++++ > > > migration/migration.c | 23 ++++++++++++++++++++++- > > > migration/migration.h | 1 + > > > migration/ram.c | 4 +++- > > > qapi/migration.json | 21 ++++++++++++++++++--- > > > 5 files changed, 52 insertions(+), 5 deletions(-) > > > > > > diff --git a/hmp.c b/hmp.c > > > index 2aafb50e8e..c38e8b1f78 100644 > > > --- a/hmp.c > > > +++ b/hmp.c > > > @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, > > const QDict *qdict) > > > monitor_printf(mon, "%s: %u\n", > > > MigrationParameter_str(MIGRATION_PARAMETER_CPU_ > > THROTTLE_INCREMENT), > > > params->cpu_throttle_increment); > > > + assert(params->has_max_cpu_throttle); > > > + monitor_printf(mon, "%s: %u\n", > > > + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_ > > THROTTLE), > > > + params->max_cpu_throttle); > > > assert(params->has_tls_creds); > > > monitor_printf(mon, "%s: '%s'\n", > > > MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > > > @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, > > const QDict *qdict) > > > p->has_cpu_throttle_increment = true; > > > visit_type_int(v, param, &p->cpu_throttle_increment, &err); > > > break; > > > + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: > > > + p->has_max_cpu_throttle = true; > > > + visit_type_int(v, param, &p->max_cpu_throttle, &err); > > > + break; > > > case MIGRATION_PARAMETER_TLS_CREDS: > > > p->has_tls_creds = true; > > > p->tls_creds = g_new0(StrOrNull, 1); > > > diff --git a/migration/migration.c b/migration/migration.c > > > index b7d9854bda..570da6c0e7 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -71,6 +71,7 @@ > > > /* Define default autoconverge cpu throttle migration parameters */ > > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 > > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 > > > +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 > > > > > > /* Migration XBZRLE default cache size */ > > > #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) > > > @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error > > **errp) > > > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > > > params->has_max_postcopy_bandwidth = true; > > > params->max_postcopy_bandwidth = s->parameters.max_postcopy_ > > bandwidth; > > > + params->has_max_cpu_throttle = true; > > > + params->max_cpu_throttle = s->parameters.max_cpu_throttle; > > > > > > return params; > > > } > > > @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters > > *params, Error **errp) > > > return false; > > > } > > > > > > + if (params->has_max_cpu_throttle && > > > + (params->max_cpu_throttle < params->cpu_throttle_initial || > > > + params->max_cpu_throttle > 99)) { > > > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > > > + "max_cpu_throttle", > > > + "an integer in the range of cpu_throttle_initial to > > 99"); > > > + return false; > > > + } > > > + > > > return true; > > > } > > > > > > @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters > > *params, > > > if (params->has_max_postcopy_bandwidth) { > > > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > > > } > > > + if (params->has_max_cpu_throttle) { > > > + dest->max_cpu_throttle = params->max_cpu_throttle; > > > + } > > > } > > > > > > static void migrate_params_apply(MigrateSetParameters *params, Error > > **errp) > > > @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters > > *params, Error **errp) > > > if (params->has_max_postcopy_bandwidth) { > > > s->parameters.max_postcopy_bandwidth = params->max_postcopy_ > > bandwidth; > > > } > > > + if (params->has_max_cpu_throttle) { > > > + s->parameters.max_cpu_throttle = params->max_cpu_throttle; > > > + } > > > } > > > > > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error > > **errp) > > > @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_ > > bandwidth(void) > > > return s->parameters.max_postcopy_bandwidth; > > > } > > > > > > - > > > bool migrate_use_block(void) > > > { > > > MigrationState *s; > > > @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { > > > DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, > > > parameters.max_postcopy_bandwidth, > > > DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), > > > + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, > > > + parameters.max_cpu_throttle, > > > + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), > > > > > > /* Migration capabilities */ > > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > > @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) > > > params->has_x_multifd_page_count = true; > > > params->has_xbzrle_cache_size = true; > > > params->has_max_postcopy_bandwidth = true; > > > + params->has_max_cpu_throttle = true; > > > > > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > > > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > > > diff --git a/migration/migration.h b/migration/migration.h > > > index 64a7b33735..eb0c04a7b4 100644 > > > --- a/migration/migration.h > > > +++ b/migration/migration.h > > > @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); > > > > > > bool migrate_use_block(void); > > > bool migrate_use_block_incremental(void); > > > +int migrate_max_cpu_throttle(void); > > > bool migrate_use_return_path(void); > > > > > > bool migrate_use_compression(void); > > > diff --git a/migration/ram.c b/migration/ram.c > > > index 24dea2730c..cdd4349177 100644 > > > --- a/migration/ram.c > > > +++ b/migration/ram.c > > > @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) > > > MigrationState *s = migrate_get_current(); > > > uint64_t pct_initial = s->parameters.cpu_throttle_initial; > > > uint64_t pct_icrement = s->parameters.cpu_throttle_increment; > > > + int pct_max = s->parameters.max_cpu_throttle; > > > > > > /* We have not started throttling yet. Let's start it. */ > > > if (!cpu_throttle_active()) { > > > cpu_throttle_set(pct_initial); > > > } else { > > > /* Throttling already on, just increase the rate */ > > > - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); > > > + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + > > pct_icrement, > > > + pct_max)); > > > > I wondered whether we should just make this change inside > > cpu_throttle_set and remove the CPU_THROTTLE_PCT_MAX define; but in the > > end I decided you're code is probably better, because the > > cpu_throttle_set code is generic, where as this limitation is specific > > to why we're throttling it for migration. > > > > > Exactly. > I also have a more think here. I made the change in ' cpu_throttle_set ' in > the qmp version. > But here I choice this as it is more clear. Also I noticed the ' > cpu_throttle_set' is not used only > in 'mig_throttle_guest_down' but also in ui/cocoa.m file(Though I'm not > sure what this is for. Oh hadn't spotted that; it's one of the GUI frontends (for MacOS) and it looks like it just has an option to let you slow the guest down. Dave > Thanks, > Li Qiang > > > > Dave > > > > > } > > > } > > > > > > diff --git a/qapi/migration.json b/qapi/migration.json > > > index 186e8a7303..865064e18c 100644 > > > --- a/qapi/migration.json > > > +++ b/qapi/migration.json > > > @@ -523,6 +523,9 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# Defaults to 99. (Since 3.1) > > > # Since: 2.4 > > > ## > > > { 'enum': 'MigrationParameter', > > > @@ -531,7 +534,8 @@ > > > 'tls-creds', 'tls-hostname', 'max-bandwidth', > > > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > > > 'x-multifd-channels', 'x-multifd-page-count', > > > - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } > > > + 'xbzrle-cache-size', 'max-postcopy-bandwidth', > > > + 'max-cpu-throttle' ] } > > > > > > ## > > > # @MigrateSetParameters: > > > @@ -603,6 +607,10 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# The default value is 99. (Since 3.1) > > > +# > > > # Since: 2.4 > > > ## > > > # TODO either fuse back into MigrationParameters, or make > > > @@ -622,7 +630,8 @@ > > > '*x-multifd-channels': 'int', > > > '*x-multifd-page-count': 'int', > > > '*xbzrle-cache-size': 'size', > > > - '*max-postcopy-bandwidth': 'size' } } > > > + '*max-postcopy-bandwidth': 'size', > > > + '*max-cpu-throttle': 'int' } } > > > > > > ## > > > # @migrate-set-parameters: > > > @@ -709,6 +718,11 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# Defaults to 99. > > > +# (Since 3.1) > > > +# > > > # Since: 2.4 > > > ## > > > { 'struct': 'MigrationParameters', > > > @@ -726,7 +740,8 @@ > > > '*x-multifd-channels': 'uint8', > > > '*x-multifd-page-count': 'uint32', > > > '*xbzrle-cache-size': 'size', > > > - '*max-postcopy-bandwidth': 'size' } } > > > + '*max-postcopy-bandwidth': 'size', > > > + '*max-cpu-throttle':'uint8'} } > > > > > > ## > > > # @query-migrate-parameters: > > > -- > > > 2.11.0 > > > > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hello Dave, What's status of this patch, Do you gonna merge it? Thanks, Li Qiang 2018-08-02 18:47 GMT+08:00 Dr. David Alan Gilbert <dgilbert@redhat.com>: > * Li Qiang (liq3ea@gmail.com) wrote: > > Currently, the default maximum CPU throttle for migration is > > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > > performance effect for the guest. We see a lot of packets latency > > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > > adds a new max-cpu-throttle parameter to limit the CPU throttle. > > I think this is OK, so > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > but I do have one comment below which made me think > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > hmp.c | 8 ++++++++ > > migration/migration.c | 23 ++++++++++++++++++++++- > > migration/migration.h | 1 + > > migration/ram.c | 4 +++- > > qapi/migration.json | 21 ++++++++++++++++++--- > > 5 files changed, 52 insertions(+), 5 deletions(-) > > > > diff --git a/hmp.c b/hmp.c > > index 2aafb50e8e..c38e8b1f78 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, > const QDict *qdict) > > monitor_printf(mon, "%s: %u\n", > > MigrationParameter_str(MIGRATION_PARAMETER_CPU_ > THROTTLE_INCREMENT), > > params->cpu_throttle_increment); > > + assert(params->has_max_cpu_throttle); > > + monitor_printf(mon, "%s: %u\n", > > + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_ > THROTTLE), > > + params->max_cpu_throttle); > > assert(params->has_tls_creds); > > monitor_printf(mon, "%s: '%s'\n", > > MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > > @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, > const QDict *qdict) > > p->has_cpu_throttle_increment = true; > > visit_type_int(v, param, &p->cpu_throttle_increment, &err); > > break; > > + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: > > + p->has_max_cpu_throttle = true; > > + visit_type_int(v, param, &p->max_cpu_throttle, &err); > > + break; > > case MIGRATION_PARAMETER_TLS_CREDS: > > p->has_tls_creds = true; > > p->tls_creds = g_new0(StrOrNull, 1); > > diff --git a/migration/migration.c b/migration/migration.c > > index b7d9854bda..570da6c0e7 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -71,6 +71,7 @@ > > /* Define default autoconverge cpu throttle migration parameters */ > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 > > +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 > > > > /* Migration XBZRLE default cache size */ > > #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) > > @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error > **errp) > > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > > params->has_max_postcopy_bandwidth = true; > > params->max_postcopy_bandwidth = s->parameters.max_postcopy_ > bandwidth; > > + params->has_max_cpu_throttle = true; > > + params->max_cpu_throttle = s->parameters.max_cpu_throttle; > > > > return params; > > } > > @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters > *params, Error **errp) > > return false; > > } > > > > + if (params->has_max_cpu_throttle && > > + (params->max_cpu_throttle < params->cpu_throttle_initial || > > + params->max_cpu_throttle > 99)) { > > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > > + "max_cpu_throttle", > > + "an integer in the range of cpu_throttle_initial to > 99"); > > + return false; > > + } > > + > > return true; > > } > > > > @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters > *params, > > if (params->has_max_postcopy_bandwidth) { > > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > > } > > + if (params->has_max_cpu_throttle) { > > + dest->max_cpu_throttle = params->max_cpu_throttle; > > + } > > } > > > > static void migrate_params_apply(MigrateSetParameters *params, Error > **errp) > > @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters > *params, Error **errp) > > if (params->has_max_postcopy_bandwidth) { > > s->parameters.max_postcopy_bandwidth = params->max_postcopy_ > bandwidth; > > } > > + if (params->has_max_cpu_throttle) { > > + s->parameters.max_cpu_throttle = params->max_cpu_throttle; > > + } > > } > > > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error > **errp) > > @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_ > bandwidth(void) > > return s->parameters.max_postcopy_bandwidth; > > } > > > > - > > bool migrate_use_block(void) > > { > > MigrationState *s; > > @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { > > DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, > > parameters.max_postcopy_bandwidth, > > DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), > > + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, > > + parameters.max_cpu_throttle, > > + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), > > > > /* Migration capabilities */ > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) > > params->has_x_multifd_page_count = true; > > params->has_xbzrle_cache_size = true; > > params->has_max_postcopy_bandwidth = true; > > + params->has_max_cpu_throttle = true; > > > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > > diff --git a/migration/migration.h b/migration/migration.h > > index 64a7b33735..eb0c04a7b4 100644 > > --- a/migration/migration.h > > +++ b/migration/migration.h > > @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); > > > > bool migrate_use_block(void); > > bool migrate_use_block_incremental(void); > > +int migrate_max_cpu_throttle(void); > > bool migrate_use_return_path(void); > > > > bool migrate_use_compression(void); > > diff --git a/migration/ram.c b/migration/ram.c > > index 24dea2730c..cdd4349177 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) > > MigrationState *s = migrate_get_current(); > > uint64_t pct_initial = s->parameters.cpu_throttle_initial; > > uint64_t pct_icrement = s->parameters.cpu_throttle_increment; > > + int pct_max = s->parameters.max_cpu_throttle; > > > > /* We have not started throttling yet. Let's start it. */ > > if (!cpu_throttle_active()) { > > cpu_throttle_set(pct_initial); > > } else { > > /* Throttling already on, just increase the rate */ > > - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); > > + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + > pct_icrement, > > + pct_max)); > > I wondered whether we should just make this change inside > cpu_throttle_set and remove the CPU_THROTTLE_PCT_MAX define; but in the > end I decided you're code is probably better, because the > cpu_throttle_set code is generic, where as this limitation is specific > to why we're throttling it for migration. > > Dave > > > } > > } > > > > diff --git a/qapi/migration.json b/qapi/migration.json > > index 186e8a7303..865064e18c 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -523,6 +523,9 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# Defaults to 99. (Since 3.1) > > # Since: 2.4 > > ## > > { 'enum': 'MigrationParameter', > > @@ -531,7 +534,8 @@ > > 'tls-creds', 'tls-hostname', 'max-bandwidth', > > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > > 'x-multifd-channels', 'x-multifd-page-count', > > - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } > > + 'xbzrle-cache-size', 'max-postcopy-bandwidth', > > + 'max-cpu-throttle' ] } > > > > ## > > # @MigrateSetParameters: > > @@ -603,6 +607,10 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# The default value is 99. (Since 3.1) > > +# > > # Since: 2.4 > > ## > > # TODO either fuse back into MigrationParameters, or make > > @@ -622,7 +630,8 @@ > > '*x-multifd-channels': 'int', > > '*x-multifd-page-count': 'int', > > '*xbzrle-cache-size': 'size', > > - '*max-postcopy-bandwidth': 'size' } } > > + '*max-postcopy-bandwidth': 'size', > > + '*max-cpu-throttle': 'int' } } > > > > ## > > # @migrate-set-parameters: > > @@ -709,6 +718,11 @@ > > # @max-postcopy-bandwidth: Background transfer bandwidth during > postcopy. > > # Defaults to 0 (unlimited). In bytes per second. > > # (Since 3.0) > > +# > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > +# Defaults to 99. > > +# (Since 3.1) > > +# > > # Since: 2.4 > > ## > > { 'struct': 'MigrationParameters', > > @@ -726,7 +740,8 @@ > > '*x-multifd-channels': 'uint8', > > '*x-multifd-page-count': 'uint32', > > '*xbzrle-cache-size': 'size', > > - '*max-postcopy-bandwidth': 'size' } } > > + '*max-postcopy-bandwidth': 'size', > > + '*max-cpu-throttle':'uint8'} } > > > > ## > > # @query-migrate-parameters: > > -- > > 2.11.0 > > > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Li Qiang (liq3ea@gmail.com) wrote: > Hello Dave, > > What's status of this patch, Do you gonna merge it? Yes, once 3.1 opens. Dave > Thanks, > Li Qiang > > > 2018-08-02 18:47 GMT+08:00 Dr. David Alan Gilbert <dgilbert@redhat.com>: > > > * Li Qiang (liq3ea@gmail.com) wrote: > > > Currently, the default maximum CPU throttle for migration is > > > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > > > performance effect for the guest. We see a lot of packets latency > > > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > > > adds a new max-cpu-throttle parameter to limit the CPU throttle. > > > > I think this is OK, so > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > but I do have one comment below which made me think > > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > --- > > > hmp.c | 8 ++++++++ > > > migration/migration.c | 23 ++++++++++++++++++++++- > > > migration/migration.h | 1 + > > > migration/ram.c | 4 +++- > > > qapi/migration.json | 21 ++++++++++++++++++--- > > > 5 files changed, 52 insertions(+), 5 deletions(-) > > > > > > diff --git a/hmp.c b/hmp.c > > > index 2aafb50e8e..c38e8b1f78 100644 > > > --- a/hmp.c > > > +++ b/hmp.c > > > @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, > > const QDict *qdict) > > > monitor_printf(mon, "%s: %u\n", > > > MigrationParameter_str(MIGRATION_PARAMETER_CPU_ > > THROTTLE_INCREMENT), > > > params->cpu_throttle_increment); > > > + assert(params->has_max_cpu_throttle); > > > + monitor_printf(mon, "%s: %u\n", > > > + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_ > > THROTTLE), > > > + params->max_cpu_throttle); > > > assert(params->has_tls_creds); > > > monitor_printf(mon, "%s: '%s'\n", > > > MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > > > @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, > > const QDict *qdict) > > > p->has_cpu_throttle_increment = true; > > > visit_type_int(v, param, &p->cpu_throttle_increment, &err); > > > break; > > > + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: > > > + p->has_max_cpu_throttle = true; > > > + visit_type_int(v, param, &p->max_cpu_throttle, &err); > > > + break; > > > case MIGRATION_PARAMETER_TLS_CREDS: > > > p->has_tls_creds = true; > > > p->tls_creds = g_new0(StrOrNull, 1); > > > diff --git a/migration/migration.c b/migration/migration.c > > > index b7d9854bda..570da6c0e7 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -71,6 +71,7 @@ > > > /* Define default autoconverge cpu throttle migration parameters */ > > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 > > > #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 > > > +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 > > > > > > /* Migration XBZRLE default cache size */ > > > #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) > > > @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error > > **errp) > > > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > > > params->has_max_postcopy_bandwidth = true; > > > params->max_postcopy_bandwidth = s->parameters.max_postcopy_ > > bandwidth; > > > + params->has_max_cpu_throttle = true; > > > + params->max_cpu_throttle = s->parameters.max_cpu_throttle; > > > > > > return params; > > > } > > > @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters > > *params, Error **errp) > > > return false; > > > } > > > > > > + if (params->has_max_cpu_throttle && > > > + (params->max_cpu_throttle < params->cpu_throttle_initial || > > > + params->max_cpu_throttle > 99)) { > > > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > > > + "max_cpu_throttle", > > > + "an integer in the range of cpu_throttle_initial to > > 99"); > > > + return false; > > > + } > > > + > > > return true; > > > } > > > > > > @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters > > *params, > > > if (params->has_max_postcopy_bandwidth) { > > > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > > > } > > > + if (params->has_max_cpu_throttle) { > > > + dest->max_cpu_throttle = params->max_cpu_throttle; > > > + } > > > } > > > > > > static void migrate_params_apply(MigrateSetParameters *params, Error > > **errp) > > > @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters > > *params, Error **errp) > > > if (params->has_max_postcopy_bandwidth) { > > > s->parameters.max_postcopy_bandwidth = params->max_postcopy_ > > bandwidth; > > > } > > > + if (params->has_max_cpu_throttle) { > > > + s->parameters.max_cpu_throttle = params->max_cpu_throttle; > > > + } > > > } > > > > > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error > > **errp) > > > @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_ > > bandwidth(void) > > > return s->parameters.max_postcopy_bandwidth; > > > } > > > > > > - > > > bool migrate_use_block(void) > > > { > > > MigrationState *s; > > > @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { > > > DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, > > > parameters.max_postcopy_bandwidth, > > > DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), > > > + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, > > > + parameters.max_cpu_throttle, > > > + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), > > > > > > /* Migration capabilities */ > > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > > @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) > > > params->has_x_multifd_page_count = true; > > > params->has_xbzrle_cache_size = true; > > > params->has_max_postcopy_bandwidth = true; > > > + params->has_max_cpu_throttle = true; > > > > > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > > > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > > > diff --git a/migration/migration.h b/migration/migration.h > > > index 64a7b33735..eb0c04a7b4 100644 > > > --- a/migration/migration.h > > > +++ b/migration/migration.h > > > @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); > > > > > > bool migrate_use_block(void); > > > bool migrate_use_block_incremental(void); > > > +int migrate_max_cpu_throttle(void); > > > bool migrate_use_return_path(void); > > > > > > bool migrate_use_compression(void); > > > diff --git a/migration/ram.c b/migration/ram.c > > > index 24dea2730c..cdd4349177 100644 > > > --- a/migration/ram.c > > > +++ b/migration/ram.c > > > @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) > > > MigrationState *s = migrate_get_current(); > > > uint64_t pct_initial = s->parameters.cpu_throttle_initial; > > > uint64_t pct_icrement = s->parameters.cpu_throttle_increment; > > > + int pct_max = s->parameters.max_cpu_throttle; > > > > > > /* We have not started throttling yet. Let's start it. */ > > > if (!cpu_throttle_active()) { > > > cpu_throttle_set(pct_initial); > > > } else { > > > /* Throttling already on, just increase the rate */ > > > - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); > > > + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + > > pct_icrement, > > > + pct_max)); > > > > I wondered whether we should just make this change inside > > cpu_throttle_set and remove the CPU_THROTTLE_PCT_MAX define; but in the > > end I decided you're code is probably better, because the > > cpu_throttle_set code is generic, where as this limitation is specific > > to why we're throttling it for migration. > > > > Dave > > > > > } > > > } > > > > > > diff --git a/qapi/migration.json b/qapi/migration.json > > > index 186e8a7303..865064e18c 100644 > > > --- a/qapi/migration.json > > > +++ b/qapi/migration.json > > > @@ -523,6 +523,9 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# Defaults to 99. (Since 3.1) > > > # Since: 2.4 > > > ## > > > { 'enum': 'MigrationParameter', > > > @@ -531,7 +534,8 @@ > > > 'tls-creds', 'tls-hostname', 'max-bandwidth', > > > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > > > 'x-multifd-channels', 'x-multifd-page-count', > > > - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } > > > + 'xbzrle-cache-size', 'max-postcopy-bandwidth', > > > + 'max-cpu-throttle' ] } > > > > > > ## > > > # @MigrateSetParameters: > > > @@ -603,6 +607,10 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# The default value is 99. (Since 3.1) > > > +# > > > # Since: 2.4 > > > ## > > > # TODO either fuse back into MigrationParameters, or make > > > @@ -622,7 +630,8 @@ > > > '*x-multifd-channels': 'int', > > > '*x-multifd-page-count': 'int', > > > '*xbzrle-cache-size': 'size', > > > - '*max-postcopy-bandwidth': 'size' } } > > > + '*max-postcopy-bandwidth': 'size', > > > + '*max-cpu-throttle': 'int' } } > > > > > > ## > > > # @migrate-set-parameters: > > > @@ -709,6 +718,11 @@ > > > # @max-postcopy-bandwidth: Background transfer bandwidth during > > postcopy. > > > # Defaults to 0 (unlimited). In bytes per second. > > > # (Since 3.0) > > > +# > > > +# @max-cpu-throttle: maximum cpu throttle percentage. > > > +# Defaults to 99. > > > +# (Since 3.1) > > > +# > > > # Since: 2.4 > > > ## > > > { 'struct': 'MigrationParameters', > > > @@ -726,7 +740,8 @@ > > > '*x-multifd-channels': 'uint8', > > > '*x-multifd-page-count': 'uint32', > > > '*xbzrle-cache-size': 'size', > > > - '*max-postcopy-bandwidth': 'size' } } > > > + '*max-postcopy-bandwidth': 'size', > > > + '*max-cpu-throttle':'uint8'} } > > > > > > ## > > > # @query-migrate-parameters: > > > -- > > > 2.11.0 > > > > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Li Qiang <liq3ea@gmail.com> wrote: > Currently, the default maximum CPU throttle for migration is > 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable > performance effect for the guest. We see a lot of packets latency > exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set > adds a new max-cpu-throttle parameter to limit the CPU throttle. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/hmp.c b/hmp.c index 2aafb50e8e..c38e8b1f78 100644 --- a/hmp.c +++ b/hmp.c @@ -339,6 +339,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: %u\n", MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT), params->cpu_throttle_increment); + assert(params->has_max_cpu_throttle); + monitor_printf(mon, "%s: %u\n", + MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE), + params->max_cpu_throttle); assert(params->has_tls_creds); monitor_printf(mon, "%s: '%s'\n", MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), @@ -1635,6 +1639,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_cpu_throttle_increment = true; visit_type_int(v, param, &p->cpu_throttle_increment, &err); break; + case MIGRATION_PARAMETER_MAX_CPU_THROTTLE: + p->has_max_cpu_throttle = true; + visit_type_int(v, param, &p->max_cpu_throttle, &err); + break; case MIGRATION_PARAMETER_TLS_CREDS: p->has_tls_creds = true; p->tls_creds = g_new0(StrOrNull, 1); diff --git a/migration/migration.c b/migration/migration.c index b7d9854bda..570da6c0e7 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -71,6 +71,7 @@ /* Define default autoconverge cpu throttle migration parameters */ #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10 +#define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99 /* Migration XBZRLE default cache size */ #define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024) @@ -697,6 +698,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; params->has_max_postcopy_bandwidth = true; params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth; + params->has_max_cpu_throttle = true; + params->max_cpu_throttle = s->parameters.max_cpu_throttle; return params; } @@ -1043,6 +1046,15 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } + if (params->has_max_cpu_throttle && + (params->max_cpu_throttle < params->cpu_throttle_initial || + params->max_cpu_throttle > 99)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + "max_cpu_throttle", + "an integer in the range of cpu_throttle_initial to 99"); + return false; + } + return true; } @@ -1110,6 +1122,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, if (params->has_max_postcopy_bandwidth) { dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; } + if (params->has_max_cpu_throttle) { + dest->max_cpu_throttle = params->max_cpu_throttle; + } } static void migrate_params_apply(MigrateSetParameters *params, Error **errp) @@ -1185,6 +1200,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) if (params->has_max_postcopy_bandwidth) { s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth; } + if (params->has_max_cpu_throttle) { + s->parameters.max_cpu_throttle = params->max_cpu_throttle; + } } void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) @@ -1962,7 +1980,6 @@ static int64_t migrate_max_postcopy_bandwidth(void) return s->parameters.max_postcopy_bandwidth; } - bool migrate_use_block(void) { MigrationState *s; @@ -3160,6 +3177,9 @@ static Property migration_properties[] = { DEFINE_PROP_SIZE("max-postcopy-bandwidth", MigrationState, parameters.max_postcopy_bandwidth, DEFAULT_MIGRATE_MAX_POSTCOPY_BANDWIDTH), + DEFINE_PROP_UINT8("max-cpu-throttle", MigrationState, + parameters.max_cpu_throttle, + DEFAULT_MIGRATE_MAX_CPU_THROTTLE), /* Migration capabilities */ DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), @@ -3230,6 +3250,7 @@ static void migration_instance_init(Object *obj) params->has_x_multifd_page_count = true; params->has_xbzrle_cache_size = true; params->has_max_postcopy_bandwidth = true; + params->has_max_cpu_throttle = true; qemu_sem_init(&ms->postcopy_pause_sem, 0); qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); diff --git a/migration/migration.h b/migration/migration.h index 64a7b33735..eb0c04a7b4 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -266,6 +266,7 @@ bool migrate_colo_enabled(void); bool migrate_use_block(void); bool migrate_use_block_incremental(void); +int migrate_max_cpu_throttle(void); bool migrate_use_return_path(void); bool migrate_use_compression(void); diff --git a/migration/ram.c b/migration/ram.c index 24dea2730c..cdd4349177 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1390,13 +1390,15 @@ static void mig_throttle_guest_down(void) MigrationState *s = migrate_get_current(); uint64_t pct_initial = s->parameters.cpu_throttle_initial; uint64_t pct_icrement = s->parameters.cpu_throttle_increment; + int pct_max = s->parameters.max_cpu_throttle; /* We have not started throttling yet. Let's start it. */ if (!cpu_throttle_active()) { cpu_throttle_set(pct_initial); } else { /* Throttling already on, just increase the rate */ - cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement); + cpu_throttle_set(MIN(cpu_throttle_get_percentage() + pct_icrement, + pct_max)); } } diff --git a/qapi/migration.json b/qapi/migration.json index 186e8a7303..865064e18c 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -523,6 +523,9 @@ # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. # Defaults to 0 (unlimited). In bytes per second. # (Since 3.0) +# +# @max-cpu-throttle: maximum cpu throttle percentage. +# Defaults to 99. (Since 3.1) # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -531,7 +534,8 @@ 'tls-creds', 'tls-hostname', 'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', 'x-multifd-channels', 'x-multifd-page-count', - 'xbzrle-cache-size', 'max-postcopy-bandwidth' ] } + 'xbzrle-cache-size', 'max-postcopy-bandwidth', + 'max-cpu-throttle' ] } ## # @MigrateSetParameters: @@ -603,6 +607,10 @@ # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. # Defaults to 0 (unlimited). In bytes per second. # (Since 3.0) +# +# @max-cpu-throttle: maximum cpu throttle percentage. +# The default value is 99. (Since 3.1) +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -622,7 +630,8 @@ '*x-multifd-channels': 'int', '*x-multifd-page-count': 'int', '*xbzrle-cache-size': 'size', - '*max-postcopy-bandwidth': 'size' } } + '*max-postcopy-bandwidth': 'size', + '*max-cpu-throttle': 'int' } } ## # @migrate-set-parameters: @@ -709,6 +718,11 @@ # @max-postcopy-bandwidth: Background transfer bandwidth during postcopy. # Defaults to 0 (unlimited). In bytes per second. # (Since 3.0) +# +# @max-cpu-throttle: maximum cpu throttle percentage. +# Defaults to 99. +# (Since 3.1) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -726,7 +740,8 @@ '*x-multifd-channels': 'uint8', '*x-multifd-page-count': 'uint32', '*xbzrle-cache-size': 'size', - '*max-postcopy-bandwidth': 'size' } } + '*max-postcopy-bandwidth': 'size', + '*max-cpu-throttle':'uint8'} } ## # @query-migrate-parameters:
Currently, the default maximum CPU throttle for migration is 99(CPU_THROTTLE_PCT_MAX). This is too big and can make a remarkable performance effect for the guest. We see a lot of packets latency exceed 500ms when the CPU_THROTTLE_PCT_MAX reached. This patch set adds a new max-cpu-throttle parameter to limit the CPU throttle. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- hmp.c | 8 ++++++++ migration/migration.c | 23 ++++++++++++++++++++++- migration/migration.h | 1 + migration/ram.c | 4 +++- qapi/migration.json | 21 ++++++++++++++++++--- 5 files changed, 52 insertions(+), 5 deletions(-)