Re: [Qemu-devel] [RFC PATCH v2 07/12] mc: introduce additional QMP statistics for micro-checkpointing

[Michael R. Hines]

On 03/12/2014 05:45 AM, Eric Blake wrote:
> > +++ b/qapi-schema.json
> > @@ -603,6 +603,36 @@
> >              'cache-miss': 'int', 'overflow': 'int' } }
> >   
> >   ##
> > +# @MCStats
> > +#
> > +# Detailed Micro Checkpointing (MC) statistics
> > +#
> > +# @mbps: throughput of transmitting last MC
> > +#
> > +# @xmit-time: milliseconds to transmit last MC
> Trailing whitespace.
>
> Rather than abbreviate, how about naming this 'transmit-time'.

Acknowledged.

> > +#
> > +# @checkpoints: cummulative total number of MCs generated
> More trailing whitespace.  Please run your series through
> scripts/checkpatch.pl.
>
> s/cummulative total/cumulative/

Acknowledged.


> > +#
> > +# Since: 2.x
> > +##
> > +{ 'type': 'MCStats',
> > +  'data': {'mbps': 'number',
> > +           'xmit-time': 'uint64',
> > +           'log-dirty-time': 'uint64',
> > +           'migration-bitmap-time': 'uint64',
> > +           'ram-copy-time': 'uint64',
> > +           'checkpoints' : 'uint64',
> > +           'copy-mbps': 'number' }}
> Again, it helps to document the fields in the same order as they are
> declared (no, it's not a hard requirement, but being nice to readers is
> always worth the effort).

Acknowledged.

> > +
> > +##
> >   # @MigrationInfo
> >   #
> >   # Information about current migration process.
> > @@ -624,6 +654,8 @@
> >   #                migration statistics, only returned if XBZRLE feature is on 
> > and
> >   #                status is 'active' or 'completed' (since 1.2)
> >   #
> > +# @mc: #options @MCStats containing details Micro-Checkpointing statistics
> s/options/optional/ - I'm assuming it is optional because it only
> appears when MC is in use.
>
> 'mc' is a rather short name, maybe 'micro-checkpoint' is better?

Funny. I thought 'micro-checkpoint' was too long, particularly
in the ./configure output and the QEMU Monitor 'help' command output =)

> Missing a '(since 2.1)' designation (or 2.x, as you used above as a
> placeholder, although obviously we'd fix the .x before actually bringing
> into mainline)

Acknowledged.

- Michael