| Message ID | 1587352003-3312-2-git-send-email-wei.w.wang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration xbzrle changes | expand |
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
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
* 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
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
* 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
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
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,