[v7,3/3] cpus-common: implement dirty page limit on vCPU

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Implement dirtyrate calculation periodically basing on
dirty-ring and throttle vCPU until it reachs the quota
dirty page rate given by user.

Introduce qmp commands set-dirty-limit/cancel-dirty-limit to
set/cancel dirty page limit on vCPU.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 cpus-common.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/core/cpu.h |  9 +++++++++
 qapi/migration.json   | 43 +++++++++++++++++++++++++++++++++++++++++++
 softmmu/vl.c          |  1 +
 4 files changed, 101 insertions(+)

On Tue, Nov 30, 2021 at 06:28:13PM +0800, huangy81@chinatelecom.cn wrote:
>  ##
> +# @set-dirty-limit:
> +#
> +# Set the upper limit of dirty page rate for a virtual CPU.
> +#
> +# Requires KVM with accelerator property "dirty-ring-size" set.
> +# A virtual CPU's dirty page rate is a measure of its memory load.
> +# To observe dirty page rates, use @calc-dirty-rate.
> +#
> +# @cpu-index: index of the virtual CPU.
> +#
> +# @dirty-rate: upper limit for the specified vCPU's dirty page rate (MB/s)
> +#
> +# Since: 7.0
> +#
> +# Example:
> +#   {"execute": "set-dirty-limit"}
> +#    "arguments": { "cpu-index": 0,
> +#                   "dirty-rate": 200 } }
> +#
> +##
> +{ 'command': 'set-dirty-limit',
> +  'data': { 'cpu-index': 'int', 'dirty-rate': 'uint64' } }
> +
> +##
> +# @cancel-dirty-limit:
> +#
> +# Cancel the dirty page limit for the vCPU which has been set with
> +# set-dirty-limit command. Note that this command requires support from
> +# dirty ring, same as the "set-dirty-limit" command.
> +#
> +# @cpu-index: index of the virtual CPU to cancel the dirty page limit
> +#
> +# Since: 7.0
> +#
> +# Example:
> +#   {"execute": "cancel-dirty-limit"}
> +#    "arguments": { "cpu-index": 0 } }
> +#
> +##
> +{ 'command': 'cancel-dirty-limit',
> +  'data': { 'cpu-index': 'int' } }

This seems to be overloaded to be a standalone cmd..

How about:

  { "cmd": "vcpu-dirty-limit",
    "arguments": {
      "cpu": $cpu,
      "enable": true/false,
      "dirty-rate": 100,
    }
  }

If "enable"==false, then "dirty-rate" can be ignored and it'll shut down the
throttling on vcpu N.  Then this command will literally merge the two you
proposed.

It'll be nice if we provide yet another command:

  { "cmd": "query-vcpu-dirty-limit",
    "arguments": {
      "*cpu": $cpu,
    }
  }

When $cpu is specified, we return (cpu=$cpu, real_dirty_rate,
target_dirty_rate) for this vcpu.  When $cpu is not specified, we return an
array of that containing all the vcpus.

It'll be nicer to enhance the output of the query command to e.g. have a global
"enabled"=true/false as long as any vcpu has throttle enabled then the global
throttle is enabled.  I didn't think more than that, but how's that sound so
far?

Thanks,

