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

[Eric Blake]

On 07/25/2012 08:50 AM, Orit Wasserman wrote:
> 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>
> ---
> +++ b/qapi-schema.json
> @@ -273,6 +273,26 @@
>  { 'type': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>             'total_time': 'int', '*duplicate': 'int', '*normal': 'int' } }
> +##
> +# @XBZRLECacheStats
> +#
> +# Detailed XBZRLE migration cache statistics
> +#
> +# @cache_size: XBZRLE cache size

s/cache_size/cache-size/, and so on throughout this struct (it is new,
so it should use '-' instead of '_'); especially since you already made
that change in the JSON itself.

> +#
> +# @xbzrle_bytes: amount of bytes already transferred to the target VM
> +#
> +# @xbzrle_pages: amount of pages transferred to the target VM
> +#
> +# @xbzrle_cache_miss: 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' } }

Why are you marking four of the five fields optional here, but not in
the text above?  I don't think any of them should be optional.

>  
>  ##
>  # @MigrationInfo
> @@ -292,11 +312,15 @@
>  #        status, only returned if status is 'active' and it is a block
>  #        migration
>  #
> +# @xbzrle-cache: #optional @XBZRLECacheStats containing detailed XBZRLE
> +#                migration statistics (since 1.2)

Now _this_ field is indeed optional - it should appear in the output
only when xbzrle is enabled.  You may want to mention that in the
description (just like the previous field mentions that it was only
available for a block migration).

> +++ b/qmp-commands.hx
> @@ -2106,10 +2106,16 @@ The main json-object contains the following:
>           - "transferred": amount transferred (json-int)
>           - "remaining": amount remaining (json-int)
>           - "total": total (json-int)
> +- "cache": only present if "status" and XBZRLE is active.

Naming mismatch; you named it 'xbzrle-cache' above.

> +  It is a json-object with the following XBZRLE information:
> +         - "cache-size": XBZRLE cache size
> +         - "xbzrle-bytes": total XBZRLE bytes transferred

Just so I'm clear, is xbzrle-bytes is the number of compressed bytes
sent over the wire, or the number of uncompressed bytes?  Likewise, at
the top level, is 'total' the number of bytes sent over the wire, or the
number of bytes after decompression?

That is, if I compare this field against 'total', which field will
always be bigger?  Knowing this will let me compute my percentage of
savings due to compressed pages.

> +         - "xbzrle-pages": number of XBZRLE compressed pages
> +         - "cache-miss": number of cache misses
> +         - "overflow": number of XBZRLE overflows
>  Examples:
>  
>  1. Before the first migration
> -
>  -> { "execute": "query-migrate" }

Spurious whitespace change.

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

signature.asc
Description: OpenPGP digital signature