| Message ID | 1588208375-19556-1-git-send-email-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] migration/xbzrle: add encoding rate | expand |
* Wei Wang (wei.w.wang@intel.com) 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> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/migration.c | 1 + > migration/ram.c | 39 +++++++++++++++++++++++++++++++++++++-- > monitor/hmp-cmds.c | 2 ++ > qapi/migration.json | 5 ++++- > 4 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 187ac0410c..e40421353c 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 04f13feb2e..41b75a0a0f 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 xbzrle encoded bytes since the beginning of the period */ > + uint64_t xbzrle_bytes_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 */ > @@ -726,6 +742,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, > } else if (encoded_len == -1) { > trace_save_xbzrle_page_overflow(); > xbzrle_counters.overflow++; > + xbzrle_counters.bytes += TARGET_PAGE_SIZE; > return -1; > } > > @@ -736,8 +753,12 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, > qemu_put_be16(rs->f, encoded_len); > qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_len); > bytes_xbzrle += encoded_len + 1 + 2; > - xbzrle_counters.pages++; > - xbzrle_counters.bytes += bytes_xbzrle; > + /* > + * Like compressed_size (please see update_compress_thread_counts), > + * the xbzrle encoded bytes don't count the 8 byte header with > + * RAM_SAVE_FLAG_CONTINUE. > + */ > + xbzrle_counters.bytes += bytes_xbzrle - 8; > ram_counters.transferred += bytes_xbzrle; > > return 1; > @@ -870,9 +891,23 @@ static void migration_update_rates(RAMState *rs, int64_t end_time) > } > > if (migrate_use_xbzrle()) { > + double encoded_size, unencoded_size; > + > 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.bytes - rs->xbzrle_bytes_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->xbzrle_bytes_prev = xbzrle_counters.bytes; > } > > if (migrate_use_compression()) { > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 9b94e67879..c2a3a667ae 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 eca2981d0a..358e40226d 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 encoded bytes (since 5.1) > +# > # @overflow: number of overflows > # > # Since: 1.2 > @@ -77,7 +79,7 @@ > { 'struct': 'XBZRLECacheStats', > 'data': {'cache-size': 'int', 'bytes': '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 > # } > # } > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Wei Wang (wei.w.wang@intel.com) 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> Queued > --- > migration/migration.c | 1 + > migration/ram.c | 39 +++++++++++++++++++++++++++++++++++++-- > monitor/hmp-cmds.c | 2 ++ > qapi/migration.json | 5 ++++- > 4 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 187ac0410c..e40421353c 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 04f13feb2e..41b75a0a0f 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 xbzrle encoded bytes since the beginning of the period */ > + uint64_t xbzrle_bytes_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 */ > @@ -726,6 +742,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, > } else if (encoded_len == -1) { > trace_save_xbzrle_page_overflow(); > xbzrle_counters.overflow++; > + xbzrle_counters.bytes += TARGET_PAGE_SIZE; > return -1; > } > > @@ -736,8 +753,12 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, > qemu_put_be16(rs->f, encoded_len); > qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_len); > bytes_xbzrle += encoded_len + 1 + 2; > - xbzrle_counters.pages++; > - xbzrle_counters.bytes += bytes_xbzrle; > + /* > + * Like compressed_size (please see update_compress_thread_counts), > + * the xbzrle encoded bytes don't count the 8 byte header with > + * RAM_SAVE_FLAG_CONTINUE. > + */ > + xbzrle_counters.bytes += bytes_xbzrle - 8; > ram_counters.transferred += bytes_xbzrle; > > return 1; > @@ -870,9 +891,23 @@ static void migration_update_rates(RAMState *rs, int64_t end_time) > } > > if (migrate_use_xbzrle()) { > + double encoded_size, unencoded_size; > + > 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.bytes - rs->xbzrle_bytes_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->xbzrle_bytes_prev = xbzrle_counters.bytes; > } > > if (migrate_use_compression()) { > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 9b94e67879..c2a3a667ae 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 eca2981d0a..358e40226d 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 encoded bytes (since 5.1) > +# > # @overflow: number of overflows > # > # Since: 1.2 > @@ -77,7 +79,7 @@ > { 'struct': 'XBZRLECacheStats', > 'data': {'cache-size': 'int', 'bytes': '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 > # } > # } > -- > 2.20.1 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, 29 Apr 2020 at 18:54, Wei Wang <wei.w.wang@intel.com> wrote: > + 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; > + } With clang 10, this produces CC aarch64-softmmu/migration/ram.o /home/rth/qemu/qemu/migration/ram.c:919:45: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion] xbzrle_counters.encoding_rate = UINT64_MAX; ~ ^~~~~~~~~~ /usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX' # define UINT64_MAX (__UINT64_C(18446744073709551615)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/stdint.h:107:25: note: expanded from macro '__UINT64_C' # define __UINT64_C(c) c ## UL ^~~~~~~ <scratch space>:36:1: note: expanded from here 18446744073709551615UL ^~~~~~~~~~~~~~~~~~~~~~ 1 error generated. UINT64_MAX apprears both arbitrary and wrong. The only way I can imagine encoded_size == 0 is unencoded_size == 0, so 0 seems like the correct answer. Moreover, it really seems like the first test sufficiently covers that possibility. In addition, the only user of this value is > + monitor_printf(mon, "xbzrle encoding rate: %0.2f\n", > + info->xbzrle_cache->encoding_rate); which would be quite happy to print NaN even if the 0/0 computation were to run. Though as I say above, I don't think that's reachable. r~
On 06/04/2020 03:28 AM, Richard Henderson wrote: > On Wed, 29 Apr 2020 at 18:54, Wei Wang <wei.w.wang@intel.com> wrote: >> + 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; >> + } > With clang 10, this produces > > CC aarch64-softmmu/migration/ram.o > /home/rth/qemu/qemu/migration/ram.c:919:45: error: implicit conversion > from 'unsigned long' to 'double' changes value from > 18446744073709551615 to 18446744073709551616 > [-Werror,-Wimplicit-int-float-conversion] > xbzrle_counters.encoding_rate = UINT64_MAX; > ~ ^~~~~~~~~~ > /usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX' > # define UINT64_MAX (__UINT64_C(18446744073709551615)) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > /usr/include/stdint.h:107:25: note: expanded from macro '__UINT64_C' > # define __UINT64_C(c) c ## UL > ^~~~~~~ > <scratch space>:36:1: note: expanded from here > 18446744073709551615UL > ^~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. > > UINT64_MAX apprears both arbitrary and wrong. > > The only way I can imagine encoded_size == 0 is unencoded_size == 0, > so 0 seems like the correct answer. Moreover, it really seems like > the first test sufficiently covers that possibility. It is possible that encoded_size==0, but unencoded_size !=0. For example, a page is written with the same data that it already has. > > In addition, the only user of this value is > >> + monitor_printf(mon, "xbzrle encoding rate: %0.2f\n", >> + info->xbzrle_cache->encoding_rate); > which would be quite happy to print NaN even if the 0/0 computation > were to run. Though as I say above, I don't think that's reachable. The encoding_rate is expected to reflect if the page is xbzrle encoding friendly. The larger, the more friendly, so 0 might not be a good representation here. Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue? Best, Wei
On 6/3/20 7:58 PM, Wei Wang wrote: > It is possible that encoded_size==0, but unencoded_size !=0. For example, > a page is written with the same data that it already has. That really contains 0 bytes? Not even the ones that say "same data"? You certainly have a magical compression algorithm there. Or bad accounting. > The encoding_rate is expected to reflect if the page is xbzrle encoding friendly. > The larger, the more friendly, so 0 might not be a good representation here. > > Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue? ~0ull is no different than UINT64_MAX -- indeed, they are *exactly* the same value -- and is not an exactly representible floating-point value. If unencoded_size != 0, and (somehow) encoded_size == 0, then unencoded_size / encoded_size = Inf which is indeed the limit of x -> 0, n / x. Which is *also* printable by %0.2f. I still contend that the middle if should be removed, and you should print out whatever's left. Either NaN or Inf is instructive. Certainly nothing in the middle cares about the actual value. r~
On 06/04/2020 11:22 AM, Richard Henderson wrote: > On 6/3/20 7:58 PM, Wei Wang wrote: >> It is possible that encoded_size==0, but unencoded_size !=0. For example, >> a page is written with the same data that it already has. > That really contains 0 bytes? > Not even the ones that say "same data"? Yes. It's a just delta operation, the diff (encoded_size) is 0 in that case. > > You certainly have a magical compression algorithm there. > Or bad accounting. > >> The encoding_rate is expected to reflect if the page is xbzrle encoding friendly. >> The larger, the more friendly, so 0 might not be a good representation here. >> >> Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue? > ~0ull is no different than UINT64_MAX -- indeed, they are *exactly* the same > value -- and is not an exactly representible floating-point value. > > If unencoded_size != 0, and (somehow) encoded_size == 0, then > > unencoded_size / encoded_size = Inf > > which is indeed the limit of x -> 0, n / x. > > Which is *also* printable by %0.2f. > > I still contend that the middle if should be removed, and you should print out > whatever's left. Either NaN or Inf is instructive. Certainly nothing in the > middle cares about the actual value. > OK, leave it as "inf" looks good to me. Will send a patch to remove the middle. Thanks! Best, Wei
* Richard Henderson (richard.henderson@linaro.org) wrote: > On 6/3/20 7:58 PM, Wei Wang wrote: > > It is possible that encoded_size==0, but unencoded_size !=0. For example, > > a page is written with the same data that it already has. > > That really contains 0 bytes? > Not even the ones that say "same data"? > > You certainly have a magical compression algorithm there. > Or bad accounting. We just don't bother sending the page at all in the case it's not changed; no headers, no nothing: if (encoded_len == 0) { trace_save_xbzrle_page_skipping(); return 0; and that's xbzrle having correctly done it's job. > > The encoding_rate is expected to reflect if the page is xbzrle encoding friendly. > > The larger, the more friendly, so 0 might not be a good representation here. > > > > Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue? > > ~0ull is no different than UINT64_MAX -- indeed, they are *exactly* the same > value -- and is not an exactly representible floating-point value. > > If unencoded_size != 0, and (somehow) encoded_size == 0, then > > unencoded_size / encoded_size = Inf > > which is indeed the limit of x -> 0, n / x. > > Which is *also* printable by %0.2f. > > I still contend that the middle if should be removed, and you should print out > whatever's left. Either NaN or Inf is instructive. Certainly nothing in the > middle cares about the actual value. Hmm OK; I'll admit to not liking NaN/Inf in output. Dave > > r~ > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/04/2020 05:38 PM, Dr. David Alan Gilbert wrote: > * Richard Henderson (richard.henderson@linaro.org) wrote: >> On 6/3/20 7:58 PM, Wei Wang wrote: >>> It is possible that encoded_size==0, but unencoded_size !=0. For example, >>> a page is written with the same data that it already has. >> That really contains 0 bytes? >> Not even the ones that say "same data"? >> >> You certainly have a magical compression algorithm there. >> Or bad accounting. > We just don't bother sending the page at all in the case it's not > changed; no headers, no nothing: > > if (encoded_len == 0) { > trace_save_xbzrle_page_skipping(); > return 0; > > and that's xbzrle having correctly done it's job. > > >>> The encoding_rate is expected to reflect if the page is xbzrle encoding friendly. >>> The larger, the more friendly, so 0 might not be a good representation here. >>> >>> Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue? >> ~0ull is no different than UINT64_MAX -- indeed, they are *exactly* the same >> value -- and is not an exactly representible floating-point value. >> >> If unencoded_size != 0, and (somehow) encoded_size == 0, then >> >> unencoded_size / encoded_size = Inf >> >> which is indeed the limit of x -> 0, n / x. >> >> Which is *also* printable by %0.2f. >> >> I still contend that the middle if should be removed, and you should print out >> whatever's left. Either NaN or Inf is instructive. Certainly nothing in the >> middle cares about the actual value. > Hmm OK; I'll admit to not liking NaN/Inf in output. > > Dave > OK. To deal with the reported issue, how about using FLT_MAX (as opposed to UINT64_MAX or inf): xbzrle_counters.encoding_rate = FLT_MAX; Best, Wei
On 6/4/20 3:27 AM, Wei Wang wrote: > On 06/04/2020 05:38 PM, Dr. David Alan Gilbert wrote: >> Hmm OK; I'll admit to not liking NaN/Inf in output. >> >> Dave >> > > OK. To deal with the reported issue, how about using FLT_MAX (as opposed to > UINT64_MAX or inf): > xbzrle_counters.encoding_rate = FLT_MAX; So you'd rather see 340282346638528859811704183484516925440.00 printed? It's arbitrary and not correct in any mathematical sense. If you *really* insist on not printing Inf (which may have some diagnostic value), then 0 is just as arbitrary, and at least smaller in the output. r~
On 06/05/2020 12:57 AM, Richard Henderson wrote: > On 6/4/20 3:27 AM, Wei Wang wrote: >> On 06/04/2020 05:38 PM, Dr. David Alan Gilbert wrote: >>> Hmm OK; I'll admit to not liking NaN/Inf in output. >>> >>> Dave >>> >> OK. To deal with the reported issue, how about using FLT_MAX (as opposed to >> UINT64_MAX or inf): >> xbzrle_counters.encoding_rate = FLT_MAX; > So you'd rather see 340282346638528859811704183484516925440.00 printed? > > It's arbitrary and not correct in any mathematical sense. > > If you *really* insist on not printing Inf (which may have some diagnostic > value), then 0 is just as arbitrary, and at least smaller in the output. 0 works fine (though it logically means the lowest encoding rate). I slightly prefer the biggest number or inf, which naturally means it's very encoding friendly. Let's see if Dave has any thought about the choices :) Best, Wei
* Wei Wang (wei.w.wang@intel.com) wrote: > On 06/05/2020 12:57 AM, Richard Henderson wrote: > > On 6/4/20 3:27 AM, Wei Wang wrote: > > > On 06/04/2020 05:38 PM, Dr. David Alan Gilbert wrote: > > > > Hmm OK; I'll admit to not liking NaN/Inf in output. > > > > > > > > Dave > > > > > > > OK. To deal with the reported issue, how about using FLT_MAX (as opposed to > > > UINT64_MAX or inf): > > > xbzrle_counters.encoding_rate = FLT_MAX; > > So you'd rather see 340282346638528859811704183484516925440.00 printed? > > > > It's arbitrary and not correct in any mathematical sense. > > > > If you *really* insist on not printing Inf (which may have some diagnostic > > value), then 0 is just as arbitrary, and at least smaller in the output. > > 0 works fine (though it logically means the lowest encoding rate). > I slightly prefer the biggest number or inf, which naturally means it's very > encoding friendly. > Let's see if Dave has any thought about the choices :) 0 is fine for me. Dave > Best, > Wei > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/migration.c b/migration/migration.c index 187ac0410c..e40421353c 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 04f13feb2e..41b75a0a0f 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 xbzrle encoded bytes since the beginning of the period */ + uint64_t xbzrle_bytes_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 */ @@ -726,6 +742,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, } else if (encoded_len == -1) { trace_save_xbzrle_page_overflow(); xbzrle_counters.overflow++; + xbzrle_counters.bytes += TARGET_PAGE_SIZE; return -1; } @@ -736,8 +753,12 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data, qemu_put_be16(rs->f, encoded_len); qemu_put_buffer(rs->f, XBZRLE.encoded_buf, encoded_len); bytes_xbzrle += encoded_len + 1 + 2; - xbzrle_counters.pages++; - xbzrle_counters.bytes += bytes_xbzrle; + /* + * Like compressed_size (please see update_compress_thread_counts), + * the xbzrle encoded bytes don't count the 8 byte header with + * RAM_SAVE_FLAG_CONTINUE. + */ + xbzrle_counters.bytes += bytes_xbzrle - 8; ram_counters.transferred += bytes_xbzrle; return 1; @@ -870,9 +891,23 @@ static void migration_update_rates(RAMState *rs, int64_t end_time) } if (migrate_use_xbzrle()) { + double encoded_size, unencoded_size; + 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.bytes - rs->xbzrle_bytes_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->xbzrle_bytes_prev = xbzrle_counters.bytes; } if (migrate_use_compression()) { diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 9b94e67879..c2a3a667ae 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 eca2981d0a..358e40226d 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 encoded bytes (since 5.1) +# # @overflow: number of overflows # # Since: 1.2 @@ -77,7 +79,7 @@ { 'struct': 'XBZRLECacheStats', 'data': {'cache-size': 'int', 'bytes': '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 # } # }