On 11/30/21 21:21, Peter Xu wrote:
> On Tue, Nov 30, 2021 at 06:28:13PM +0800, huangy81@chinatelecom.cn wrote:
>>   ##
>> +# @set-dirty-limit:
>> +#
>> +# Set the upper limit of dirty page rate for a virtual CPU.
>> +#
>> +# Requires KVM with accelerator property "dirty-ring-size" set.
>> +# A virtual CPU's dirty page rate is a measure of its memory load.
>> +# To observe dirty page rates, use @calc-dirty-rate.
>> +#
>> +# @cpu-index: index of the virtual CPU.
>> +#
>> +# @dirty-rate: upper limit for the specified vCPU's dirty page rate (MB/s)
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +#   {"execute": "set-dirty-limit"}
>> +#    "arguments": { "cpu-index": 0,
>> +#                   "dirty-rate": 200 } }
>> +#
>> +##
>> +{ 'command': 'set-dirty-limit',
>> +  'data': { 'cpu-index': 'int', 'dirty-rate': 'uint64' } }
>> +
>> +##
>> +# @cancel-dirty-limit:
>> +#
>> +# Cancel the dirty page limit for the vCPU which has been set with
>> +# set-dirty-limit command. Note that this command requires support from
>> +# dirty ring, same as the "set-dirty-limit" command.
>> +#
>> +# @cpu-index: index of the virtual CPU to cancel the dirty page limit
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +#   {"execute": "cancel-dirty-limit"}
>> +#    "arguments": { "cpu-index": 0 } }
>> +#
>> +##
>> +{ 'command': 'cancel-dirty-limit',
>> +  'data': { 'cpu-index': 'int' } }
> 
> This seems to be overloaded to be a standalone cmd..
> 
> How about:
> 
>    { "cmd": "vcpu-dirty-limit",
>      "arguments": {
>        "cpu": $cpu,
>        "enable": true/false,
>        "dirty-rate": 100,
>      }
>    }
> 
> If "enable"==false, then "dirty-rate" can be ignored and it'll shut down the
> throttling on vcpu N.  Then this command will literally merge the two you
> proposed.
> 
> It'll be nice if we provide yet another command:
> 
>    { "cmd": "query-vcpu-dirty-limit",
>      "arguments": {
>        "*cpu": $cpu,
>      }
>    }
> 
> When $cpu is specified, we return (cpu=$cpu, real_dirty_rate,
> target_dirty_rate) for this vcpu.  When $cpu is not specified, we return an
> array of that containing all the vcpus.
> 
> It'll be nicer to enhance the output of the query command to e.g. have a global
> "enabled"=true/false as long as any vcpu has throttle enabled then the global
> throttle is enabled.  I didn't think more than that, but how's that sound so
> far?
Soud good, it makes the command easier for programmers to use and 
understand, i'll try this out next version.
> 
> Thanks,
>

huangy81@chinatelecom.cn writes:

> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Implement dirtyrate calculation periodically basing on
> dirty-ring and throttle vCPU until it reachs the quota
> dirty page rate given by user.
>
> Introduce qmp commands set-dirty-limit/cancel-dirty-limit to
> set/cancel dirty page limit on vCPU.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  cpus-common.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/core/cpu.h |  9 +++++++++
>  qapi/migration.json   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  softmmu/vl.c          |  1 +
>  4 files changed, 101 insertions(+)
>
> diff --git a/cpus-common.c b/cpus-common.c
> index 6e73d3e..86c7712 100644
> --- a/cpus-common.c
> +++ b/cpus-common.c
> @@ -23,6 +23,11 @@
>  #include "hw/core/cpu.h"
>  #include "sysemu/cpus.h"
>  #include "qemu/lockable.h"
> +#include "sysemu/dirtylimit.h"
> +#include "sysemu/cpu-throttle.h"
> +#include "sysemu/kvm.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
>  
>  static QemuMutex qemu_cpu_list_lock;
>  static QemuCond exclusive_cond;
> @@ -352,3 +357,46 @@ void process_queued_cpu_work(CPUState *cpu)
>      qemu_mutex_unlock(&cpu->work_mutex);
>      qemu_cond_broadcast(&qemu_work_cond);
>  }
> +
> +void qmp_set_dirty_limit(int64_t idx,
> +                         uint64_t dirtyrate,
> +                         Error **errp)
> +{
> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
> +        error_setg(errp, "setting a dirty page limit requires KVM with"
> +                   " accelerator property 'dirty-ring-size' set'");
> +        return;
> +    }
> +
> +    dirtylimit_calc();
> +    dirtylimit_vcpu(idx, dirtyrate);
> +}
> +
> +void qmp_cancel_dirty_limit(int64_t idx,
> +                            Error **errp)
> +{
> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
> +        error_setg(errp, "KVM with accelerator property 'dirty-ring-size'"
> +                   " not set, abort canceling a dirty page limit");
> +        return;
> +    }

Is this check actually needed?  It's not when !dirtylimit_enabled(idx).

