Re: [Qemu-devel] [PATCH 09/11] Add migration accounting for normal and duplicate pages

[Orit Wasserman <owasserm@redhat.com>]

On 07/30/2012 10:30 PM, Luiz Capitulino wrote:
> On Sun, 29 Jul 2012 12:43:01 +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      |   38 ++++++++++++++++++++++++++++++++++++++
>>  migration.c      |    4 ++++
>>  migration.h      |    5 +++++
>>  qapi-schema.json |    8 ++++++--
>>  qmp-commands.hx  |   14 +++++++++++---
>>  5 files changed, 64 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index d709ccb..7f12317 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -199,6 +199,40 @@ int64_t xbzrle_cache_resize(int64_t new_size)
>>      return pow2floor(new_size);
>>  }
>>  
>> +/* accounting for migration statistics */
>> +typedef struct AccountingInfo {
>> +    uint64_t dup_pages;
>> +    uint64_t norm_pages;
>> +    uint64_t iterations;
>> +} AccountingInfo;
>> +
>> +static AccountingInfo acct_info;
>> +
>> +static void acct_clear(void)
>> +{
>> +    memset(&acct_info, 0, sizeof(acct_info));
>> +}
>> +
>> +uint64_t dup_mig_bytes_transferred(void)
>> +{
>> +    return acct_info.dup_pages * TARGET_PAGE_SIZE;
>> +}
>> +
>> +uint64_t dup_mig_pages_transferred(void)
>> +{
>> +    return acct_info.dup_pages;
>> +}
>> +
>> +uint64_t norm_mig_bytes_transferred(void)
>> +{
>> +    return acct_info.norm_pages * TARGET_PAGE_SIZE;
>> +}
>> +
>> +uint64_t norm_mig_pages_transferred(void)
>> +{
>> +    return acct_info.norm_pages;
>> +}
>> +
>>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>          int cont, int flag)
>>  {
>> @@ -293,6 +327,7 @@ static int ram_save_block(QEMUFile *f)
>>              p = memory_region_get_ram_ptr(mr) + offset;
>>  
>>              if (is_dup_page(p)) {
>> +                acct_info.dup_pages++;
>>                  save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
>>                  qemu_put_byte(f, *p);
>>                  bytes_sent = 1;
>> @@ -308,6 +343,7 @@ static int ram_save_block(QEMUFile *f)
>>                  save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
>>                  qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>>                  bytes_sent = TARGET_PAGE_SIZE;
>> +                acct_info.norm_pages++;
>>              }
>>  
>>              /* if page is unmodified, continue to the next */
>> @@ -429,6 +465,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>          }
>>          XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
>>          XBZRLE.current_buf = g_malloc(TARGET_PAGE_SIZE);
>> +        acct_clear();
>>      }
>>  
>>      /* Make sure all dirty bits are set */
>> @@ -477,6 +514,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>              break;
>>          }
>>          bytes_transferred += bytes_sent;
>> +        acct_info.iterations++;
>>          /* we want to check in the 1st loop, just in case it was the 1st time
>>             and we had to sync the dirty bitmap.
>>             qemu_get_clock_ns() is a bit expensive, so we only check each some
>> diff --git a/migration.c b/migration.c
>> index bc2231d..4dc99ba 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -156,6 +156,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>          info->ram->total = ram_bytes_total();
>>          info->ram->total_time = qemu_get_clock_ms(rt_clock)
>>              - s->total_time;
>> +        info->ram->duplicate = dup_mig_pages_transferred();
>> +        info->ram->normal = norm_mig_pages_transferred();
>>  
>>          if (blk_mig_active()) {
>>              info->has_disk = true;
>> @@ -175,6 +177,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>          info->ram->remaining = 0;
>>          info->ram->total = ram_bytes_total();
>>          info->ram->total_time = s->total_time;
>> +        info->ram->duplicate = dup_mig_pages_transferred();
>> +        info->ram->normal = norm_mig_pages_transferred();
>>          break;
>>      case MIG_STATE_ERROR:
>>          info->has_status = true;
>> diff --git a/migration.h b/migration.h
>> index 337e225..e4a7cd7 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -87,6 +87,11 @@ uint64_t ram_bytes_total(void);
>>  
>>  extern SaveVMHandlers savevm_ram_handlers;
>>  
>> +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);
>> +
>>  /**
>>   * @migrate_add_blocker - prevent migration from proceeding
>>   *
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 04adcee..a936714 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -264,11 +264,15 @@
>>  #        migration has ended, it returns the total migration
>>  #        time. (since 1.2)
>>  #
>> -# Since: 0.14.0.
>> +# @duplicate: number of duplicate pages (since 1.2)
>> +#
>> +# @normal : number of normal pages (since 1.2)
> 
> Is the number of pages actually useful? I think this should be in bytes,
> just like the other stats.
> 
Why? the pages are always the same size (4k), and what is interesting here is the number of the normal
pages compared to the duplicate pages not the total bytes.
In XBZRLE there is meaning both for the bytes and the number of pages because the page size is not fixed.

Orit
>> +#
>> +# Since: 0.14.0
>>  ##
>>  { 'type': 'MigrationStats',
>>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>> -           'total_time': 'int' } }
>> +           'total_time': 'int', 'duplicate': 'int', 'normal': 'int' } }
>>  
>>  ##
>>  # @MigrationInfo
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index cfc950e..a5a67eb 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2099,6 +2099,8 @@ The main json-object contains the following:
>>           - "transferred": amount transferred (json-int)
>>           - "remaining": amount remaining (json-int)
>>           - "total": total (json-int)
>> +	 - "duplicate": number of duplicated pages (json-int)
>> +	 - "normal" : number of normal pages transferred (json-int)
>>  - "disk": only present if "status" is "active" and it is a block migration,
>>    it is a json-object with the following disk information (in bytes):
>>           - "transferred": amount transferred (json-int)
>> @@ -2119,7 +2121,9 @@ Examples:
>>          "ram":{
>>            "transferred":123,
>>            "remaining":123,
>> -          "total":246
>> +          "total":246,
>> +          "duplicate":123,
>> +          "normal":123
>>          }
>>       }
>>     }
>> @@ -2138,7 +2142,9 @@ Examples:
>>           "ram":{
>>              "transferred":123,
>>              "remaining":123,
>> -            "total":246
>> +            "total":246,
>> +            "duplicate":123,
>> +            "normal":123
>>           }
>>        }
>>     }
>> @@ -2152,7 +2158,9 @@ Examples:
>>           "ram":{
>>              "total":1057024,
>>              "remaining":1053304,
>> -            "transferred":3720
>> +            "transferred":3720,
>> +            "duplicate":123,
>> +            "normal":123
>>           },
>>           "disk":{
>>              "total":20971520,
>