[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] migrate/cpu-throttle: Add max-cpu-throttle migr
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH] migrate/cpu-throttle: Add max-cpu-throttle migration parameter |
Date: |
Fri, 3 Aug 2018 09:10:14 +0100 |
User-agent: |
Mutt/1.10.0 (2018-05-17) |
* Li Qiang (address@hidden) wrote:
> 2018-08-02 18:47 GMT+08:00 Dr. David Alan Gilbert <address@hidden>:
>
> > * Li Qiang (address@hidden) 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 <address@hidden>
> >
> > but I do have one comment below which made me think
> >
> > > Signed-off-by: Li Qiang <address@hidden>
> > > ---
> > > 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 / address@hidden / Manchester, UK
> >
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK