Re: [Qemu-devel] [PATCH 10/11] Add XBZRLE statistics

[Eric Blake]

On 08/01/2012 12:01 PM, Juan Quintela wrote:
> From: Orit Wasserman <address@hidden>
> 
> Signed-off-by: Benoit Hudzia <address@hidden>
> Signed-off-by: Petter Svard <address@hidden>
> Signed-off-by: Aidan Shribman <address@hidden>
> Signed-off-by: Orit Wasserman <address@hidden>
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  arch_init.c      | 28 ++++++++++++++++++++++++++++
>  hmp.c            | 13 +++++++++++++
>  migration.c      | 17 +++++++++++++++++
>  migration.h      |  4 ++++
>  qapi-schema.json | 28 +++++++++++++++++++++++++++-
>  qmp-commands.hx  | 36 +++++++++++++++++++++++++++++++++++-
>  6 files changed, 124 insertions(+), 2 deletions(-)
> 

>  ##
> +# @XBZRLECacheStats
> +#
> +# Detailed XBZRLE migration cache statistics
> +#
> +# @cache-size: XBZRLE cache size
> +#
> +# @xbzrle-bytes: amount of bytes already transferred to the target VM
> +#
> +# @xbzrle-pages: amount of pages transferred to the target VM
> +#
> +# @xbzrle-cache-miss: @optional, number of cache miss
> +#
> +# @xbzrle-overflow: number of overflows
> +#
> +# Since: 1.2
> +##
> +{ 'type': 'XBZRLECacheStats',
> +  'data': {'cache-size': 'int', 'xbzrle-bytes': 'int', 'xbzrle-pages': 'int',
> +           'xbzrle-cache-miss': 'int', 'xbzrle-overflow': 'int' } }

Given that this struct already has xbzrle in the name, do all the fields
need to repeat that information?

> +++ b/qmp-commands.hx
> @@ -2128,7 +2128,17 @@ The main json-object contains the following:
>           - "transferred": amount transferred (json-int)
>           - "remaining": amount remaining (json-int)
>           - "total": total (json-int)
> -
> +- "xbzrle-cache": only present if XBZRLE is active.
> +  It is a json-object with the following XBZRLE information:
> +         - "cache-size": XBZRLE cache size
> +         - "xbzrle-bytes": total XBZRLE bytes transferred, only present if
> +                           status is "active" or "completed"
> +         - "xbzrle-pages": number of XBZRLE compressed pages, only present if
> +                           status is "active" or "completed"
> +         - "cache-miss": number of cache misses, only present if

s/cache-miss/xbzrle-cache-miss/ (unless you heed my earlier complaint,
then s/xbzrle-bytes/bytes/ instead)

> +                           status is "active" or "completed"
> +         - "overflow": number of XBZRLE overflows, only present if

s/overflow/xbzrle-overflow/

> +                           status is "active" or "completed"

Given that the overall 'xbzrle-cache' struct is already documented as
present when XBZRLE is active, do you need to also document that the
subfields are present when active or completed?

>  Examples:
> 
>  1. Before the first migration
> @@ -2196,6 +2206,30 @@ Examples:
>        }
>     }
> 
> +6. Migration is being performed and XBZRLE is active:
> +
> +-> { "execute": "query-migrate" }
> +<- {
> +      "return":{
> +         "status":"active",
> +         "capabilities" : [ { "capability": "xbzrle", "state" : true } ],
> +         "ram":{
> +            "total":1057024,
> +            "remaining":1053304,
> +            "transferred":3720,
> +            "duplicate": 10,
> +            "normal" : 3333

Inconsistent spacing around these ':'.

> +         },
> +         "cache":{

s/cache/xbzrle-cache/

> +            "cache-size": 1024

s/ 1024/1024,/

> +            "xbzrle-transferred":20971520,
> +            "xbzrle-pages":2444343,
> +            "xbzrle-cache-miss":2244,
> +            "xbzrle-overflow":34434

and if you take my earlier comments, these lines would change too.

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature