[Qemu-devel] [PATCH 0/3] Add a new migrate_set_max_cpu_throttle qmp command

[Qemu-devel] [PATCH 0/3] Add a new migrate_set_max_cpu_throttle qmp command

[Li Qiang]

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 qmp command to limit the CPU throttle.

Li Qiang (3):
  migrate: replace the cpu throttle percentage max with a variable
  migration: Add qmp command for migrate_set_max_cpu_throttle
  hmp: add hmp for migrate_set_max_cpu_throttle

 cpus.c                |  9 ++++++++-
 hmp-commands.hx       | 14 ++++++++++++++
 hmp.c                 |  6 ++++++
 hmp.h                 |  1 +
 include/qom/cpu.h     |  8 ++++++++
 migration/migration.c | 10 ++++++++++
 qapi/migration.json   | 13 +++++++++++++
 7 files changed, 60 insertions(+), 1 deletion(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] migrate: replace the cpu throttle percentage max with a variable

[Li Qiang]

So we can config it using qmp.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 cpus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index b5844b7103..6569c73d24 100644
--- a/cpus.c
+++ b/cpus.c
@@ -83,6 +83,8 @@ static unsigned int throttle_percentage;
 #define CPU_THROTTLE_PCT_MAX 99
 #define CPU_THROTTLE_TIMESLICE_NS 10000000
 
+static unsigned int throttle_percentage_max = CPU_THROTTLE_PCT_MAX;
+
 bool cpu_is_stopped(CPUState *cpu)
 {
     return cpu->stopped || !runstate_is_running();
@@ -754,7 +756,7 @@ static void cpu_throttle_timer_tick(void *opaque)
 void cpu_throttle_set(int new_throttle_pct)
 {
     /* Ensure throttle percentage is within valid range */
-    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
+    new_throttle_pct = MIN(new_throttle_pct, throttle_percentage_max);
     new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
 
     atomic_set(&throttle_percentage, new_throttle_pct);
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Li Qiang]

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

[Qemu-devel] [PATCH 3/3] hmp: add hmp for migrate_set_max_cpu_throttle

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hmp-commands.hx | 14 ++++++++++++++
 hmp.c           |  6 ++++++
 hmp.h           |  1 +
 3 files changed, 21 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 91dfe51c37..9d54ecbe4e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1042,6 +1042,20 @@ Set maximum speed to @var{value} (in bytes) for migrations.
 ETEXI
 
     {
+        .name       = "migrate_set_max_cpu_throttle",
+        .args_type  = "value:i",
+        .params     = "value",
+        .help       = "set maximum cpu throttle  for migrations.",
+        .cmd        = hmp_migrate_set_max_cpu_throttle,
+    },
+
+STEXI
+@item hmp_migrate_set_max_cpu_throttle @var{value}
+@findex migrate_set_max_cpu_throttle
+Set maximum cpu throttle to @var{value}  for migrations.
+ETEXI
+
+    {
         .name       = "migrate_set_downtime",
         .args_type  = "value:T",
         .params     = "value",
diff --git a/hmp.c b/hmp.c
index 2aafb50e8e..4ee9af7165 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1574,6 +1574,12 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
     qmp_migrate_set_speed(value, NULL);
 }
 
+void hmp_migrate_set_max_cpu_throttle(Monitor *mon, const QDict *qdict)
+{
+    int64_t value = qdict_get_int(qdict, "value");
+    qmp_migrate_set_max_cpu_throttle(value, NULL);
+}
+
 void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
 {
     const char *cap = qdict_get_str(qdict, "capability");
diff --git a/hmp.h b/hmp.h
index 33354f1bdd..e57f7e4005 100644
--- a/hmp.h
+++ b/hmp.h
@@ -73,6 +73,7 @@ void hmp_migrate_recover(Monitor *mon, const QDict *qdict);
 void hmp_migrate_pause(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
+void hmp_migrate_set_max_cpu_throttle(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Eric Blake]

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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Juan Quintela]

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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Daniel P. Berrangé]

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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Li Qiang]

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

Re: [Qemu-devel] [PATCH 2/3] migration: Add qmp command for migrate_set_max_cpu_throttle

[Juan Quintela]

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.