Re: [Qemu-devel] [PATCH 06/13] Add spent time for migration

[Luiz Capitulino]

On Fri, 29 Jun 2012 18:43:57 +0200
Juan Quintela <address@hidden> wrote:

> We add time spent for migration to the output of "info migrate"
> command.  'total_time' means time since the start fo migration if
> migration is 'active', and total time of migration if migration is
> completed.  As we are also interested in transferred ram when
> migration completes, adding all ram statistics

I see this has already been merged and am sorry for being late with my
review, but it turns out that there are a few issues to be addressed in
this patch, comments inlined below.

Another point is that this patch extends the query-migrate command. We've
decided not to extend QMP commands, however I think that we should relax
that restriction for query commands, because the client doesn't need to know
the new fields in advance.

> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  hmp.c            |    2 ++
>  migration.c      |   11 +++++++++++
>  migration.h      |    1 +
>  qapi-schema.json |   12 +++++++++---
>  4 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index b9cec1d..4c6d4ae 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -145,6 +145,8 @@ 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);

This adds a new line to the HMP output between the end of the ram stats and
the disk stats. Iirc libvirt parses this output when in non-json mode, although
I don't think it ever does it for disk migration.

Eric, does libvirt do that?

Btw, we'll need to change where 'total_time' is printed, see below.

>      }
> 
>      if (info->has_disk) {
> diff --git a/migration.c b/migration.c
> index 810727f..8db1b43 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -131,6 +131,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          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;
> 

I really don't think that 'total_time' pertains to the ram stats info, I think
it should be in the MigrationInfo dict.


>          if (blk_mig_active()) {
>              info->has_disk = true;
> @@ -143,6 +145,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>      case MIG_STATE_COMPLETED:
>          info->has_status = true;
>          info->status = g_strdup("completed");
> +
> +        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;

Having the 'total_time' in the MigrationInfo dict would avoid this change.

>          break;
>      case MIG_STATE_ERROR:
>          info->has_status = true;
> @@ -260,6 +269,7 @@ static void migrate_fd_put_ready(void *opaque)
>          } else {
>              migrate_fd_completed(s);
>          }
> +        s->total_time = qemu_get_clock_ms(rt_clock) - s->total_time;
>          if (s->state != MIG_STATE_COMPLETED) {
>              if (old_vm_running) {
>                  vm_start();
> @@ -372,6 +382,7 @@ static MigrationState *migrate_init(const MigrationParams 
> *params)
> 
>      s->bandwidth_limit = bandwidth_limit;
>      s->state = MIG_STATE_SETUP;
> +    s->total_time = qemu_get_clock_ms(rt_clock);
> 
>      return s;
>  }
> diff --git a/migration.h b/migration.h
> index 35207bd..de13004 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -37,6 +37,7 @@ struct MigrationState
>      int (*write)(MigrationState *s, const void *buff, size_t size);
>      void *opaque;
>      MigrationParams params;
> +    int64_t total_time;
>  };
> 
>  void process_incoming_migration(QEMUFile *f);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b6e346..1ab5dbd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -260,10 +260,15 @@
>  #
>  # @total: total amount of bytes involved in the migration process
>  #
> +# @total_time: tota0l amount of ms since migration started.  If

s/total0l/total

s/ms/miliseconds

> +#        migration has ended, it returns the total migration
> +#        time. (since 1.2)
> +#
>  # Since: 0.14.0.
>  ##
>  { 'type': 'MigrationStats',
> -  'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' } }
> +  'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> +           'total_time': 'int' } }
> 
>  ##
>  # @MigrationInfo
> @@ -275,8 +280,9 @@
>  #          'cancelled'. If this field is not returned, no migration process
>  #          has been initiated
>  #
> -# @ram: #optional @MigrationStats containing detailed migration status,
> -#       only returned if status is 'active'
> +# @ram: #optional @MigrationStats containing detailed migration
> +#       status, only returned if status is 'active' or
> +#       'completed'. 'comppleted' (since 1.2)
>  #
>  # @disk: #optional @MigrationStats containing detailed disk migration
>  #        status, only returned if status is 'active' and it is a block