> +
> +    if (!dirtylimit_enabled(idx)) {
> +        error_setg(errp, "dirty page limit for the CPU %ld not set", idx);

"for CPU"

> +        return;
> +    }
> +
> +    if (unlikely(!dirtylimit_cancel_vcpu(idx))) {

I don't think unlikely() matters here.

> +        dirtylimit_calc_quit();
> +    }
> +}
> +
> +void dirtylimit_setup(int max_cpus)
> +{
> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
> +        return;
> +    }
> +
> +    dirtylimit_calc_state_init(max_cpus);
> +    dirtylimit_state_init(max_cpus);
> +}
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index e948e81..11df012 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -881,6 +881,15 @@ void end_exclusive(void);
>   */
>  void qemu_init_vcpu(CPUState *cpu);
>  
> +/**
> + * dirtylimit_setup:
> + *
> + * Initializes the global state of dirtylimit calculation and
> + * dirtylimit itself. This is prepared for vCPU dirtylimit which
> + * could be triggered during vm lifecycle.
> + */
> +void dirtylimit_setup(int max_cpus);
> +
>  #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
>  #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */
>  #define SSTEP_NOTIMER 0x4  /* Do not Timers while single stepping */
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bbfd48c..57c9a63 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1850,6 +1850,49 @@
>  { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
>  
>  ##
> +# @set-dirty-limit:
> +#
> +# Set the upper limit of dirty page rate for a virtual CPU.
> +#
> +# Requires KVM with accelerator property "dirty-ring-size" set.
> +# A virtual CPU's dirty page rate is a measure of its memory load.
> +# To observe dirty page rates, use @calc-dirty-rate.
> +#
> +# @cpu-index: index of the virtual CPU.
> +#
> +# @dirty-rate: upper limit for the specified vCPU's dirty page rate (MB/s)
> +#
> +# Since: 7.0
> +#
> +# Example:
> +#   {"execute": "set-dirty-limit"}
> +#    "arguments": { "cpu-index": 0,
> +#                   "dirty-rate": 200 } }
> +#
> +##
> +{ 'command': 'set-dirty-limit',
> +  'data': { 'cpu-index': 'int', 'dirty-rate': 'uint64' } }
> +
> +##
> +# @cancel-dirty-limit:
> +#
> +# Cancel the dirty page limit for the vCPU which has been set with
> +# set-dirty-limit command. Note that this command requires support from
> +# dirty ring, same as the "set-dirty-limit" command.
> +#
> +# @cpu-index: index of the virtual CPU to cancel the dirty page limit

I'd go with

   # @cpu-index: index of the virtual CPU.

> +#
> +# Since: 7.0
> +#
> +# Example:
> +#   {"execute": "cancel-dirty-limit"}
> +#    "arguments": { "cpu-index": 0 } }
> +#
> +##
> +{ 'command': 'cancel-dirty-limit',
> +  'data': { 'cpu-index': 'int' } }
> +
> +##
>  # @snapshot-save:
>  #
>  # Save a VM snapshot
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 620a1f1..0f83ce3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3777,5 +3777,6 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_init_displays();
>      accel_setup_post(current_machine);
>      os_setup_post();
> +    dirtylimit_setup(current_machine->smp.max_cpus);
>      resume_mux_open();
>  }

QAPI schema:
Acked-by: Markus Armbruster <armbru@redhat.com>

