Re: [PATCH v2] migration/xbzrle: add encoding rate

[Wei Wang <wei.w.wang@intel.com>]

On 04/28/2020 10:51 PM, Dr. David Alan Gilbert wrote:
> * 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>
>> ---
>>   migration/migration.c |  1 +
>>   migration/ram.c       | 38 ++++++++++++++++++++++++++++++++++++--
>>   monitor/hmp-cmds.c    |  2 ++
>>   qapi/migration.json   |  5 ++++-
>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>
>> ChangeLog:
>> - include the 3 bytes (ENCODING_FLAG_XBZRLE flag and encoded_len) when
>>    calculating the encoding rate. Similar to the compress rate
>>    calculation, the 8 byte RAM_SAVE_FLAG_CONTINUE flag isn't included in
>>    the calculation.
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 187ac04..e404213 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 04f13fe..f46ab96 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++;
> Can you explain how that works with overflow?  Doesn't overflow return
> -1 here, not increment the bytes, so it looks like you've xbzrle'd a
> page, but the encoding rate hasn't incremented.

OK. How about adding below before returning -1 (for the overflow case):

...
xbzrle_counters.bytes += TARGET_PAGE_SIZE;
return -1;

Example: if we have 2 pages encoded as below:
4KB--> after normal encoding: 2KB
4KB--> after overflow: 4KB (will be sent as non-encoded page)
then the encoding rate is 8KB / 6KB = ~1.3
(if we skip the counting of the overflow case,
the encoding rate will be 4KB/ 2KB=2. Users may think that's
good to go with xbzrle, unless they do another calculation with
checking the overflow numbers themselves)

Best,
Wei