[PATCH v1 0/2] Migration xbzrle changes

[PATCH v1 0/2] Migration xbzrle changes

[Wei Wang]

This patches change/add some xbzrle statistics to give users some useful
feedbacks about the delta operations.

Wei Wang (2):
  migration/xbzrle: replace transferred xbzrle bytes with encoded bytes
  migration/xbzrle: add encoding rate

 migration/migration.c |  3 ++-
 migration/ram.c       | 49 +++++++++++++++++++++++++++++++++----------
 monitor/hmp-cmds.c    |  6 ++++--
 qapi/migration.json   | 11 ++++++----
 slirp                 |  2 +-
 5 files changed, 52 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Wei Wang]

Like compressed_size which indicates how many bytes are compressed, we
need encoded_size to understand how many bytes are encoded with xbzrle
during migration.

Replace the old xbzrle_counter.bytes, instead of adding a new counter,
because we don't find a usage of xbzrle_counter.bytes currently, which
includes 3 more bytes of the migration transfer protocol header (in
addition to the encoding header). The encoded_size will further be used
to calculate the encoding rate.

Signed-off-by: Yi Sun <yi.y.sun@intel.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 migration/migration.c |  2 +-
 migration/ram.c       | 18 +++++++++---------
 monitor/hmp-cmds.c    |  4 ++--
 qapi/migration.json   |  6 +++---
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 187ac0410c..8e7ee0d76b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -926,7 +926,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         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->bytes = xbzrle_counters.bytes;
+        info->xbzrle_cache->encoded_size = xbzrle_counters.encoded_size;
         info->xbzrle_cache->pages = xbzrle_counters.pages;
         info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
         info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
