Re: [PATCH v2 3/8] migration: show the statistics of compression

[Xiao Guangrong <guangrong.xiao@gmail.com>]

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@gmail.com 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? :)