On Mon, Jul 23, 2018 at 04:40:29PM +0800, Xiao Guangrong wrote:
>
>
> 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...
If below [1] is wrong too, then I'm thinking whether we could just
move the rs->iterations++ from ram_save_iterate() loop to
ram_save_host_page() loop, then we possibly fix both places...
After all I don't see any other usages of rs->iterations so it seems
fine. Dave/Juan?
>
> > >
> > > > 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...
[1]
> >
> > >
> > > > > + 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%.
I think it should be 99%...
>
> So if i understand correctly, the reduced_size is really you want? :)
>
Here's the "offical" definition. :)
https://en.wikipedia.org/wiki/Data_compression_ratio
But obviously I reverted the molecular and denominator... So maybe we
can follow what the wiki page said (then I think you'll just store the
summation of len-8)?
Regards,
--
Peter Xu
