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

[Orit Wasserman <owasserm@redhat.com>]

On 07/27/2012 01:48 AM, Eric Blake wrote:
> On 07/25/2012 08:50 AM, Orit Wasserman wrote:
>> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
>> Signed-off-by: Petter Svard <petters@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> +++ 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.
> 
ok
>> +#
>> +# @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.
If the migration is not active only cache size will be available (requested by Luiz).
I will add it to the comment.
> 
>>  
>>  ##
>>  # @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).
ok
> 
>> +++ 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.
ok
> 
>> +  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?
xbzrle-bytes are the compressed bytes sent on the wire.
total is the total bytes sent on the wire (including xbzrle_bytes and duplicates ...).
> 
> 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.
total will be always bigger.
> 
>> +         - "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.
> 
Thanks,
Orit