Patchwork [7/7] migration: Add dirty_pages_rate to query migrate output

login
register
mail settings
Submitter Juan Quintela
Date Aug. 13, 2012, 10:50 a.m.
Message ID <1344855057-32509-8-git-send-email-quintela@redhat.com>
Download mbox | patch
Permalink /patch/176915/
State New
Headers show

Comments

Juan Quintela - Aug. 13, 2012, 10:50 a.m.
For now this is a placeholder, real info will appear once the bitmap
changes in the migration thread series is integrated.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c            | 4 ++++
 migration.c      | 2 ++
 migration.h      | 1 +
 qapi-schema.json | 6 +++++-
 4 files changed, 12 insertions(+), 1 deletion(-)
Eric Blake - Aug. 13, 2012, 3:12 p.m.
On 08/13/2012 04:50 AM, Juan Quintela wrote:
> For now this is a placeholder, real info will appear once the bitmap
> changes in the migration thread series is integrated.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c            | 4 ++++
>  migration.c      | 2 ++
>  migration.h      | 1 +
>  qapi-schema.json | 6 +++++-
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index fc75ec3..dd40631 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -174,6 +174,10 @@ void hmp_info_migrate(Monitor *mon)
>                         info->ram->normal);
>          monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
>                         info->ram->normal_bytes >> 10);
> +        if (info->ram->dirty_pages_rate) {
> +            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pagfes\n",

s/pagfes/pages/

> +++ b/qapi-schema.json
> @@ -266,11 +266,15 @@
>  #
>  # @normal-bytes : number of normal bytes sent (since 1.2)

As long as you are touching here: s/ :/:/

>  #
> +# @dirty-pages-rate: number of pages dirtied by second by the
> +#        guest. (since 1.2)

Inconsistent on whether we have a '.' prior to the (since 1.2) marking.

Since HMP only prints this stat when it is non-zero, should this field
be marked optional?  Then again, once you have dirty page tracking, I
suspect this would never be zero (or even showing an explicit zero would
help detect stalls).
Juan Quintela - Aug. 13, 2012, 4:04 p.m.
Eric Blake <eblake@redhat.com> wrote:
> On 08/13/2012 04:50 AM, Juan Quintela wrote:
>> For now this is a placeholder, real info will appear once the bitmap
>> changes in the migration thread series is integrated.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hmp.c            | 4 ++++
>>  migration.c      | 2 ++
>>  migration.h      | 1 +
>>  qapi-schema.json | 6 +++++-
>>  4 files changed, 12 insertions(+), 1 deletion(-)
>> 
>>  #
>> +# @dirty-pages-rate: number of pages dirtied by second by the
>> +#        guest. (since 1.2)
>
> Inconsistent on whether we have a '.' prior to the (since 1.2) marking.
>
> Since HMP only prints this stat when it is non-zero, should this field
> be marked optional?  Then again, once you have dirty page tracking, I
> suspect this would never be zero (or even showing an explicit zero would
> help detect stalls).

What is easier for libvirt to have here.

At the beggining, this value is going to be wrong/cero. I can:
- not print it until it is != 0
- print a big enough number )
- make it optional?  I guess this would alse be more difficult for
  libvirt.
- put your good idea here?

Later, Juan.
Eric Blake - Aug. 13, 2012, 4:49 p.m.
On 08/13/2012 10:04 AM, Juan Quintela wrote:

>>> +# @dirty-pages-rate: number of pages dirtied by second by the
>>> +#        guest. (since 1.2)
>>

>> Since HMP only prints this stat when it is non-zero, should this field
>> be marked optional?  Then again, once you have dirty page tracking, I
>> suspect this would never be zero (or even showing an explicit zero would
>> help detect stalls).
> 
> What is easier for libvirt to have here.
> 
> At the beggining, this value is going to be wrong/cero. I can:
> - not print it until it is != 0
> - print a big enough number )
> - make it optional?  I guess this would alse be more difficult for
>   libvirt.
> - put your good idea here?

I see your point about querying early enough that there is not yet
enough data to have a good estimate.  In that case, either outputting 0
or omitting the field will do the trick.  Libvirt already has to support
older qemu that always omitted the field, and it is a one-liner code
addition in libvirt to say that if the field is omitted, treat it like
'0'.  I'm not even quite sure what libvirt will be able to do with this
number; libvirt made some unfortunate API decisions where migration
statistics are exposed to the user in a hard-coded struct
(virDomainJobInfo), and since we didn't allow for the struct to grow
without breaking ABI, exposing additional information would require a
new libvirt API.  So don't let libvirt hold up the inclusion of this
useful information on the qemu side of things.

Maybe another option would be to return [U]INT_MAX when there is not
enough data to yet provide a valid number, where having a non-zero value
at least lets people know that the stat will be available in a later call.

Patch

diff --git a/hmp.c b/hmp.c
index fc75ec3..dd40631 100644
--- a/hmp.c
+++ b/hmp.c
@@ -174,6 +174,10 @@  void hmp_info_migrate(Monitor *mon)
                        info->ram->normal);
         monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
                        info->ram->normal_bytes >> 10);
+        if (info->ram->dirty_pages_rate) {
+            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pagfes\n",
+                           info->ram->dirty_pages_rate);
+        }
     }

     if (info->has_disk) {
diff --git a/migration.c b/migration.c
index 28e23db..8d67e9b 100644
--- a/migration.c
+++ b/migration.c
@@ -178,6 +178,8 @@  MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->duplicate = dup_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
+        info->ram->dirty_pages_rate = s->dirty_pages_rate;
+

         if (blk_mig_active()) {
             info->has_disk = true;
diff --git a/migration.h b/migration.h
index 552200c..66d7f68 100644
--- a/migration.h
+++ b/migration.h
@@ -42,6 +42,7 @@  struct MigrationState
     int64_t total_time;
     int64_t downtime;
     int64_t expected_downtime;
+    int64_t dirty_pages_rate;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index 3dcc12f..55ef73c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -266,11 +266,15 @@ 
 #
 # @normal-bytes : number of normal bytes sent (since 1.2)
 #
+# @dirty-pages-rate: number of pages dirtied by second by the
+#        guest. (since 1.2)
+#
 # Since: 0.14.0
 ##
 { 'type': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
+           'dirty-pages-rate' : 'int' } }

 ##
 # @XBZRLECacheStats