在 2021/12/3 0:02, Markus Armbruster 写道:
> huangy81@chinatelecom.cn writes:
> 
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Implement dirtyrate calculation periodically basing on
>> dirty-ring and throttle vCPU until it reachs the quota
>> dirty page rate given by user.
>>
>> Introduce qmp commands set-dirty-limit/cancel-dirty-limit to
>> set/cancel dirty page limit on vCPU.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> ---
>>   cpus-common.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/core/cpu.h |  9 +++++++++
>>   qapi/migration.json   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   softmmu/vl.c          |  1 +
>>   4 files changed, 101 insertions(+)
>>
>> diff --git a/cpus-common.c b/cpus-common.c
>> index 6e73d3e..86c7712 100644
>> --- a/cpus-common.c
>> +++ b/cpus-common.c
>> @@ -23,6 +23,11 @@
>>   #include "hw/core/cpu.h"
>>   #include "sysemu/cpus.h"
>>   #include "qemu/lockable.h"
>> +#include "sysemu/dirtylimit.h"
>> +#include "sysemu/cpu-throttle.h"
>> +#include "sysemu/kvm.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qapi-commands-migration.h"
>>   
>>   static QemuMutex qemu_cpu_list_lock;
>>   static QemuCond exclusive_cond;
>> @@ -352,3 +357,46 @@ void process_queued_cpu_work(CPUState *cpu)
>>       qemu_mutex_unlock(&cpu->work_mutex);
>>       qemu_cond_broadcast(&qemu_work_cond);
>>   }
>> +
>> +void qmp_set_dirty_limit(int64_t idx,
>> +                         uint64_t dirtyrate,
>> +                         Error **errp)
>> +{
>> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
>> +        error_setg(errp, "setting a dirty page limit requires KVM with"
>> +                   " accelerator property 'dirty-ring-size' set'");
>> +        return;
>> +    }
>> +
>> +    dirtylimit_calc();
>> +    dirtylimit_vcpu(idx, dirtyrate);
>> +}
>> +
>> +void qmp_cancel_dirty_limit(int64_t idx,
>> +                            Error **errp)
>> +{
>> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
>> +        error_setg(errp, "KVM with accelerator property 'dirty-ring-size'"
>> +                   " not set, abort canceling a dirty page limit");
>> +        return;
>> +    }
> 
> Is this check actually needed?  It's not when !dirtylimit_enabled(idx).
The logic never go there if user follows the enable/disable step, but 
just in case one call the qmp "cancel-dirty-limit" only.
Anyway, the set/cancel has been merged into one in latest version. And 
we do check before doing anything in both case.
> 
>> +
>> +    if (!dirtylimit_enabled(idx)) {
>> +        error_setg(errp, "dirty page limit for the CPU %ld not set", idx);
> 
> "for CPU"
> 
>> +        return;
>> +    }
>> +
>> +    if (unlikely(!dirtylimit_cancel_vcpu(idx))) {
> 
> I don't think unlikely() matters here.
Ok, i dropped the compilation syntax in latest version.
> 
>> +        dirtylimit_calc_quit();
>> +    }
>> +}
>> +
>> +void dirtylimit_setup(int max_cpus)
>> +{
>> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
>> +        return;
>> +    }
>> +
>> +    dirtylimit_calc_state_init(max_cpus);
>> +    dirtylimit_state_init(max_cpus);
>> +}
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index e948e81..11df012 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -881,6 +881,15 @@ void end_exclusive(void);
>>    */
>>   void qemu_init_vcpu(CPUState *cpu);
>>   
>> +/**
>> + * dirtylimit_setup:
>> + *
>> + * Initializes the global state of dirtylimit calculation and
>> + * dirtylimit itself. This is prepared for vCPU dirtylimit which
>> + * could be triggered during vm lifecycle.
>> + */
>> +void dirtylimit_setup(int max_cpus);
>> +
>>   #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
>>   #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */
>>   #define SSTEP_NOTIMER 0x4  /* Do not Timers while single stepping */
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index bbfd48c..57c9a63 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1850,6 +1850,49 @@
>>   { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
>>   
>>   ##
>> +# @set-dirty-limit:
>> +#
>> +# Set the upper limit of dirty page rate for a virtual CPU.
>> +#
>> +# Requires KVM with accelerator property "dirty-ring-size" set.
>> +# A virtual CPU's dirty page rate is a measure of its memory load.
>> +# To observe dirty page rates, use @calc-dirty-rate.
>> +#
>> +# @cpu-index: index of the virtual CPU.
>> +#
>> +# @dirty-rate: upper limit for the specified vCPU's dirty page rate (MB/s)
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +#   {"execute": "set-dirty-limit"}
>> +#    "arguments": { "cpu-index": 0,
>> +#                   "dirty-rate": 200 } }
>> +#
>> +##
>> +{ 'command': 'set-dirty-limit',
>> +  'data': { 'cpu-index': 'int', 'dirty-rate': 'uint64' } }
>> +
>> +##
>> +# @cancel-dirty-limit:
>> +#
>> +# Cancel the dirty page limit for the vCPU which has been set with
>> +# set-dirty-limit command. Note that this command requires support from
>> +# dirty ring, same as the "set-dirty-limit" command.
>> +#
>> +# @cpu-index: index of the virtual CPU to cancel the dirty page limit
> 
> I'd go with
> 
>     # @cpu-index: index of the virtual CPU.
Ok.
> 
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +#   {"execute": "cancel-dirty-limit"}
>> +#    "arguments": { "cpu-index": 0 } }
>> +#
>> +##
>> +{ 'command': 'cancel-dirty-limit',
>> +  'data': { 'cpu-index': 'int' } }
>> +
>> +##
>>   # @snapshot-save:
>>   #
>>   # Save a VM snapshot
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 620a1f1..0f83ce3 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -3777,5 +3777,6 @@ void qemu_init(int argc, char **argv, char **envp)
>>       qemu_init_displays();
>>       accel_setup_post(current_machine);
>>       os_setup_post();
>> +    dirtylimit_setup(current_machine->smp.max_cpus);
>>       resume_mux_open();
>>   }
> 
> QAPI schema:
> Acked-by: Markus Armbruster <armbru@redhat.com>
Thanks Markus very much for reviewing the code, :)
>