Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

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

On 04/24/2020 06:47 PM, Dr. David Alan Gilbert wrote:
> * Wei Wang (wei.w.wang@intel.com) wrote:
>> On 04/22/2020 03:21 AM, Dr. David Alan Gilbert wrote:
>>> * Wei Wang (wei.w.wang@intel.com) wrote:
>>>> Like compressed_size which indicates how many bytes are compressed, we
>>>> need encoded_size to understand how many bytes are encoded with xbzrle
>>>> during migration.
>>>>
>>>> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
>>>> because we don't find a usage of xbzrle_counter.bytes currently, which
>>>> includes 3 more bytes of the migration transfer protocol header (in
>>>> addition to the encoding header). The encoded_size will further be used
>>>> to calculate the encoding rate.
>>>>
>>>> Signed-off-by: Yi Sun <yi.y.sun@intel.com>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> Can you explain why these 3 bytes matter?  Certainly the 2 bytes of the
>>> encoded_len are an overhead that's a cost of using XBZRLE; so if you're
>>> trying to figure out whether xbzrle is worth it, then you should include
>>> those 2 bytes in the cost.
>>> That other byte, that holds ENCODING_FLAG_XBZRLE also seems to be pure
>>> oerhead of XBZRLE; so your cost of using XBZRLE really does include
>>> those 3 bytes.
>>>
>>> SO to me it makes sense to include the 3 bytes as it currently does.
>>>
>>> Dave
>> Thanks Dave for sharing your thoughts.
>>
>> We hope to do a fair comparison of compression rate and xbzrle encoding
>> rate.
>> The current compression_rate doesn't include the migration flag overhead
>> (please see
>> update_compress thread_counts() ). So for xbzrle encoding rate, we wanted it
>> not include the migration
>> protocol flags as well (but the 2 bytes xbzrle encoding overhead is kept
>> there, as the compression rate
>> includes the compression header overhead).
>>
>> Or would you think it is necessary to add the migration flag (8 bytes) for
>> compression
>> when calculating the compression rate?
> I don't think the migration flag (8 bytes) matters, because everyone has
> that; but isn't this patch about the 3 bytes (1 byte
> ENCONDING_FLAG_XBZRLE) (2 byte encoded_len) ?
>
> The 2 byte encoded_len in this code, corresponds to the 4 byte blen in
> qemu_put_compression_data;  I'm not sure but I think that 4 bytes is
> included in the length update_compress_thread_counts() sees - if so
> that makes it equivalent including the length.
>

Right, that makes sense, thanks.

Best,
Wei