From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp1lf-0004hY-1E for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:36:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sp1lb-000832-LL for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:36:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46937) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp1lb-00082v-Cl for qemu-devel@nongnu.org; Wed, 11 Jul 2012 14:36:03 -0400 Date: Wed, 11 Jul 2012 15:36:31 -0300 From: Luiz Capitulino Message-ID: <20120711153631.6af83928@doriath.home> In-Reply-To: <4FFDC522.9070500@redhat.com> References: <2b6cf8e2cf421bb6645a653bd7d79a5d321faee1.1340987905.git.quintela@redhat.com> <20120711150817.09c87ff4@doriath.home> <4FFDC522.9070500@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: Eric Blake Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, Juan Quintela On Wed, 11 Jul 2012 12:25:38 -0600 Eric Blake wrote: > On 07/11/2012 12:08 PM, Luiz Capitulino wrote: > > 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. > > I see a difference between extending output (such as query command > return values), where clients can gracefully deal with the absence of a > parameter from an older qemu, and extending input (where it is hard to > tell up-front whether a parameter will be rejected by an older qemu). > This is a case of extending output, so I am okay with it; in fact, I'd > rather extend output of existing commands than add a new command, > because then libvirt has to probe which command to call, instead of > calling one command and then parsing everything available. Yes, agreed. At least until we have libqmp... > >> + 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? > > Libvirt forces the use of QMP for qemu 0.15 and newer. So while there > may be other users that care, libvirt could care less whether HMP output > changes between qemu 1.1 and 1.2. Good to know. > > >> +++ 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 so, now's the time to change it before it becomes locked in stone > with the qemu 1.2 release. Libvirt has not yet been taught to use the > new information. Juan, if you agree with the change I can do it myself, or you could submit the patch and I'll apply it to the QMP tree. > > >> # > >> # @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 > > Actually, milliseconds Thanks. > >> +# 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' } } > > Also, s/total_time/total-time/ - QMP favors '-' over '_' > > >> > >> ## > >> # @MigrationInfo > >> @@ -275,8 +280,9 @@ > >> # 'cancelled'. If this field is not returned, no migration process > > Did we ever decide whether to favor US (canceled) or UK (cancelled)? I don't think so :) Maybe we should accept both? Would a UK grammar teacher mark 'canceled' as wrong (or vice-versa in the US)? :) > > >> # 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) > > s/comppleted/completed/ > > >> # > >> # @disk: #optional @MigrationStats containing detailed disk migration > >> # status, only returned if status is 'active' and it is a block > > > > > > >