Patchwork [3/8] migration: move total_time from ram stats to migration info

login
register
mail settings
Submitter Juan Quintela
Date Aug. 18, 2012, 11:17 a.m.
Message ID <1345288635-1369-4-git-send-email-quintela@redhat.com>
Download mbox | patch
Permalink /patch/178448/
State New
Headers show

Comments

Juan Quintela - Aug. 18, 2012, 11:17 a.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c            |  4 ++--
 migration.c      |  6 +++---
 qapi-schema.json | 14 +++++++-------
 qmp-commands.hx  |  6 +++---
 4 files changed, 15 insertions(+), 15 deletions(-)
Eric Blake - Aug. 18, 2012, 1:02 p.m.
On 08/18/2012 05:17 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c            |  4 ++--
>  migration.c      |  6 +++---
>  qapi-schema.json | 14 +++++++-------
>  qmp-commands.hx  |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -290,10 +290,6 @@
>  #
>  # @total: total amount of bytes involved in the migration process
>  #
> -# @total-time: total amount of ms since migration started.  If
> -#        migration has ended, it returns the total migration
> -#        time. (since 1.2)
> -#
>  # @duplicate: number of duplicate pages (since 1.2)
>  #
>  # @normal : number of normal pages (since 1.2)
> @@ -304,8 +300,7 @@
>  ##
>  { 'type': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> -           'total-time': 'int', 'duplicate': 'int', 'normal': 'int',
> -           'normal-bytes': 'int' } }
> +           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
> 
>  ##
>  # @XBZRLECacheStats
> @@ -350,12 +345,17 @@
>  #                migration statistics, only returned if XBZRLE feature is on and
>  #                status is 'active' or 'completed' (since 1.2)
>  #
> +# @total-time: total amount of milliseconds since migration started.
> +#        If migration has ended, it returns the total migration
> +#        time. (since 1.2)
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',
> -           '*xbzrle-cache': 'XBZRLECacheStats'} }
> +           '*xbzrle-cache': 'XBZRLECacheStats',
> +           'total-time': 'int'} }

