| Message ID | 1533026124-6740-3-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a new migrate_set_max_cpu_throttle qmp command | expand |
* Li Qiang (liq3ea@gmail.com) wrote: > The default max cpu throttle is 99, this is too big that may > influence the guest loads. Add a qmp to config it can make it > more flexible. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> This should be done as a migration parameter rather than a new command. For example, follow the cpu-throttle-increment parameter; and this should work just like it. Dave > --- > cpus.c | 5 +++++ > include/qom/cpu.h | 8 ++++++++ > migration/migration.c | 10 ++++++++++ > qapi/migration.json | 13 +++++++++++++ > 4 files changed, 36 insertions(+) > > diff --git a/cpus.c b/cpus.c > index 6569c73d24..0505477086 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -765,6 +765,11 @@ void cpu_throttle_set(int new_throttle_pct) > CPU_THROTTLE_TIMESLICE_NS); > } > > +void set_cpu_max_throttle(int64_t max_pct) > +{ > + throttle_percentage_max = max_pct; > +} > + > void cpu_throttle_stop(void) > { > atomic_set(&throttle_percentage, 0); > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index bd796579ee..4328d2e8ad 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -816,6 +816,14 @@ CPUState *cpu_by_arch_id(int64_t id); > void cpu_throttle_set(int new_throttle_pct); > > /** > + * set_cpu_max_throttle: > + * @max_pct: the max percent of sleep time. Valid range is 1 to 99. > + * > + * Set the max throttle percentage. > + */ > +void set_cpu_max_throttle(int64_t max_pct); > + > +/** > * cpu_throttle_stop: > * > * Stops the vcpu throttling started by cpu_throttle_set. > diff --git a/migration/migration.c b/migration/migration.c > index b7d9854bda..9c7a6f7477 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1774,6 +1774,16 @@ void qmp_migrate_set_speed(int64_t value, Error **errp) > qmp_migrate_set_parameters(&p, errp); > } > > +void qmp_migrate_set_max_cpu_throttle(int64_t value, Error **errp) > +{ > + if (value > 99 || value < 1) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, " max cpu throttle", > + "not in 1 between 99"); > + return; > + } > + set_cpu_max_throttle(value); > +} > + > void qmp_migrate_set_downtime(double value, Error **errp) > { > if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { > diff --git a/qapi/migration.json b/qapi/migration.json > index 186e8a7303..2ef3ddfe43 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -997,6 +997,19 @@ > { 'command': 'migrate_set_speed', 'data': {'value': 'int'} } > > ## > +# @migrate_set_max_cpu_throttle: > +# > +# Set maximum cpu throttle for migration. > +# > +# @value: maximum cpu throttle. > +# > +# Returns: nothing on success > +# > +# Since: 2.12 > +## > +{ 'command': 'migrate_set_max_cpu_throttle', 'data': {'value': 'int'} } > + > +## > # @migrate-set-cache-size: > # > # Set cache size to be used by XBZRLE migration > -- > 2.11.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/31/2018 06:29 AM, Dr. David Alan Gilbert wrote: > * Li Qiang (liq3ea@gmail.com) wrote: >> The default max cpu throttle is 99, this is too big that may >> influence the guest loads. Add a qmp to config it can make it >> more flexible. >> >> Signed-off-by: Li Qiang <liq3ea@gmail.com> > > This should be done as a migration parameter rather than a new command. > For example, follow the cpu-throttle-increment parameter; and this > should work just like it. 100% agreed - extend existing commands, instead of inventing a new one. But in the future, if you do have a valid reason to add a new command... >> +++ b/qapi/migration.json >> @@ -997,6 +997,19 @@ >> { 'command': 'migrate_set_speed', 'data': {'value': 'int'} } >> >> ## >> +# @migrate_set_max_cpu_throttle: >> +# >> +# Set maximum cpu throttle for migration. >> +# >> +# @value: maximum cpu throttle. >> +# >> +# Returns: nothing on success >> +# >> +# Since: 2.12 You've missed 2.12 by a long shot; you've also missed freeze for 3.0. The earliest you can introduce this is 3.1 (which does need to be documented even if you properly add this as a new parameter to the existing migration parameter commands). >> +## >> +{ 'command': 'migrate_set_max_cpu_throttle', 'data': {'value': 'int'} } ...new commands should favor '-' rather than '_' for separation between words. And your documentation should mention which unit the throttle is in (bytes per second?)
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Li Qiang (liq3ea@gmail.com) wrote: >> The default max cpu throttle is 99, this is too big that may >> influence the guest loads. Add a qmp to config it can make it >> more flexible. >> >> Signed-off-by: Li Qiang <liq3ea@gmail.com> > > This should be done as a migration parameter rather than a new command. > For example, follow the cpu-throttle-increment parameter; and this > should work just like it. I was about to comment this one. migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, query-migrate-cache-size are marked as deprecated. Any way that we could have make more clear that one should use migrate_set/get_parameter/capability? Thanks, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Li Qiang (liq3ea@gmail.com) wrote: > >> The default max cpu throttle is 99, this is too big that may > >> influence the guest loads. Add a qmp to config it can make it > >> more flexible. > >> > >> Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > > This should be done as a migration parameter rather than a new command. > > For example, follow the cpu-throttle-increment parameter; and this > > should work just like it. > > I was about to comment this one. > > migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, > query-migrate-cache-size are marked as deprecated. Any way that we > could have make more clear that one should use > migrate_set/get_parameter/capability? A big hairy comment? Dave > Thanks, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Jul 31, 2018 at 04:48:35PM +0200, Juan Quintela wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Li Qiang (liq3ea@gmail.com) wrote: > >> The default max cpu throttle is 99, this is too big that may > >> influence the guest loads. Add a qmp to config it can make it > >> more flexible. > >> > >> Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > > This should be done as a migration parameter rather than a new command. > > For example, follow the cpu-throttle-increment parameter; and this > > should work just like it. > > I was about to comment this one. > > migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, > query-migrate-cache-size are marked as deprecated. Any way that we > could have make more clear that one should use > migrate_set/get_parameter/capability? Where are they marked as deprecated ? They're not included in our official list of deprecated features, so per our deprecation policy, they are still considered supported. https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features To deprecate something it needs to be added to qemu-deprecated.texi, and ideally also made to print a message to stderr when triggered. Regards, Daniel
2018-07-31 23:50 GMT+08:00 Daniel P. Berrangé <berrange@redhat.com>: > On Tue, Jul 31, 2018 at 04:48:35PM +0200, Juan Quintela wrote: > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > * Li Qiang (liq3ea@gmail.com) wrote: > > >> The default max cpu throttle is 99, this is too big that may > > >> influence the guest loads. Add a qmp to config it can make it > > >> more flexible. > > >> > > >> Signed-off-by: Li Qiang <liq3ea@gmail.com> > > > > > > This should be done as a migration parameter rather than a new command. > > > For example, follow the cpu-throttle-increment parameter; and this > > > should work just like it. > > > Hello all, Thanks for your review. I will prepare another migration parameter version patch set for this. Thanks, Li Qiang > I was about to comment this one. > > > > migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, > > query-migrate-cache-size are marked as deprecated. Any way that we > > could have make more clear that one should use > > migrate_set/get_parameter/capability? > > Where are they marked as deprecated ? > They're not included in our official list of deprecated features, so per > our deprecation policy, they are still considered supported. > > https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features > > To deprecate something it needs to be added to qemu-deprecated.texi, and > ideally also made to print a message to stderr when triggered. > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/ > dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/ > dberrange :| >
Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Jul 31, 2018 at 04:48:35PM +0200, Juan Quintela wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > * Li Qiang (liq3ea@gmail.com) wrote: >> >> The default max cpu throttle is 99, this is too big that may >> >> influence the guest loads. Add a qmp to config it can make it >> >> more flexible. >> >> >> >> Signed-off-by: Li Qiang <liq3ea@gmail.com> >> > >> > This should be done as a migration parameter rather than a new command. >> > For example, follow the cpu-throttle-increment parameter; and this >> > should work just like it. >> >> I was about to comment this one. >> >> migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, >> query-migrate-cache-size are marked as deprecated. Any way that we >> could have make more clear that one should use >> migrate_set/get_parameter/capability? > > Where are they marked as deprecated ? ## # @migrate_set_downtime: # # Set maximum tolerated downtime for migration. # # @value: maximum downtime in seconds # # Returns: nothing on success # # Notes: This command is deprecated in favor of 'migrate-set-parameters' # # Since: 0.14.0 # # Example: # # -> { "execute": "migrate_set_downtime", "arguments": { "value": 0.1 } } # <- { "return": {} } # ## { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} } Notes: field. Clearly we need to do still more work there. Will change the texi, thanks. > > They're not included in our official list of deprecated features, so per > our deprecation policy, they are still considered supported. > > https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features > > To deprecate something it needs to be added to qemu-deprecated.texi, and > ideally also made to print a message to stderr when triggered. will send a patch to add to the texi and the print to stderr, thanks.
diff --git a/cpus.c b/cpus.c index 6569c73d24..0505477086 100644 --- a/cpus.c +++ b/cpus.c @@ -765,6 +765,11 @@ void cpu_throttle_set(int new_throttle_pct) CPU_THROTTLE_TIMESLICE_NS); } +void set_cpu_max_throttle(int64_t max_pct) +{ + throttle_percentage_max = max_pct; +} + void cpu_throttle_stop(void) { atomic_set(&throttle_percentage, 0); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index bd796579ee..4328d2e8ad 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -816,6 +816,14 @@ CPUState *cpu_by_arch_id(int64_t id); void cpu_throttle_set(int new_throttle_pct); /** + * set_cpu_max_throttle: + * @max_pct: the max percent of sleep time. Valid range is 1 to 99. + * + * Set the max throttle percentage. + */ +void set_cpu_max_throttle(int64_t max_pct); + +/** * cpu_throttle_stop: * * Stops the vcpu throttling started by cpu_throttle_set. diff --git a/migration/migration.c b/migration/migration.c index b7d9854bda..9c7a6f7477 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1774,6 +1774,16 @@ void qmp_migrate_set_speed(int64_t value, Error **errp) qmp_migrate_set_parameters(&p, errp); } +void qmp_migrate_set_max_cpu_throttle(int64_t value, Error **errp) +{ + if (value > 99 || value < 1) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, " max cpu throttle", + "not in 1 between 99"); + return; + } + set_cpu_max_throttle(value); +} + void qmp_migrate_set_downtime(double value, Error **errp) { if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { diff --git a/qapi/migration.json b/qapi/migration.json index 186e8a7303..2ef3ddfe43 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -997,6 +997,19 @@ { 'command': 'migrate_set_speed', 'data': {'value': 'int'} } ## +# @migrate_set_max_cpu_throttle: +# +# Set maximum cpu throttle for migration. +# +# @value: maximum cpu throttle. +# +# Returns: nothing on success +# +# Since: 2.12 +## +{ 'command': 'migrate_set_max_cpu_throttle', 'data': {'value': 'int'} } + +## # @migrate-set-cache-size: # # Set cache size to be used by XBZRLE migration
The default max cpu throttle is 99, this is too big that may influence the guest loads. Add a qmp to config it can make it more flexible. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- cpus.c | 5 +++++ include/qom/cpu.h | 8 ++++++++ migration/migration.c | 10 ++++++++++ qapi/migration.json | 13 +++++++++++++ 4 files changed, 36 insertions(+)