diff --git a/migration/ram.c b/migration/ram.c
index 04f13feb2e..bca5878f25 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -677,7 +677,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
                             ram_addr_t current_addr, RAMBlock *block,
                             ram_addr_t offset, bool last_stage)
 {
-    int encoded_len = 0, bytes_xbzrle;
+    int encoded_size = 0, bytes_xbzrle;
     uint8_t *prev_cached_page;
 
     if (!cache_is_cached(XBZRLE.cache, current_addr,
@@ -702,7 +702,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
 
     /* XBZRLE encoding (if there is no overflow) */
-    encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
+    encoded_size = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
                                        TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
                                        TARGET_PAGE_SIZE);
 
@@ -710,7 +710,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
      * Update the cache contents, so that it corresponds to the data
      * sent, in all cases except where we skip the page.
      */
-    if (!last_stage && encoded_len != 0) {
+    if (!last_stage && encoded_size != 0) {
         memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
         /*
          * In the case where we couldn't compress, ensure that the caller
@@ -720,10 +720,10 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
         *current_data = prev_cached_page;
     }
 
-    if (encoded_len == 0) {
+    if (encoded_size == 0) {
         trace_save_xbzrle_page_skipping();
         return 0;
-    } else if (encoded_len == -1) {
+    } else if (encoded_size == -1) {
         trace_save_xbzrle_page_overflow();
         xbzrle_counters.overflow++;
         return -1;
@@ -733,11 +733,11 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     bytes_xbzrle = save_page_header(rs, rs->f, block,
                                     offset | RAM_SAVE_FLAG_XBZRLE);
     qemu_put_byte(rs->f, ENCODING_FLAG_XBZRLE);
-    qemu_put_be16(rs->f, encoded_len);
-    qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_len);
-    bytes_xbzrle += encoded_len + 1 + 2;
+    qemu_put_be16(rs->f, encoded_size);
+    qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_size);
+    bytes_xbzrle += encoded_size + 1 + 2;
     xbzrle_counters.pages++;
-    xbzrle_counters.bytes += bytes_xbzrle;
+    xbzrle_counters.encoded_size += encoded_size;
     ram_counters.transferred += bytes_xbzrle;
 
     return 1;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9b94e67879..6d3b35ca64 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -295,8 +295,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
     if (info->has_xbzrle_cache) {
         monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
                        info->xbzrle_cache->cache_size);
-        monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
-                       info->xbzrle_cache->bytes >> 10);
+        monitor_printf(mon, "xbzrle encoded size: %" PRIu64 " kbytes\n",
+                       info->xbzrle_cache->encoded_size >> 10);
         monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
                        info->xbzrle_cache->pages);
         monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
diff --git a/qapi/migration.json b/qapi/migration.json
index eca2981d0a..bf195ff6ac 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -62,7 +62,7 @@
 #
 # @cache-size: XBZRLE cache size
 #
-# @bytes: amount of bytes already transferred to the target VM
+# @encoded_size: amount of bytes encoded
 #
 # @pages: amount of pages transferred to the target VM
 #
@@ -75,7 +75,7 @@
 # Since: 1.2
 ##
 { 'struct': 'XBZRLECacheStats',
-  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
+  'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
            'cache-miss': 'int', 'cache-miss-rate': 'number',
            'overflow': 'int' } }
 
@@ -333,7 +333,7 @@
 #          },
 #          "xbzrle-cache":{
 #             "cache-size":67108864,
-#             "bytes":20971520,
+#             "encoded_size":20971520,
 #             "pages":2444343,
 #             "cache-miss":2244,
 #             "cache-miss-rate":0.123,
-- 
2.20.1

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Daniel P. Berrangé]

On Mon, Apr 20, 2020 at 11:06:42AM +0800, Wei Wang wrote:
> Like compressed_size which indicates how many bytes are compressed, we
> need encoded_size to understand how many bytes are encoded with xbzrle
> during migration.
> 
> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
> because we don't find a usage of xbzrle_counter.bytes currently, which
> includes 3 more bytes of the migration transfer protocol header (in
> addition to the encoding header). The encoded_size will further be used
> to calculate the encoding rate.
> 
> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  migration/migration.c |  2 +-
>  migration/ram.c       | 18 +++++++++---------
>  monitor/hmp-cmds.c    |  4 ++--
>  qapi/migration.json   |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)


> diff --git a/qapi/migration.json b/qapi/migration.json
> index eca2981d0a..bf195ff6ac 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -62,7 +62,7 @@
>  #
>  # @cache-size: XBZRLE cache size
>  #
> -# @bytes: amount of bytes already transferred to the target VM
> +# @encoded_size: amount of bytes encoded

Woah, this is part of QEMU's public API, so it isn't permissible to just
arbitrarily remove a field with no warning, and replace it with a new
field reporting different data. Adding a new field is allowed, but any
existing field should be deprecated first, if there is a genuine need
to remove it. If it isn't costly though, just leave the existing field
unchanged.

I would also note that the other fields in this struct use a hyphen, not
an underscore.

>  #
>  # @pages: amount of pages transferred to the target VM
>  #
> @@ -75,7 +75,7 @@
>  # Since: 1.2
>  ##
>  { 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> +  'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>             'overflow': 'int' } }
>  
> @@ -333,7 +333,7 @@
>  #          },
>  #          "xbzrle-cache":{
>  #             "cache-size":67108864,
> -#             "bytes":20971520,
> +#             "encoded_size":20971520,
>  #             "pages":2444343,
>  #             "cache-miss":2244,
>  #             "cache-miss-rate":0.123,
> -- 
> 2.20.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Wei Wang]

On 04/20/2020 05:29 PM, Daniel P. Berrangé wrote:
> On Mon, Apr 20, 2020 at 11:06:42AM +0800, Wei Wang wrote:
>> Like compressed_size which indicates how many bytes are compressed, we
>> need encoded_size to understand how many bytes are encoded with xbzrle
>> during migration.
>>
>> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
>> because we don't find a usage of xbzrle_counter.bytes currently, which
>> includes 3 more bytes of the migration transfer protocol header (in
>> addition to the encoding header). The encoded_size will further be used
>> to calculate the encoding rate.
>>
>> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>>   migration/migration.c |  2 +-
>>   migration/ram.c       | 18 +++++++++---------
>>   monitor/hmp-cmds.c    |  4 ++--
>>   qapi/migration.json   |  6 +++---
>>   4 files changed, 15 insertions(+), 15 deletions(-)
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index eca2981d0a..bf195ff6ac 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -62,7 +62,7 @@
>>   #
>>   # @cache-size: XBZRLE cache size
>>   #
>> -# @bytes: amount of bytes already transferred to the target VM
>> +# @encoded_size: amount of bytes encoded
> Woah, this is part of QEMU's public API, so it isn't permissible to just
> arbitrarily remove a field with no warning, and replace it with a new
> field reporting different data. Adding a new field is allowed, but any
> existing field should be deprecated first, if there is a genuine need
> to remove it. If it isn't costly though, just leave the existing field
> unchanged.
>
> I would also note that the other fields in this struct use a hyphen, not
> an underscore.
OK. Thanks for reviewing and pointing it out.
We can add it as a new filed using hyphen in this case.
Will wait for other comments to post out a new version.

Best,
Wei

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Dr. David Alan Gilbert]

* Wei Wang (wei.w.wang@intel.com) wrote:
> Like compressed_size which indicates how many bytes are compressed, we
> need encoded_size to understand how many bytes are encoded with xbzrle
> during migration.
> 
> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
> because we don't find a usage of xbzrle_counter.bytes currently, which
> includes 3 more bytes of the migration transfer protocol header (in
> addition to the encoding header). The encoded_size will further be used
> to calculate the encoding rate.
> 
> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

Can you explain why these 3 bytes matter?  Certainly the 2 bytes of the
encoded_len are an overhead that's a cost of using XBZRLE; so if you're
trying to figure out whether xbzrle is worth it, then you should include
those 2 bytes in the cost.
That other byte, that holds ENCODING_FLAG_XBZRLE also seems to be pure
oerhead of XBZRLE; so your cost of using XBZRLE really does include
those 3 bytes.

SO to me it makes sense to include the 3 bytes as it currently does.

Dave

> ---
>  migration/migration.c |  2 +-
>  migration/ram.c       | 18 +++++++++---------
>  monitor/hmp-cmds.c    |  4 ++--
>  qapi/migration.json   |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 187ac0410c..8e7ee0d76b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -926,7 +926,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>          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->bytes = xbzrle_counters.bytes;
> +        info->xbzrle_cache->encoded_size = xbzrle_counters.encoded_size;
>          info->xbzrle_cache->pages = xbzrle_counters.pages;
>          info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
>          info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
> diff --git a/migration/ram.c b/migration/ram.c
> index 04f13feb2e..bca5878f25 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -677,7 +677,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>                              ram_addr_t current_addr, RAMBlock *block,
>                              ram_addr_t offset, bool last_stage)
>  {
> -    int encoded_len = 0, bytes_xbzrle;
> +    int encoded_size = 0, bytes_xbzrle;
>      uint8_t *prev_cached_page;
>  
>      if (!cache_is_cached(XBZRLE.cache, current_addr,
> @@ -702,7 +702,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
>  
>      /* XBZRLE encoding (if there is no overflow) */
> -    encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
> +    encoded_size = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
>                                         TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
>                                         TARGET_PAGE_SIZE);
>  
> @@ -710,7 +710,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>       * Update the cache contents, so that it corresponds to the data
>       * sent, in all cases except where we skip the page.
>       */
> -    if (!last_stage && encoded_len != 0) {
> +    if (!last_stage && encoded_size != 0) {
>          memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
>          /*
>           * In the case where we couldn't compress, ensure that the caller
> @@ -720,10 +720,10 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>          *current_data = prev_cached_page;
>      }
>  
> -    if (encoded_len == 0) {
> +    if (encoded_size == 0) {
>          trace_save_xbzrle_page_skipping();
>          return 0;
> -    } else if (encoded_len == -1) {
> +    } else if (encoded_size == -1) {
>          trace_save_xbzrle_page_overflow();
>          xbzrle_counters.overflow++;
>          return -1;
> @@ -733,11 +733,11 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      bytes_xbzrle = save_page_header(rs, rs->f, block,
>                                      offset | RAM_SAVE_FLAG_XBZRLE);
>      qemu_put_byte(rs->f, ENCODING_FLAG_XBZRLE);
> -    qemu_put_be16(rs->f, encoded_len);
> -    qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_len);
> -    bytes_xbzrle += encoded_len + 1 + 2;
> +    qemu_put_be16(rs->f, encoded_size);
> +    qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_size);
> +    bytes_xbzrle += encoded_size + 1 + 2;
>      xbzrle_counters.pages++;
> -    xbzrle_counters.bytes += bytes_xbzrle;
> +    xbzrle_counters.encoded_size += encoded_size;
>      ram_counters.transferred += bytes_xbzrle;
>  
>      return 1;
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 9b94e67879..6d3b35ca64 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -295,8 +295,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>      if (info->has_xbzrle_cache) {
>          monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
>                         info->xbzrle_cache->cache_size);
> -        monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
> -                       info->xbzrle_cache->bytes >> 10);
> +        monitor_printf(mon, "xbzrle encoded size: %" PRIu64 " kbytes\n",
> +                       info->xbzrle_cache->encoded_size >> 10);
>          monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
>                         info->xbzrle_cache->pages);
>          monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
> diff --git a/qapi/migration.json b/qapi/migration.json
> index eca2981d0a..bf195ff6ac 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -62,7 +62,7 @@
>  #
>  # @cache-size: XBZRLE cache size
>  #
> -# @bytes: amount of bytes already transferred to the target VM
> +# @encoded_size: amount of bytes encoded
>  #
>  # @pages: amount of pages transferred to the target VM
>  #
> @@ -75,7 +75,7 @@
>  # Since: 1.2
>  ##
>  { 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> +  'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>             'overflow': 'int' } }
>  
> @@ -333,7 +333,7 @@
>  #          },
>  #          "xbzrle-cache":{
>  #             "cache-size":67108864,
> -#             "bytes":20971520,
> +#             "encoded_size":20971520,
>  #             "pages":2444343,
>  #             "cache-miss":2244,
>  #             "cache-miss-rate":0.123,
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Wei Wang]

On 04/22/2020 03:21 AM, Dr. David Alan Gilbert wrote:
> * Wei Wang (wei.w.wang@intel.com) wrote:
>> Like compressed_size which indicates how many bytes are compressed, we
>> need encoded_size to understand how many bytes are encoded with xbzrle
>> during migration.
>>
>> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
>> because we don't find a usage of xbzrle_counter.bytes currently, which
>> includes 3 more bytes of the migration transfer protocol header (in
>> addition to the encoding header). The encoded_size will further be used
>> to calculate the encoding rate.
>>
>> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Can you explain why these 3 bytes matter?  Certainly the 2 bytes of the
> encoded_len are an overhead that's a cost of using XBZRLE; so if you're
> trying to figure out whether xbzrle is worth it, then you should include
> those 2 bytes in the cost.
> That other byte, that holds ENCODING_FLAG_XBZRLE also seems to be pure
> oerhead of XBZRLE; so your cost of using XBZRLE really does include
> those 3 bytes.
>
> SO to me it makes sense to include the 3 bytes as it currently does.
>
> Dave

Thanks Dave for sharing your thoughts.

We hope to do a fair comparison of compression rate and xbzrle encoding 
rate.
The current compression_rate doesn't include the migration flag overhead 
(please see
update_compress thread_counts() ). So for xbzrle encoding rate, we 
wanted it not include the migration
protocol flags as well (but the 2 bytes xbzrle encoding overhead is kept 
there, as the compression rate
includes the compression header overhead).

Or would you think it is necessary to add the migration flag (8 bytes) 
for compression
when calculating the compression rate?

Best,
Wei

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Dr. David Alan Gilbert]

* Wei Wang (wei.w.wang@intel.com) wrote:
> On 04/22/2020 03:21 AM, Dr. David Alan Gilbert wrote:
> > * Wei Wang (wei.w.wang@intel.com) wrote:
> > > Like compressed_size which indicates how many bytes are compressed, we
> > > need encoded_size to understand how many bytes are encoded with xbzrle
> > > during migration.
> > > 
> > > Replace the old xbzrle_counter.bytes, instead of adding a new counter,
> > > because we don't find a usage of xbzrle_counter.bytes currently, which
> > > includes 3 more bytes of the migration transfer protocol header (in
> > > addition to the encoding header). The encoded_size will further be used
> > > to calculate the encoding rate.
> > > 
> > > Signed-off-by: Yi Sun <yi.y.sun@intel.com>
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Can you explain why these 3 bytes matter?  Certainly the 2 bytes of the
> > encoded_len are an overhead that's a cost of using XBZRLE; so if you're
> > trying to figure out whether xbzrle is worth it, then you should include
> > those 2 bytes in the cost.
> > That other byte, that holds ENCODING_FLAG_XBZRLE also seems to be pure
> > oerhead of XBZRLE; so your cost of using XBZRLE really does include
> > those 3 bytes.
> > 
> > SO to me it makes sense to include the 3 bytes as it currently does.
> > 
> > Dave
> 
> Thanks Dave for sharing your thoughts.
> 
> We hope to do a fair comparison of compression rate and xbzrle encoding
> rate.
> The current compression_rate doesn't include the migration flag overhead
> (please see
> update_compress thread_counts() ). So for xbzrle encoding rate, we wanted it
> not include the migration
> protocol flags as well (but the 2 bytes xbzrle encoding overhead is kept
> there, as the compression rate
> includes the compression header overhead).
> 
> Or would you think it is necessary to add the migration flag (8 bytes) for
> compression
> when calculating the compression rate?

I don't think the migration flag (8 bytes) matters, because everyone has
that; but isn't this patch about the 3 bytes (1 byte
ENCONDING_FLAG_XBZRLE) (2 byte encoded_len) ?

The 2 byte encoded_len in this code, corresponds to the 4 byte blen in
qemu_put_compression_data;  I'm not sure but I think that 4 bytes is
included in the length update_compress_thread_counts() sees - if so
that makes it equivalent including the length.

Dave


> Best,
> Wei
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Wei Wang]

On 04/24/2020 06:47 PM, Dr. David Alan Gilbert wrote:
> * Wei Wang (wei.w.wang@intel.com) wrote:
>> On 04/22/2020 03:21 AM, Dr. David Alan Gilbert wrote:
>>> * Wei Wang (wei.w.wang@intel.com) wrote:
>>>> Like compressed_size which indicates how many bytes are compressed, we
>>>> need encoded_size to understand how many bytes are encoded with xbzrle
>>>> during migration.
>>>>
>>>> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
>>>> because we don't find a usage of xbzrle_counter.bytes currently, which
>>>> includes 3 more bytes of the migration transfer protocol header (in
>>>> addition to the encoding header). The encoded_size will further be used
>>>> to calculate the encoding rate.
>>>>
>>>> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> Can you explain why these 3 bytes matter?  Certainly the 2 bytes of the
>>> encoded_len are an overhead that's a cost of using XBZRLE; so if you're
>>> trying to figure out whether xbzrle is worth it, then you should include
>>> those 2 bytes in the cost.
>>> That other byte, that holds ENCODING_FLAG_XBZRLE also seems to be pure
>>> oerhead of XBZRLE; so your cost of using XBZRLE really does include
>>> those 3 bytes.
>>>
>>> SO to me it makes sense to include the 3 bytes as it currently does.
>>>
>>> Dave
>> Thanks Dave for sharing your thoughts.
>>
>> We hope to do a fair comparison of compression rate and xbzrle encoding
>> rate.
>> The current compression_rate doesn't include the migration flag overhead
>> (please see
>> update_compress thread_counts() ). So for xbzrle encoding rate, we wanted it
>> not include the migration
>> protocol flags as well (but the 2 bytes xbzrle encoding overhead is kept
>> there, as the compression rate
>> includes the compression header overhead).
>>
>> Or would you think it is necessary to add the migration flag (8 bytes) for
>> compression
>> when calculating the compression rate?
> I don't think the migration flag (8 bytes) matters, because everyone has
> that; but isn't this patch about the 3 bytes (1 byte
> ENCONDING_FLAG_XBZRLE) (2 byte encoded_len) ?
>
> The 2 byte encoded_len in this code, corresponds to the 4 byte blen in
> qemu_put_compression_data;  I'm not sure but I think that 4 bytes is
> included in the length update_compress_thread_counts() sees - if so
> that makes it equivalent including the length.
>

Right, that makes sense, thanks.

Best,
Wei

[PATCH v1 2/2] migration/xbzrle: add encoding rate

[Wei Wang]

Users may need to check the xbzrle encoding rate to know if the guest
memory is xbzrle encoding-friendly, and dynamically turn off the
encoding if the encoding rate is low.

Signed-off-by: Yi Sun <yi.y.sun@intel.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 migration/migration.c |  1 +
 migration/ram.c       | 31 +++++++++++++++++++++++++++++--
 monitor/hmp-cmds.c    |  2 ++
 qapi/migration.json   |  5 ++++-
 slirp                 |  2 +-
 5 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 8e7ee0d76b..84a556a4ac 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -930,6 +930,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->xbzrle_cache->pages = xbzrle_counters.pages;
         info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
         info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
+        info->xbzrle_cache->encoding_rate = xbzrle_counters.encoding_rate;
         info->xbzrle_cache->overflow = xbzrle_counters.overflow;
     }
 
diff --git a/migration/ram.c b/migration/ram.c
index bca5878f25..c87c38ec80 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -327,6 +327,10 @@ struct RAMState {
     uint64_t num_dirty_pages_period;
     /* xbzrle misses since the beginning of the period */
     uint64_t xbzrle_cache_miss_prev;
+    /* Amount of xbzrle pages since the beginning of the period */
+    uint64_t xbzrle_pages_prev;
+    /* Amount of encoded bytes since the beginning of the period */
+    uint64_t encoded_size_prev;
 
     /* compression statistics since the beginning of the period */
     /* amount of count that no free thread to compress data */
@@ -696,6 +700,18 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
         return -1;
     }
 
+    /*
+     * Reaching here means the page has hit the xbzrle cache, no matter what
+     * encoding result it is (normal encoding, overflow or skipping the page),
+     * count the page as encoded. This is used to caculate the encoding rate.
+     *
+     * Example: 2 pages (8KB) being encoded, first page encoding generates 2KB,
+     * 2nd page turns out to be skipped (i.e. no new bytes written to the
+     * page), the overall encoding rate will be 8KB / 2KB = 4, which has the
+     * skipped page included. In this way, the encoding rate can tell if the
+     * guest page is good for xbzrle encoding.
+     */
+    xbzrle_counters.pages++;
     prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
 
     /* save current buffer into memory */
@@ -736,7 +752,6 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
     qemu_put_be16(rs->f, encoded_size);
     qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_size);
     bytes_xbzrle += encoded_size + 1 + 2;
-    xbzrle_counters.pages++;
     xbzrle_counters.encoded_size += encoded_size;
     ram_counters.transferred += bytes_xbzrle;
 
@@ -859,7 +874,7 @@ uint64_t ram_get_total_transferred_pages(void)
 static void migration_update_rates(RAMState *rs, int64_t end_time)
 {
     uint64_t page_count = rs->target_page_count - rs->target_page_count_prev;
-    double compressed_size;
+    double compressed_size, encoded_size, unencoded_size;
 
     /* calculate period counters */
     ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
@@ -873,6 +888,18 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
             rs->xbzrle_cache_miss_prev) / page_count;
         rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
+        unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
+                         TARGET_PAGE_SIZE;
+        encoded_size = xbzrle_counters.encoded_size - rs->encoded_size_prev;
+        if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
+            xbzrle_counters.encoding_rate = 0;
+        } else if (!encoded_size) {
+            xbzrle_counters.encoding_rate = UINT64_MAX;
+        } else {
+            xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
+        }
+        rs->xbzrle_pages_prev = xbzrle_counters.pages;
+        rs->encoded_size_prev = xbzrle_counters.encoded_size;
     }
 
     if (migrate_use_compression()) {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 6d3b35ca64..07f41e582f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -303,6 +303,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->cache_miss);
         monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
                        info->xbzrle_cache->cache_miss_rate);
+        monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
+                       info->xbzrle_cache->encoding_rate);
         monitor_printf(mon, "xbzrle overflow: %" PRIu64 "\n",
                        info->xbzrle_cache->overflow);
     }
diff --git a/qapi/migration.json b/qapi/migration.json
index bf195ff6ac..ee4513c565 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -70,6 +70,8 @@
 #
 # @cache-miss-rate: rate of cache miss (since 2.1)
 #
+# @encoding-rate: rate of cache miss
+#
 # @overflow: number of overflows
 #
 # Since: 1.2
@@ -77,7 +79,7 @@
 { 'struct': 'XBZRLECacheStats',
   'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
            'cache-miss': 'int', 'cache-miss-rate': 'number',
-           'overflow': 'int' } }
+           'encoding-rate': 'number', 'overflow': 'int' } }
 
 ##
 # @CompressionStats:
@@ -337,6 +339,7 @@
 #             "pages":2444343,
 #             "cache-miss":2244,
 #             "cache-miss-rate":0.123,
+#             "encoding-rate":80.1,
 #             "overflow":34434
 #          }
 #       }
diff --git a/slirp b/slirp
index 55ab21c9a3..126c04acba 160000
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 55ab21c9a36852915b81f1b41ebaf3b6509dd8ba
+Subproject commit 126c04acbabd7ad32c2b018fe10dfac2a3bc1210
-- 
2.20.1