Anthony - are you planning on taking this series for 1.2?  If we don't
get this patch in on time, then taking this for 1.3 would result in
changing released QMP interface (right now, there has been no release
with the field in the wrong type).
Orit Wasserman - Aug. 21, 2012, 10:53 a.m.
On 08/18/2012 02:17 PM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c            |  4 ++--
>  migration.c      |  6 +++---
>  qapi-schema.json | 14 +++++++-------
>  qmp-commands.hx  |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index a9d5675..81c8acb 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -149,6 +149,8 @@ void hmp_info_migrate(Monitor *mon)
> 
>      if (info->has_status) {
>          monitor_printf(mon, "Migration status: %s\n", info->status);
> +        monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
> +                       info->total_time);
>      }
> 
>      if (info->has_ram) {
> @@ -158,8 +160,6 @@ void hmp_info_migrate(Monitor *mon)
>                         info->ram->remaining >> 10);
>          monitor_printf(mon, "total ram: %" PRIu64 " kbytes\n",
>                         info->ram->total >> 10);
> -        monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
> -                       info->ram->total_time);
>          monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
>                         info->ram->duplicate);
>          monitor_printf(mon, "normal: %" PRIu64 " pages\n",
> diff --git a/migration.c b/migration.c
> index 653a3c1..8e4c508 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -166,14 +166,14 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>      case MIG_STATE_ACTIVE:
>          info->has_status = true;
>          info->status = g_strdup("active");
> +        info->total_time = qemu_get_clock_ms(rt_clock)
> +            - s->total_time;
> 
>          info->has_ram = true;
>          info->ram = g_malloc0(sizeof(*info->ram));
>          info->ram->transferred = ram_bytes_transferred();
>          info->ram->remaining = ram_bytes_remaining();
>          info->ram->total = ram_bytes_total();
> -        info->ram->total_time = qemu_get_clock_ms(rt_clock)
> -            - s->total_time;
>          info->ram->duplicate = dup_mig_pages_transferred();
>          info->ram->normal = norm_mig_pages_transferred();
>          info->ram->normal_bytes = norm_mig_bytes_transferred();
> @@ -193,13 +193,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> 
>          info->has_status = true;
>          info->status = g_strdup("completed");
> +        info->total_time = s->total_time;
> 
>          info->has_ram = true;
>          info->ram = g_malloc0(sizeof(*info->ram));
>          info->ram->transferred = ram_bytes_transferred();
>          info->ram->remaining = 0;
>          info->ram->total = ram_bytes_total();
> -        info->ram->total_time = s->total_time;
>          info->ram->duplicate = dup_mig_pages_transferred();
>          info->ram->normal = norm_mig_pages_transferred();
>          info->ram->normal_bytes = norm_mig_bytes_transferred();
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3d2b2d1..f83cf22 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -290,10 +290,6 @@
>  #
>  # @total: total amount of bytes involved in the migration process
>  #
> -# @total-time: total amount of ms since migration started.  If
> -#        migration has ended, it returns the total migration
> -#        time. (since 1.2)
> -#
>  # @duplicate: number of duplicate pages (since 1.2)
>  #
>  # @normal : number of normal pages (since 1.2)
> @@ -304,8 +300,7 @@
>  ##
>  { 'type': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> -           'total-time': 'int', 'duplicate': 'int', 'normal': 'int',
> -           'normal-bytes': 'int' } }
> +           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
> 
>  ##
>  # @XBZRLECacheStats
> @@ -350,12 +345,17 @@
>  #                migration statistics, only returned if XBZRLE feature is on and
>  #                status is 'active' or 'completed' (since 1.2)
>  #
> +# @total-time: total amount of milliseconds since migration started.
> +#        If migration has ended, it returns the total migration
> +#        time. (since 1.2)
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',
> -           '*xbzrle-cache': 'XBZRLECacheStats'} }
> +           '*xbzrle-cache': 'XBZRLECacheStats',
> +           'total-time': 'int'} }
> 
>  ##
>  # @query-migrate
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2ce4ce6..8671bf3 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2239,14 +2239,14 @@ The main json-object contains the following:
> 
>  - "status": migration status (json-string)
>       - Possible values: "active", "completed", "failed", "cancelled"
> +- "total-time": total amount of ms since migration started.  If
> +                migration has ended, it returns the total migration
> +		 time (json-int)
>  - "ram": only present if "status" is "active", it is a json-object with the
>    following RAM information (in bytes):
>           - "transferred": amount transferred (json-int)
>           - "remaining": amount remaining (json-int)
>           - "total": total (json-int)
> -         - "total-time": total amount of ms since migration started.  If
> -                         migration has ended, it returns the total migration time
> -                         (json-int)
>           - "duplicate": number of duplicated pages (json-int)
>           - "normal" : number of normal pages transferred (json-int)
>           - "normal-bytes" : number of normal bytes transferred (json-int)
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Luiz Capitulino - Aug. 21, 2012, 2:42 p.m.
On Sat, 18 Aug 2012 07:02:50 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 08/18/2012 05:17 AM, Juan Quintela wrote:
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  hmp.c            |  4 ++--
> >  migration.c      |  6 +++---
> >  qapi-schema.json | 14 +++++++-------
> >  qmp-commands.hx  |  6 +++---
> >  4 files changed, 15 insertions(+), 15 deletions(-)
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -290,10 +290,6 @@
> >  #
> >  # @total: total amount of bytes involved in the migration process
> >  #
> > -# @total-time: total amount of ms since migration started.  If
> > -#        migration has ended, it returns the total migration
> > -#        time. (since 1.2)
> > -#
> >  # @duplicate: number of duplicate pages (since 1.2)
> >  #
> >  # @normal : number of normal pages (since 1.2)
> > @@ -304,8 +300,7 @@
> >  ##
> >  { 'type': 'MigrationStats',
> >    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> > -           'total-time': 'int', 'duplicate': 'int', 'normal': 'int',
> > -           'normal-bytes': 'int' } }
> > +           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
> > 
> >  ##
> >  # @XBZRLECacheStats
> > @@ -350,12 +345,17 @@
> >  #                migration statistics, only returned if XBZRLE feature is on and
> >  #                status is 'active' or 'completed' (since 1.2)
> >  #
> > +# @total-time: total amount of milliseconds since migration started.
> > +#        If migration has ended, it returns the total migration
> > +#        time. (since 1.2)

Field is optional, needs to be marked as such and the has_total_time field
should be set appropriately.

> > +#
> >  # Since: 0.14.0
> >  ##
> >  { 'type': 'MigrationInfo',
> >    'data': {'*status': 'str', '*ram': 'MigrationStats',
> >             '*disk': 'MigrationStats',
> > -           '*xbzrle-cache': 'XBZRLECacheStats'} }
> > +           '*xbzrle-cache': 'XBZRLECacheStats',
> > +           'total-time': 'int'} }
> 
> Anthony - are you planning on taking this series for 1.2?  If we don't
> get this patch in on time, then taking this for 1.3 would result in
> changing released QMP interface (right now, there has been no release
> with the field in the wrong type).

