Re: [PATCH v2 3/3] cpus-common: implement dirty limit on vCPU

[Markus Armbruster <armbru@redhat.com>]

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
> dirtyrate given by user.
>
> introduce qmp commands set-dirty-limit/cancel-dirty-limit to
> set/cancel dirty limit on vCPU.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

[...]

> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index e948e81..dd65e9e 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -881,6 +881,13 @@ void end_exclusive(void);
>   */
>  void qemu_init_vcpu(CPUState *cpu);
>  
> +/**
> + * dirtylimit_setup:
> + *
> + * dirtylimit setup.
> + */

This is even worse than no documentation.

> +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/misc.json b/qapi/misc.json
> index 358548a..7f6da34 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -527,3 +527,47 @@
>   'data': { '*option': 'str' },
>   'returns': ['CommandLineOptionInfo'],
>   'allow-preconfig': true }
> +
> +##
> +# @DirtyRateQuotaVcpu:
> +#
> +# Dirty rate of vcpu.
> +#
> +# @idx: vcpu index.
> +#
> +# @dirtyrate: dirty rate.
> +#
> +# Since: 6.3
> +#
> +##
> +{ 'struct': 'DirtyRateQuotaVcpu',
> +  'data': { 'idx': 'int', 'dirtyrate': 'uint64' } }
> +
> +##
> +# @set-dirty-limit:
> +#
> +# Since: 6.3
> +#
> +# Example:
> +#   {"execute": "set-dirty-limit"}
> +#    "arguments": { "idx": "cpu-index",
> +#                   "dirtyrate": "quota-dirtyrate" } }

The example cannot work: the arguments must be numbers, not strings.

> +#
> +##
> +{ 'command': 'set-dirty-limit',
> +  'data': 'DirtyRateQuotaVcpu' }

Why make DirtyRateQuotaVcpu a separate type?  Why not

   { 'command': 'set-dirty-limit',
     'data': { 'idx': 'int', 'dirtyrate': 'uint64' } }

> +
> +##
> +# @cancel-dirty-limit:
> +#
> +# @idx: index of vCPU to be canceled
> +#
> +# Since: 6.3
> +#
> +# Example:
> +#   {"execute": "cancel-dirty-limit"}
> +#    "arguments": { "idx": "cpu-index" } }
> +#
> +##
> +{ 'command': 'cancel-dirty-limit',
> +  'data': { 'idx': 'int' } }

Overall, documentation is too terse.  What is a "dirty rate of vcpu",
and why should I care?  Is it related to query-dirty-rate?

Nitpick: you use both "vcpu" and "vCPU" in comments.  Stick to the
latter, please.

[...]