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

[Juan Quintela]

Luiz Capitulino <address@hidden> wrote:
> On Wed, 11 Jul 2012 12:25:38 -0600
> Eric Blake <address@hidden> wrote:
>
>> On 07/11/2012 12:08 PM, Luiz Capitulino wrote:
>> > 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.
>> 
>> 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.