On 07/23/2018 04:05 PM, Peter Xu wrote:
On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
On 07/23/2018 12:36 PM, Peter Xu wrote:
On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@xxxxxxxxx wrote:
@@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
rs->xbzrle_cache_miss_prev) / iter_count;
rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
}
+
+ if (migrate_use_compression()) {
+ uint64_t comp_pages;
+
+ compression_counters.busy_rate = (double)(compression_counters.busy -
+ rs->compress_thread_busy_prev) / iter_count;
Here I'm not sure it's correct...
"iter_count" stands for ramstate.iterations. It's increased per
ram_find_and_save_block(), so IMHO it might contain multiple guest
ram_find_and_save_block() returns if a page is successfully posted and
it only posts 1 page out at one time.
ram_find_and_save_block() calls ram_save_host_page(), and we should be
sending multiple guest pages in ram_save_host_page() if the host page
is a huge page?
You're right, thank you for pointing it out.
So, how about introduce a filed, posted_pages, into RAMState that is used
to track total pages posted out.
Then will use this filed to replace 'iter_count' for compression and use
'RAMState.posted_pages - ram_counters.duplicate' to calculate
xbzrle_cache_miss as the zero page is not handled by xbzrle.
Or introduce a new function, total_posted_pages, which returns the
sum of all page counts:
static total_posted_pages(void)
{
return ram_counters.normal + ram_counters.duplicate + compression_counters.pages
+ xbzrle_counters.pages;
}
that would be a bit more complexity...
pages. However compression_counters.busy should be per guest page.
Actually, it's derived from xbzrle_counters.cache_miss_rate:
xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
rs->xbzrle_cache_miss_prev) / iter_count;
Then this is suspecious to me too...
+ rs->compress_thread_busy_prev = compression_counters.busy;
+
+ comp_pages = compression_counters.pages - rs->compress_pages_prev;
+ if (comp_pages) {
+ compression_counters.compression_rate =
+ (double)(compression_counters.reduced_size -
+ rs->compress_reduced_size_prev) /
+ (comp_pages * TARGET_PAGE_SIZE);
+ rs->compress_pages_prev = compression_counters.pages;
+ rs->compress_reduced_size_prev = compression_counters.reduced_size;
+ }
+ }
}
static void migration_bitmap_sync(RAMState *rs)
@@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
qemu_mutex_lock(&comp_param[idx].mutex);
if (!comp_param[idx].quit) {
len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
+ /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+ compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
I would agree with Dave here - why we store the "reduced size" instead
of the size of the compressed data (which I think should be len - 8)?
len-8 is the size of data after compressed rather than the data improved
by compression that is not straightforward for the user to see how much
the improvement is by applying compression.
Hmm... but it is not a big deal to me... :)
Yeah it might be a personal preference indeed. :)
It's just natural to do that this way for me since AFAIU the
compression ratio is defined as:
compressed data size
compression ratio = ------------------------
original data size
Er, we do it as following:
compression_counters.compression_rate =
(double)(compression_counters.reduced_size -
rs->compress_reduced_size_prev) /
(comp_pages * TARGET_PAGE_SIZE);
We use reduced_size, i.e,:
original data size - compressed data size
compression ratio = ------------------------
original data size
for example, for 100 bytes raw data, if we posted 99 bytes out, then
the compression ration should be 1%.
So if i understand correctly, the reduced_size is really you want? :)