Re: [PATCH v1 2/2] migration/xbzrle: add encoding rate

[Daniel P. Berrangé]

On Mon, Apr 20, 2020 at 11:06:43AM +0800, Wei Wang wrote:
> Users may need to check the xbzrle encoding rate to know if the guest
> memory is xbzrle encoding-friendly, and dynamically turn off the
> encoding if the encoding rate is low.
> 
> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  migration/migration.c |  1 +
>  migration/ram.c       | 31 +++++++++++++++++++++++++++++--
>  monitor/hmp-cmds.c    |  2 ++
>  qapi/migration.json   |  5 ++++-
>  slirp                 |  2 +-
>  5 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8e7ee0d76b..84a556a4ac 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -930,6 +930,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>          info->xbzrle_cache->pages = xbzrle_counters.pages;
>          info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
>          info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
> +        info->xbzrle_cache->encoding_rate = xbzrle_counters.encoding_rate;
>          info->xbzrle_cache->overflow = xbzrle_counters.overflow;
>      }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index bca5878f25..c87c38ec80 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -327,6 +327,10 @@ struct RAMState {
>      uint64_t num_dirty_pages_period;
>      /* xbzrle misses since the beginning of the period */
>      uint64_t xbzrle_cache_miss_prev;
> +    /* Amount of xbzrle pages since the beginning of the period */
> +    uint64_t xbzrle_pages_prev;
> +    /* Amount of encoded bytes since the beginning of the period */
> +    uint64_t encoded_size_prev;
>  
>      /* compression statistics since the beginning of the period */
>      /* amount of count that no free thread to compress data */
> @@ -696,6 +700,18 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>          return -1;
>      }
>  
> +    /*
> +     * Reaching here means the page has hit the xbzrle cache, no matter what
> +     * encoding result it is (normal encoding, overflow or skipping the page),
> +     * count the page as encoded. This is used to caculate the encoding rate.
> +     *
> +     * Example: 2 pages (8KB) being encoded, first page encoding generates 2KB,
> +     * 2nd page turns out to be skipped (i.e. no new bytes written to the
> +     * page), the overall encoding rate will be 8KB / 2KB = 4, which has the
> +     * skipped page included. In this way, the encoding rate can tell if the
> +     * guest page is good for xbzrle encoding.
> +     */
> +    xbzrle_counters.pages++;
>      prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
>  
>      /* save current buffer into memory */
> @@ -736,7 +752,6 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      qemu_put_be16(rs->f, encoded_size);
>      qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_size);
>      bytes_xbzrle += encoded_size + 1 + 2;
> -    xbzrle_counters.pages++;
>      xbzrle_counters.encoded_size += encoded_size;
>      ram_counters.transferred += bytes_xbzrle;
>  
> @@ -859,7 +874,7 @@ uint64_t ram_get_total_transferred_pages(void)
>  static void migration_update_rates(RAMState *rs, int64_t end_time)
>  {
>      uint64_t page_count = rs->target_page_count - rs->target_page_count_prev;
> -    double compressed_size;
> +    double compressed_size, encoded_size, unencoded_size;
>  
>      /* calculate period counters */
>      ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
> @@ -873,6 +888,18 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>          xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>              rs->xbzrle_cache_miss_prev) / page_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> +        unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
> +                         TARGET_PAGE_SIZE;
> +        encoded_size = xbzrle_counters.encoded_size - rs->encoded_size_prev;
> +        if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
> +            xbzrle_counters.encoding_rate = 0;
> +        } else if (!encoded_size) {
> +            xbzrle_counters.encoding_rate = UINT64_MAX;
> +        } else {
> +            xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
> +        }
> +        rs->xbzrle_pages_prev = xbzrle_counters.pages;
> +        rs->encoded_size_prev = xbzrle_counters.encoded_size;
>      }
>  
>      if (migrate_use_compression()) {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6d3b35ca64..07f41e582f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -303,6 +303,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->xbzrle_cache->cache_miss);
>          monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
>                         info->xbzrle_cache->cache_miss_rate);
> +        monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
> +                       info->xbzrle_cache->encoding_rate);
>          monitor_printf(mon, "xbzrle overflow: %" PRIu64 "\n",
>                         info->xbzrle_cache->overflow);
>      }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bf195ff6ac..ee4513c565 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -70,6 +70,8 @@
>  #
>  # @cache-miss-rate: rate of cache miss (since 2.1)
>  #
> +# @encoding-rate: rate of cache miss
> +#
>  # @overflow: number of overflows
>  #
>  # Since: 1.2
> @@ -77,7 +79,7 @@
>  { 'struct': 'XBZRLECacheStats',
>    'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
>             'cache-miss': 'int', 'cache-miss-rate': 'number',
> -           'overflow': 'int' } }
> +           'encoding-rate': 'number', 'overflow': 'int' } }
>  
>  ##
>  # @CompressionStats:
> @@ -337,6 +339,7 @@
>  #             "pages":2444343,
>  #             "cache-miss":2244,
>  #             "cache-miss-rate":0.123,
> +#             "encoding-rate":80.1,
>  #             "overflow":34434
>  #          }
>  #       }
> diff --git a/slirp b/slirp
> index 55ab21c9a3..126c04acba 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 55ab21c9a36852915b81f1b41ebaf3b6509dd8ba
> +Subproject commit 126c04acbabd7ad32c2b018fe10dfac2a3bc1210

