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

[Orit Wasserman]

On 08/01/2012 10:08 PM, Eric Blake wrote:
> 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?
I will remove it.
> 
>> +++ 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?
I will remove the comment
Orit
> 
>>  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.
>