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

Commit Message

Zheng Chuan July 25, 2020, 3:11 a.m. UTC

From: Zheng Chuan <[e-mail]>

Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
by libvirt api.

Signed-off-by: Zheng Chuan <[e-mail]>
Signed-off-by: YanYing Zhang <[e-mail]>
---
 migration/Makefile.objs |  1 +
 migration/dirtyrate.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json     | 24 ++++++++++++++++++++++++
 qapi/pragma.json        |  3 ++-
 4 files changed, 72 insertions(+), 1 deletion(-)

Comments

Eric Blake Aug. 4, 2020, 4:28 p.m. UTC | #1

On 7/24/20 10:11 PM, Chuan Zheng wrote:
> From: Zheng Chuan <[e-mail]>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <[e-mail]>
> Signed-off-by: YanYing Zhang <[e-mail]>
> ---

> +++ b/qapi/migration.json
> @@ -1621,3 +1621,27 @@
>   ##
>   { 'event': 'UNPLUG_PRIMARY',
>     'data': { 'device-id': 'str' } }
> +
> +##
> +# @cal_dirty_rate:

New QMP commands should be named favoring '-' over '_'; also, it doesn't 
hurt to spell it out:

calculate-dirty-rate

> +#
> +# start calculating dirty rate for vm
> +#
> +# @value: time for sample dirty pages

In what unit?

> +#
> +# Since: 5.1

We've missed 5.1; this will need to be updated to 5.2.

> +#
> +# Example:
> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
> +#
> +##
> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }
> +
> +##
> +# @get_dirty_rate:

get-dirty-rate, except that we tend to use 'query-' as the prefix for 
commands that read values.

> +#
> +# get dirty rate for vm
> +#
> +# Since: 5.1

5.2

What units is the rate expressed in?


> +##
> +{ '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
>

Eric Blake Aug. 4, 2020, 4:34 p.m. UTC | #2

On 7/24/20 10:11 PM, Chuan Zheng wrote:
> From: Zheng Chuan <[e-mail]>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <[e-mail]>
> Signed-off-by: YanYing Zhang <[e-mail]>

> +##
> +{ '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' ],

Nack.  You should not have to change the whitelist; this is evidence 
that your command is returning the wrong type.  Instead, you should be 
using:

{ 'command': 'get-dirty-rate', 'returns': { 'rate': 'int64' } }

and populating a struct, so that if we ever want to return more than 
just a single rate, we can extend the command in-place by adding to the 
struct.

Dr. David Alan Gilbert Aug. 4, 2020, 6:02 p.m. UTC | #3

* Chuan Zheng ([e-mail]) wrote:
> From: Zheng Chuan <[e-mail]>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <[e-mail]>
> Signed-off-by: YanYing Zhang <[e-mail]>
> ---
>  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 / [e-mail] / Manchester, UK

Zheng Chuan Aug. 6, 2020, 7:37 a.m. UTC | #4

On 2020/8/5 0:28, Eric Blake wrote:
> On 7/24/20 10:11 PM, Chuan Zheng wrote:
>> From: Zheng Chuan <[e-mail]>
>>
>> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>> by libvirt api.
>>
>> Signed-off-by: Zheng Chuan <[e-mail]>
>> Signed-off-by: YanYing Zhang <[e-mail]>
>> ---
> 
>> +++ b/qapi/migration.json
>> @@ -1621,3 +1621,27 @@
>>   ##
>>   { 'event': 'UNPLUG_PRIMARY',
>>     'data': { 'device-id': 'str' } }
>> +
>> +##
>> +# @cal_dirty_rate:
> 
> New QMP commands should be named favoring '-' over '_'; also, it doesn't hurt to spell it out:
> 
> calculate-dirty-rate
> 
Hi, Eric.
Thank you for your review.
I'll fix that in V2.

>> +#
>> +# start calculating dirty rate for vm
>> +#
>> +# @value: time for sample dirty pages
> 
> In what unit?
> 
The unit is second, i'll make comments in V2:)

>> +#
>> +# Since: 5.1
> 
> We've missed 5.1; this will need to be updated to 5.2.
> 
>> +#
>> +# Example:
>> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
>> +#
>> +##
>> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }
>> +
>> +##
>> +# @get_dirty_rate:
> 
> get-dirty-rate, except that we tend to use 'query-' as the prefix for commands that read values.
> 
>> +#
>> +# get dirty rate for vm
>> +#
>> +# Since: 5.1
> 
> 5.2
> 
> What units is the rate expressed in?
> 
The unit is MB/s, i'll make comments in V2:)
> 
>> +##
>> +{ '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
>>
>

Patch

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;
+    }  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;
+
+    dirtyrate_config.sample_period_seconds = value;
+    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'} }
+
+##
+# @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