From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33975) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp1KJ-0005IM-Im for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:07:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sp1KI-0007hN-8S for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:07:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20750) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp1KH-0007h1-SC for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:07:50 -0400 Date: Wed, 11 Jul 2012 15:08:17 -0300 From: Luiz Capitulino Message-ID: <20120711150817.09c87ff4@doriath.home> In-Reply-To: <2b6cf8e2cf421bb6645a653bd7d79a5d321faee1.1340987905.git.quintela@redhat.com> References: <2b6cf8e2cf421bb6645a653bd7d79a5d321faee1.1340987905.git.quintela@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/13] Add spent time for migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: aliguori@us.ibm.com, Eric Blake , qemu-devel@nongnu.org On Fri, 29 Jun 2012 18:43:57 +0200 Juan Quintela 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 > --- > 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