Re: [Qemu-devel] [PATCH v2 11/13] hbitmap: serialization

[Fam Zheng]

On Tue, 01/26 12:01, John Snow wrote:
> 
> 
> On 01/20/2016 01:11 AM, Fam Zheng wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> > 
> > Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
> > saved to linear sequence of bits independently of endianness and bitmap
> > array element (unsigned long) size. Therefore Little Endian is chosen.
> > 
> > These functions are appropriate for dirty bitmap migration, restoring
> > the bitmap in several steps is available. To save performance, every
> > step writes only the last level of the bitmap. All other levels are
> > restored by hbitmap_deserialize_finish() as a last step of restoring.
> > So, HBitmap is inconsistent while restoring.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > [Fix left shift operand to 1UL; add "finish" parameter. - Fam]
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  include/qemu/hbitmap.h |  78 ++++++++++++++++++++++++++++
> >  util/hbitmap.c         | 135 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 213 insertions(+)
> > 
> > diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> > index 595ad65..00dbb60 100644
> > --- a/include/qemu/hbitmap.h
> > +++ b/include/qemu/hbitmap.h
> > @@ -149,6 +149,84 @@ void hbitmap_reset_all(HBitmap *hb);
> >  bool hbitmap_get(const HBitmap *hb, uint64_t item);
> >  
> >  /**
> > + * hbitmap_serialization_granularity:
> > + * @hb: HBitmap to operate on.
> > + *
> > + * Grunularity of serialization chunks, used by other serializetion 
> > functions.
> 
> "Granularity," "serialization."
> 
> Perhaps we should be consistent with "hbitmap" vs "HBitmap" as well, too.
> 
> > + * For every chunk:
> > + * 1. Chunk start should be aligned to this granularity.
> > + * 2. Chunk size should be aligned too, except for last chunk (for which
> > + *      start + count == hb->size)
> > + */
> > +uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
> > +
> > +/**
> > + * hbitmap_data_size:
> > + * @hb: HBitmap to operate on.
> > + * @count: Number of bits
> > + *
> > + * Return amount of bytes hbitmap_(de)serialize_part needs
> > + */
> 
> "number of bytes" -- amount is for uncountable nouns (like "water.")
> 
> > +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> > +                                    uint64_t start, uint64_t count);
> > +
> > +/**
> > + * hbitmap_serialize_part
> > + * @hb: HBitmap to operate on.
> > + * @buf: Buffer to store serialized bitmap.
> > + * @start: First bit to store.
> > + * @count: Number of bits to store.
> > + *
> > + * Stores HBitmap data corresponding to given region. The format of saved 
> > data
> > + * is linear sequence of bits, so it can be used by 
> > hbitmap_deserialize_part
> > + * independently of endianness and size of HBitmap level array elements
> > + */
> > +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> > +                            uint64_t start, uint64_t count);
> > +
> > +/**
> > + * hbitmap_deserialize_part
> > + * @hb: HBitmap to operate on.
> > + * @buf: Buffer to restore bitmap data from.
> > + * @start: First bit to restore.
> > + * @count: Number of bits to restore.
> > + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> > + *
> > + * Restores HBitmap data corresponding to given region. The format is the 
> > same
> > + * as for hbitmap_serialize_part.
> > + *
> > + * If @finish is false, caller must call hbitmap_serialize_finish before 
> > using
> > + * the bitmap.
> > + */
> > +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> > +                              uint64_t start, uint64_t count,
> > +                              bool finish);
> > +
> > +/**
> > + * hbitmap_deserialize_zeroes
> > + * @hb: HBitmap to operate on.
> > + * @start: First bit to restore.
> > + * @count: Number of bits to restore.
> > + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> > + *
> > + * Fills the bitmap with zeroes.
> > + *
> > + * If @finish is false, caller must call hbitmap_serialize_finish before 
> > using
> > + * the bitmap.
> > + */
> > +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t 
> > count,
> > +                                bool finish);
> > +
> > +/**
> > + * hbitmap_deserialize_finish
> > + * @hb: HBitmap to operate on.
> > + *
> > + * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all 
> > HBitmap
> > + * layers are restored here.
> > + */
> > +void hbitmap_deserialize_finish(HBitmap *hb);
> > +
> > +/**
> >   * hbitmap_free:
> >   * @hb: HBitmap to operate on.
> >   *
> > diff --git a/util/hbitmap.c b/util/hbitmap.c
> > index 79f6236..1e49ab7 100644
> > --- a/util/hbitmap.c
> > +++ b/util/hbitmap.c
> > @@ -397,6 +397,141 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
> >      return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) 
> > != 0;
> >  }
> >  
> > +uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
> > +{
> > +    return 64 << hb->granularity;
> > +}
> > +
> 
> Sorry, about to be confused about what this function is meant to do,
> please bear with me:
> 
> Normally the granularity specifies the mapping of "interface" bits to
> "implementation" bits. I've never came up with good terminology for
> this, but if the user asks for 128 bits and a granularity of 2, we
> secretly only allocate 32 bits.
> 
> So "virtually" we have 128, and "physically" we have 32.
> 
> What is this function giving us? for the size=128,g=2 example above this
> gives us (64 << 2 == 256), which I guess is the number of "virtual" bits
> represented by each physical uint64_t.
> 
> 64 bits, each bit represents 2^g==2^2==4 bits, so 256 bits total for a
> uint64_t.
> 
> If I'm right, perhaps just a brief comment to help me remember it in the
> future?

OK, I'll add a comment here.

> 
> Further: this function does not appear to be used for any reason other
> than making sure the alignment is correct, so it appears that:
> 
> - On a 64bit host, an el is 64 bits and we will serialize like that
> - On a 32bit host, an el is 32 bits and we'll serialize in multiples of
> 32 bits
> - After transfer, though, the destination host may deserialize assuming
> either 32bit or 64bit elements -- hence aligning to 64bits is the safest
> bet.
> 
> Correct?

Yes.