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(+)

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

* 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?)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

"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
-- 
|: 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 :|

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.