I can cherry-pick this into the qmp branch.

Juan, I can also fix myself the problem I pointed out above if that
works for you.
Juan Quintela - Aug. 21, 2012, 3 p.m.
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Sat, 18 Aug 2012 07:02:50 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
> I can cherry-pick this into the qmp branch.
>
> Juan, I can also fix myself the problem I pointed out above if that
> works for you.

I very much preffer that.  But what to do with the other ones?

They are really simple, and it makes things better for users?

Anthony?

Later, Juan.
Luiz Capitulino - Aug. 21, 2012, 6:24 p.m.
On Tue, 21 Aug 2012 17:00:20 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Sat, 18 Aug 2012 07:02:50 -0600
> > Eric Blake <eblake@redhat.com> wrote:
> >
> > I can cherry-pick this into the qmp branch.
> >
> > Juan, I can also fix myself the problem I pointed out above if that
> > works for you.
> 
> I very much preffer that.

Done, and applied to the qmp branch for 1.2.

>  But what to do with the other ones?

They look good to me, but I think it's too late for 1.2.
Anthony Liguori - Aug. 22, 2012, 1:22 p.m.
Eric Blake <eblake@redhat.com> writes:

> On 08/18/2012 05:17 AM, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hmp.c            |  4 ++--
>>  migration.c      |  6 +++---
>>  qapi-schema.json | 14 +++++++-------
>>  qmp-commands.hx  |  6 +++---
>>  4 files changed, 15 insertions(+), 15 deletions(-)
>> 
>
>> +++ b/qapi-schema.json
>> @@ -290,10 +290,6 @@
>>  #
>>  # @total: total amount of bytes involved in the migration process
>>  #
>> -# @total-time: total amount of ms since migration started.  If
>> -#        migration has ended, it returns the total migration
>> -#        time. (since 1.2)
>> -#
>>  # @duplicate: number of duplicate pages (since 1.2)
>>  #
>>  # @normal : number of normal pages (since 1.2)
>> @@ -304,8 +300,7 @@
>>  ##
>>  { 'type': 'MigrationStats',
>>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>> -           'total-time': 'int', 'duplicate': 'int', 'normal': 'int',
>> -           'normal-bytes': 'int' } }
>> +           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }
>> 
>>  ##
>>  # @XBZRLECacheStats
>> @@ -350,12 +345,17 @@
>>  #                migration statistics, only returned if XBZRLE feature is on and
>>  #                status is 'active' or 'completed' (since 1.2)
>>  #
>> +# @total-time: total amount of milliseconds since migration started.
>> +#        If migration has ended, it returns the total migration
>> +#        time. (since 1.2)
>> +#
>>  # Since: 0.14.0
>>  ##
>>  { 'type': 'MigrationInfo',
>>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>>             '*disk': 'MigrationStats',
>> -           '*xbzrle-cache': 'XBZRLECacheStats'} }
>> +           '*xbzrle-cache': 'XBZRLECacheStats',
>> +           'total-time': 'int'} }
>
> Anthony - are you planning on taking this series for 1.2?

No.  This is a new feature and we're past freeze.

> If we don't
> get this patch in on time, then taking this for 1.3 would result in
> changing released QMP interface (right now, there has been no release
> with the field in the wrong type).

Ack.  We need to preserve compat with the 1.2 interface.

Regards,

Anthony Liguori

>
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Eric Blake - Aug. 22, 2012, 2:48 p.m.
On 08/22/2012 07:22 AM, Anthony Liguori wrote:

Just restating things to make sure I'm clear...

>>>  { 'type': 'MigrationInfo',
>>>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>>>             '*disk': 'MigrationStats',
>>> -           '*xbzrle-cache': 'XBZRLECacheStats'} }
>>> +           '*xbzrle-cache': 'XBZRLECacheStats',
>>> +           'total-time': 'int'} }
>>
>> Anthony - are you planning on taking this series for 1.2?
> 
> No.  This is a new feature and we're past freeze.

No, the overall series (patches 1,2,4-8) is not appropriate at this time.

> 
>> If we don't
>> get this patch in on time, then taking this for 1.3 would result in
>> changing released QMP interface (right now, there has been no release
>> with the field in the wrong type).
> 
> Ack.  We need to preserve compat with the 1.2 interface.

