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

[Wei Wang]

On 04/24/2020 06:47 PM, Dr. David Alan Gilbert wrote:
> * 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.

Right, that makes sense, thanks.

Best,
Wei