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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

On 02/18/2014 01:50 AM, mrhines@linux.vnet.ibm.com wrote:
> From: "Michael R. Hines" <mrhines@us.ibm.com>
> 
> MC provides a lot of new information, including the same RAM statistics
> that ordinary migration does, so we centralize a lot of that printing
> code into a common function so that the QMP printing statements don't
> get duplicated too much.
> 
> We also introduce a new MCStats structure (like MigrationStats) due
> to the large number of non-migration related statistics - don't want
> to confuse migration and MC too much, so let's keep them separate for now.
> 
> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
> ---

> +++ 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'.

> +#
> +# @checkpoints: cummulative total number of MCs generated 

More trailing whitespace.  Please run your series through
scripts/checkpatch.pl.

s/cummulative total/cumulative/

> +#
> +# 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).

> +
> +##
>  # @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?

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)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]