Yes, this particular patch 3 is a bug fix in order to prevent a future
regression when 1.3 takes the rest of the series, and must therefore be
part of the 1.2 release (and Luiz is on top of that, via the qmp branch).

Patch

diff --git a/hmp.c b/hmp.c
index a9d5675..81c8acb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -149,6 +149,8 @@  void hmp_info_migrate(Monitor *mon)

     if (info->has_status) {
         monitor_printf(mon, "Migration status: %s\n", info->status);
+        monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
+                       info->total_time);
     }

     if (info->has_ram) {
@@ -158,8 +160,6 @@  void hmp_info_migrate(Monitor *mon)
                        info->ram->remaining >> 10);
         monitor_printf(mon, "total ram: %" PRIu64 " kbytes\n",
                        info->ram->total >> 10);
-        monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
-                       info->ram->total_time);
         monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
                        info->ram->duplicate);
         monitor_printf(mon, "normal: %" PRIu64 " pages\n",
diff --git a/migration.c b/migration.c
index 653a3c1..8e4c508 100644
--- a/migration.c
+++ b/migration.c
@@ -166,14 +166,14 @@  MigrationInfo *qmp_query_migrate(Error **errp)
     case MIG_STATE_ACTIVE:
         info->has_status = true;
         info->status = g_strdup("active");
+        info->total_time = qemu_get_clock_ms(rt_clock)
+            - s->total_time;

         info->has_ram = true;
         info->ram = g_malloc0(sizeof(*info->ram));
         info->ram->transferred = ram_bytes_transferred();
         info->ram->remaining = ram_bytes_remaining();
         info->ram->total = ram_bytes_total();
-        info->ram->total_time = qemu_get_clock_ms(rt_clock)
-            - s->total_time;
         info->ram->duplicate = dup_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
@@ -193,13 +193,13 @@  MigrationInfo *qmp_query_migrate(Error **errp)

         info->has_status = true;
         info->status = g_strdup("completed");
+        info->total_time = s->total_time;

         info->has_ram = true;
         info->ram = g_malloc0(sizeof(*info->ram));
         info->ram->transferred = ram_bytes_transferred();
         info->ram->remaining = 0;
         info->ram->total = ram_bytes_total();
-        info->ram->total_time = s->total_time;
         info->ram->duplicate = dup_mig_pages_transferred();
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
diff --git a/qapi-schema.json b/qapi-schema.json
index 3d2b2d1..f83cf22 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -290,10 +290,6 @@ 
 #
 # @total: total amount of bytes involved in the migration process
 #
-# @total-time: total amount of ms since migration started.  If
-#        migration has ended, it returns the total migration
-#        time. (since 1.2)
-#
 # @duplicate: number of duplicate pages (since 1.2)
 #
 # @normal : number of normal pages (since 1.2)
@@ -304,8 +300,7 @@ 
 ##
 { 'type': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'total-time': 'int', 'duplicate': 'int', 'normal': 'int',
-           'normal-bytes': 'int' } }
+           'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int' } }

 ##
 # @XBZRLECacheStats
@@ -350,12 +345,17 @@ 
 #                migration statistics, only returned if XBZRLE feature is on and
 #                status is 'active' or 'completed' (since 1.2)
 #
+# @total-time: total amount of milliseconds since migration started.
+#        If migration has ended, it returns the total migration
+#        time. (since 1.2)
+#
 # Since: 0.14.0
 ##
 { 'type': 'MigrationInfo',
   'data': {'*status': 'str', '*ram': 'MigrationStats',
            '*disk': 'MigrationStats',
-           '*xbzrle-cache': 'XBZRLECacheStats'} }
+           '*xbzrle-cache': 'XBZRLECacheStats',
+           'total-time': 'int'} }

 ##
 # @query-migrate
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2ce4ce6..8671bf3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2239,14 +2239,14 @@  The main json-object contains the following:

 - "status": migration status (json-string)
      - Possible values: "active", "completed", "failed", "cancelled"
+- "total-time": total amount of ms since migration started.  If
+                migration has ended, it returns the total migration
+		 time (json-int)
 - "ram": only present if "status" is "active", it is a json-object with the
   following RAM information (in bytes):
          - "transferred": amount transferred (json-int)
          - "remaining": amount remaining (json-int)
          - "total": total (json-int)
-         - "total-time": total amount of ms since migration started.  If
-                         migration has ended, it returns the total migration time
-                         (json-int)
          - "duplicate": number of duplicated pages (json-int)
          - "normal" : number of normal pages transferred (json-int)
          - "normal-bytes" : number of normal bytes transferred (json-int)