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

[Orit Wasserman <owasserm@redhat.com>]

On 07/30/2012 10:37 PM, Luiz Capitulino wrote:
> On Sun, 29 Jul 2012 12:43:02 +0300
> Orit Wasserman <owasserm@redhat.com> 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>
>> ---
>>  arch_init.c      |   28 ++++++++++++++++++++++++++++
>>  hmp.c            |   21 +++++++++++++++++++++
>>  migration.c      |   28 ++++++++++++++++++++++++++++
>>  migration.h      |    4 ++++
>>  qapi-schema.json |   32 +++++++++++++++++++++++++++++++-
>>  qmp-commands.hx  |   35 +++++++++++++++++++++++++++++++++++
>>  6 files changed, 147 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 7f12317..9833d54 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -203,7 +203,11 @@ int64_t xbzrle_cache_resize(int64_t new_size)
>>  typedef struct AccountingInfo {
>>      uint64_t dup_pages;
>>      uint64_t norm_pages;
>> +    uint64_t xbzrle_bytes;
>> +    uint64_t xbzrle_pages;
>> +    uint64_t xbzrle_cache_miss;
>>      uint64_t iterations;
>> +    uint64_t xbzrle_overflows;
>>  } AccountingInfo;
>>  
>>  static AccountingInfo acct_info;
>> @@ -233,6 +237,26 @@ uint64_t norm_mig_pages_transferred(void)
>>      return acct_info.norm_pages;
>>  }
>>  
>> +uint64_t xbzrle_mig_bytes_transferred(void)
>> +{
>> +    return acct_info.xbzrle_bytes;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_transferred(void)
>> +{
>> +    return acct_info.xbzrle_pages;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_cache_miss(void)
>> +{
>> +    return acct_info.xbzrle_cache_miss;
>> +}
>> +
>> +uint64_t xbzrle_mig_pages_overflow(void)
>> +{
>> +    return acct_info.xbzrle_overflows;
>> +}
>> +
>>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>          int cont, int flag)
>>  {
>> @@ -257,6 +281,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>>      if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>>          cache_insert(XBZRLE.cache, current_addr,
>>                       g_memdup(current_data, TARGET_PAGE_SIZE));
>> +        acct_info.xbzrle_cache_miss++;
>>          return -1;
>>      }
>>  
>> @@ -274,6 +299,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>>          return 0;
>>      } else if (encoded_len == -1) {
>>          DPRINTF("Overflow\n");
>> +        acct_info.xbzrle_overflows++;
>>          /* update data in the cache */
>>          memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
>>          return -1;
>> @@ -288,6 +314,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>>      qemu_put_be16(f, encoded_len);
>>      qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
>>      bytes_sent = encoded_len + 1 + 2;
>> +    acct_info.xbzrle_pages++;
>> +    acct_info.xbzrle_bytes += bytes_sent;
>>  
>>      return bytes_sent;
>>  }
>> diff --git a/hmp.c b/hmp.c
>> index 9770d7b..383d5b1 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -172,6 +172,27 @@ void hmp_info_migrate(Monitor *mon)
>>                         info->disk->total >> 10);
>>      }
>>  
>> +    if (info->has_xbzrle_cache) {
>> +        monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
>> +                       info->xbzrle_cache->cache_size);
>> +        if (info->xbzrle_cache->has_xbzrle_bytes) {
>> +            monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
>> +                           info->xbzrle_cache->xbzrle_bytes >> 10);
>> +        }
>> +        if (info->xbzrle_cache->has_xbzrle_pages) {
>> +            monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
>> +                           info->xbzrle_cache->xbzrle_pages);
>> +        }
>> +        if (info->xbzrle_cache->has_xbzrle_cache_miss) {
>> +            monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
>> +                           info->xbzrle_cache->xbzrle_cache_miss);
>> +        }
>> +        if (info->xbzrle_cache->has_xbzrle_overflow) {
>> +            monitor_printf(mon, "xbzrle overflow : %" PRIu64 "\n",
>> +                           info->xbzrle_cache->xbzrle_overflow);
>> +        }
>> +    }
>> +
>>      qapi_free_MigrationInfo(info);
>>      qapi_free_MigrationParameters(params);
>>  }
>> diff --git a/migration.c b/migration.c
>> index 4dc99ba..fb802bc 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -136,6 +136,23 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>      return params;
>>  }
>>  
>> +static void get_xbzrle_cache_stats(MigrationInfo *info)
>> +{
>> +    if (migrate_use_xbzrle()) {
>> +        info->has_xbzrle_cache = true;
>> +        info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
>> +        info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
>> +        info->xbzrle_cache->has_xbzrle_bytes = true;
>> +        info->xbzrle_cache->xbzrle_bytes = xbzrle_mig_bytes_transferred();
>> +        info->xbzrle_cache->has_xbzrle_pages = true;
>> +        info->xbzrle_cache->xbzrle_pages = xbzrle_mig_pages_transferred();
>> +        info->xbzrle_cache->has_xbzrle_cache_miss = true;
>> +        info->xbzrle_cache->xbzrle_cache_miss = xbzrle_mig_pages_cache_miss();
>> +        info->xbzrle_cache->has_xbzrle_overflow = true;
>> +        info->xbzrle_cache->xbzrle_overflow = xbzrle_mig_pages_overflow();
>> +    }
>> +}
>> +
>>  MigrationInfo *qmp_query_migrate(Error **errp)
>>  {
>>      MigrationInfo *info = g_malloc0(sizeof(*info));
>> @@ -144,6 +161,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>      switch (s->state) {
>>      case MIG_STATE_SETUP:
>>          /* no migration has happened ever */
>> +
>> +        /* display xbzrle cache size */
>> +        if (migrate_use_xbzrle()) {
>> +            info->has_xbzrle_cache = true;
>> +            info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
>> +            info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
>> +        }
> 
> Is it really useful to return this in MIG_SETUP? Can't the cache size
> be queried by some query- command?
I like it this way and it is very simple to use, you need to use query-migrate command if you do migration 
anyway.
Otherwise we will need to add a query command for each configurable parameters,
so one for speed and another for downtime and probably more in the future.
This way we use one command to query the migration setup.

Orit
> 
>>          break;
>>      case MIG_STATE_ACTIVE:
>>          info->has_status = true;
>> @@ -166,8 +190,12 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>              info->disk->remaining = blk_mig_bytes_remaining();
>>              info->disk->total = blk_mig_bytes_total();
>>          }
>> +
>> +        get_xbzrle_cache_stats(info);
>>          break;
>>      case MIG_STATE_COMPLETED:
>> +        get_xbzrle_cache_stats(info);
>> +
>>          info->has_status = true;
>>          info->status = g_strdup("completed");
>>  
>> diff --git a/migration.h b/migration.h
>> index e4a7cd7..a9852fc 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -91,6 +91,10 @@ uint64_t dup_mig_bytes_transferred(void);
>>  uint64_t dup_mig_pages_transferred(void);
>>  uint64_t norm_mig_bytes_transferred(void);
>>  uint64_t norm_mig_pages_transferred(void);
>> +uint64_t xbzrle_mig_bytes_transferred(void);
>> +uint64_t xbzrle_mig_pages_transferred(void);
>> +uint64_t xbzrle_mig_pages_overflow(void);
>> +uint64_t xbzrle_mig_pages_cache_miss(void);
>>  
>>  /**
>>   * @migrate_add_blocker - prevent migration from proceeding
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index a936714..91dee72 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -275,6 +275,31 @@
>>             'total_time': 'int', 'duplicate': 'int', 'normal': 'int' } }
>>  
>>  ##
>> +# @XBZRLECacheStats
>> +#
>> +# Detailed XBZRLE migration cache statistics
>> +#
>> +# @cache-size: XBZRLE cache size
>> +#
>> +# @xbzrle-bytes: @optional, amount of bytes already transferred to the target VM
>> +#                only returned when migration is active or completed
> 
> s/xbzrle-bytes/transferred-bytes
> 
>> +#
>> +# @xbzrle-pages: @optional, amount of pages transferred to the target VM
>> +#                only returned when migration is active or completed
> 
> s/xbzrle-pages/transferred-pages
> 
>> +#
>> +# @xbzrle-cache-miss: @optional, number of cache miss
>> +#                     only returned when migration is active or completed
> 
> s/xbzrle/xbzrle//
> 
>> +#
>> +# @xbzrle-overflow: @optional, number of overflows
>> +#                   only returned when migration is active or completed
> 
> s/xbzrle///
> 
> Besides, all these parameters shouldn't be optional as they are unconditionally
> included (BZRLECacheStats itself is optional as returned by query-migrate).
> 
>> +#
>> +# Since: 1.2
>> +##
>> +{ 'type': 'XBZRLECacheStats',
>> +  'data': {'cache-size': 'int', '*xbzrle-bytes': 'int', '*xbzrle-pages': 'int',
>> +           '*xbzrle-cache-miss': 'int', '*xbzrle-overflow': 'int' } }
>> +
>> +##
>>  # @MigrationInfo
>>  #
>>  # Information about current migration process.
>> @@ -292,11 +317,16 @@
>>  #        status, only returned if status is 'active' and it is a block
>>  #        migration
>>  #
>> +# @xbzrle-cache: #optional @XBZRLECacheStats containing detailed XBZRLE
>> +#                migration statistics, only returned if XBZRLE feature is on
>> +#                (since 1.2)
>> +#
>>  # Since: 0.14.0
>>  ##
>>  { 'type': 'MigrationInfo',
>>    'data': {'*status': 'str', '*ram': 'MigrationStats',
>> -           '*disk': 'MigrationStats'} }
>> +           '*disk': 'MigrationStats',
>> +           '*xbzrle-cache': 'XBZRLECacheStats'} }
>>  
>>  ##
>>  # @query-migrate
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index a5a67eb..0546f42 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2106,6 +2106,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
>> +                           status is "active" or "completed"
>> +         - "overflow": number of XBZRLE overflows, only present if
>> +                           status is "active" or "completed"
>>  Examples:
>>  
>>  1. Before the first migration
>> @@ -2170,6 +2181,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
>> +         },
>> +         "cache":{
>> +            "cache-size": 1024
>> +            "xbzrle-transferred":20971520,
>> +            "xbzrle-pages":2444343,
>> +            "xbzrle-cache-miss":2244,
>> +            "xbzrle-overflow":34434
>> +         }
>> +      }
>> +   }
>> +
>>  EQMP
>>  
>>      {
>