Accidental inclusion of submodule changes

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 2/2] migration/xbzrle: add encoding rate

[Eric Blake]

On 4/19/20 10:06 PM, Wei Wang wrote:
> Users may need to check the xbzrle encoding rate to know if the guest
> memory is xbzrle encoding-friendly, and dynamically turn off the
> encoding if the encoding rate is low.
> 
> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---

In addition to Dan's review comments,

> +++ b/qapi/migration.json
> @@ -70,6 +70,8 @@
>   #
>   # @cache-miss-rate: rate of cache miss (since 2.1)
>   #
> +# @encoding-rate: rate of cache miss

This is missing a '(since 5.1)' tag.

> +#
>   # @overflow: number of overflows
>   #
>   # Since: 1.2
> @@ -77,7 +79,7 @@
>   { 'struct': 'XBZRLECacheStats',
>     'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
>              'cache-miss': 'int', 'cache-miss-rate': 'number',
> -           'overflow': 'int' } }
> +           'encoding-rate': 'number', 'overflow': 'int' } }
>   
>   ##
>   # @CompressionStats:
> @@ -337,6 +339,7 @@
>   #             "pages":2444343,
>   #             "cache-miss":2244,
>   #             "cache-miss-rate":0.123,
> +#             "encoding-rate":80.1,
>   #             "overflow":34434
>   #          }
>   #       }
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v1 2/2] migration/xbzrle: add encoding rate

[Wei Wang]

On 04/20/2020 10:53 PM, Eric Blake wrote:
> On 4/19/20 10:06 PM, Wei Wang wrote:
>> Users may need to check the xbzrle encoding rate to know if the guest
>> memory is xbzrle encoding-friendly, and dynamically turn off the
>> encoding if the encoding rate is low.
>>
>> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>
> In addition to Dan's review comments,
>
>> +++ b/qapi/migration.json
>> @@ -70,6 +70,8 @@
>>   #
>>   # @cache-miss-rate: rate of cache miss (since 2.1)
>>   #
>> +# @encoding-rate: rate of cache miss
>
> This is missing a '(since 5.1)' tag.

OK, will add it, thanks.

Best,
Wei