diff mbox

[10/11] Add XBZRLE statistics

Message ID 1343227834-5400-12-git-send-email-owasserm@redhat.com
State New
Headers show

Commit Message

Orit Wasserman July 25, 2012, 2:50 p.m. UTC
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 |   26 +++++++++++++++++++++++++-
 qmp-commands.hx  |   32 +++++++++++++++++++++++++++++++-
 6 files changed, 137 insertions(+), 2 deletions(-)

Comments

Eric Blake July 26, 2012, 10:48 p.m. UTC | #1
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.

> +#
> +# @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.
Orit Wasserman July 29, 2012, 7:10 a.m. UTC | #2
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
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index e50cdf2..f484bd5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -209,7 +209,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;
@@ -239,6 +243,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)
 {
@@ -267,6 +291,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;
     }
 
@@ -284,6 +309,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;
@@ -301,7 +327,9 @@  static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
     qemu_put_byte(f, hdr.xh_flags);
     qemu_put_be16(f, hdr.xh_len);
     qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
+    acct_info.xbzrle_pages++;
     bytes_sent = encoded_len + sizeof(hdr);
+    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();
+        }
         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 88b7b56..9418f23 100644
--- a/qapi-schema.json
+++ 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
+#
+# @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' } }
 
 ##
 # @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)
+#
 # 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 d0e8878..946d921 100644
--- a/qmp-commands.hx
+++ 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.
+  It is a json-object with the following XBZRLE information:
+         - "cache-size": XBZRLE cache size
+         - "xbzrle-bytes": total XBZRLE bytes transferred
+         - "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" }
 <- { "return": {} }
 
@@ -2164,6 +2170,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
 
     {