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

[Dr. David Alan Gilbert]

* Wei Wang (address@hidden) wrote:
> On 04/22/2020 03:21 AM, Dr. David Alan Gilbert wrote:
> > * Wei Wang (address@hidden) 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 <address@hidden>
> > > Signed-off-by: Wei Wang <address@hidden>
> > 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.

Dave


> Best,
> Wei
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK