Re: [RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/Makefile.objs |  1 +
>  migration/dirtyrate.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json     | 24 ++++++++++++++++++++++++
>  qapi/pragma.json        |  3 ++-
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index 0fc619e..12ae98c 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
>  common-obj-y += qemu-file-channel.o
>  common-obj-y += xbzrle.o postcopy-ram.o
>  common-obj-y += qjson.o
> +common-obj-y += dirtyrate.o
>  common-obj-y += block-dirty-bitmap.o
>  common-obj-y += multifd.o
>  common-obj-y += multifd-zlib.o
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index d87e16d..5717521 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -31,6 +31,21 @@ CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>          INTERNAL_RAMBLOCK_FOREACH(block)                   \
>          if (!qemu_ram_is_migratable(block)) {} else
>  
> +static int64_t get_dirty_rate_info(void)
> +{
> +    int64_t dirty_rate = dirty_stat.dirty_rate;
> +    /*
> +     *    Return dirty_rate only when we get CAL_DIRTY_RATE_END.
> +     *    And we must initial calculating_dirty_rate_stage.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) {
> +        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
> +        return dirty_rate;

I don't think a 'get' should change state - in particular I think we
should be able to call 'get' multiple times and get the result back.
Also, instead of just returning -1 you should probably give an error;
e.g. 'Still measuring' or 'not yet measured'

> +    }  else {
> +        return -1;
> +    }
> +}
> +
>  static void reset_dirtyrate_stat(void)
>  {
>      dirty_stat.total_dirty_samples = 0;
> @@ -377,3 +392,33 @@ void *get_dirtyrate_thread(void *arg)
>  
>      return NULL;
>  }
> +
> +void qmp_cal_dirty_rate(int64_t value, Error **errp)
> +{
> +    static struct dirtyrate_config dirtyrate_config;
> +    QemuThread thread;
> +
> +    /*
> +     * We don't begin calculating thread only when it's in calculating status.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_ING) {
> +        return;
> +    }
> +
> +    /*
> +     * If we get CAL_DIRTY_RATE_END here, libvirtd may be restarted at last round,
> +     * we need to initial it.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END)
> +        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;

Note please run the patches through scripts/checkpatch.pl - you need
some {'s here and that script tells you about most of these things.

> +    dirtyrate_config.sample_period_seconds = value;

Ah, this is where the config comes from - OK, I think you said there was
a max, so you should probably check that the maximum is sensible here
and give an error if it's not.

Dave

> +    dirtyrate_config.sample_pages_per_gigabytes = sample_pages_per_gigabytes;
> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
> +                       (void *)&dirtyrate_config, QEMU_THREAD_DETACHED);
> +}
> +
> +int64_t qmp_get_dirty_rate(Error **errp)
> +{
> +    return get_dirty_rate_info();
> +}
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d500055..59f35bb 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1621,3 +1621,27 @@
>  ##
>  { 'event': 'UNPLUG_PRIMARY',
>    'data': { 'device-id': 'str' } }
> +
> +##
> +# @cal_dirty_rate:
> +#
> +# start calculating dirty rate for vm
> +#
> +# @value: time for sample dirty pages
> +#
> +# Since: 5.1
> +#
> +# Example:
> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
> +#
> +##
> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }

I'm OK with abbreviating 'calculate' but prefer calc.

> +##
> +# @get_dirty_rate:
> +#
> +# get dirty rate for vm
> +#
> +# Since: 5.1
> +##
> +{ 'command': 'get_dirty_rate', 'returns': 'int64' }
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index cffae27..ecd294b 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -10,7 +10,8 @@
>          'query-migrate-cache-size',
>          'query-tpm-models',
>          'query-tpm-types',
> -        'ringbuf-read' ],
> +        'ringbuf-read',
> +        'get_dirty_rate' ],
>      'name-case-whitelist': [
>          'ACPISlotType',             # DIMM, visible through query-acpi-ospm-status
>          'CpuInfoMIPS',              # PC, visible through query-cpu
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK