From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp2eg-00064d-7K for qemu-devel@nongnu.org; Wed, 11 Jul 2012 15:32:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sp2ee-00069E-Tr for qemu-devel@nongnu.org; Wed, 11 Jul 2012 15:32:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37021) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sp2ee-000697-MJ for qemu-devel@nongnu.org; Wed, 11 Jul 2012 15:32:56 -0400 From: Juan Quintela In-Reply-To: <20120711153631.6af83928@doriath.home> (Luiz Capitulino's message of "Wed, 11 Jul 2012 15:36:31 -0300") References: <2b6cf8e2cf421bb6645a653bd7d79a5d321faee1.1340987905.git.quintela@redhat.com> <20120711150817.09c87ff4@doriath.home> <4FFDC522.9070500@redhat.com> <20120711153631.6af83928@doriath.home> Date: Wed, 11 Jul 2012 21:32:25 +0200 Message-ID: <87hate6qly.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 06/13] Add spent time for migration Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, Eric Blake , qemu-devel@nongnu.org Luiz Capitulino wrote: > 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. I can send a patch. Thanks for the review, Juan.