Re: [Qemu-devel] [PATCH 4/9] hbitmap: store / restore

[Vladimir Sementsov-Ogievskiy]

Best regards,
Vladimir

On 13.01.2015 20:08, John Snow wrote:
> On 01/13/2015 07:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> > On 09.01.2015 00:21, John Snow wrote:
> > > On 12/11/2014 09:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> ....
> > > > +/**
> > > > + * hbitmap_restore_finish
> > > > + * @hb: HBitmap to operate on.
> > > > + *
> > > > + * Repair HBitmap after calling hbitmap_restore_data. Actuall all
> > > > HBitmap
> > > > + * layers are restore here.
> > > > + */
> > > > +void hbitmap_restore_finish(HBitmap *hb);
> > > > +
> > > > +/**
> > > >    * hbitmap_free:
> > > >    * @hb: HBitmap to operate on.
> > > >    *
> > >
> > > These are just biased opinions:
> > >
> > > - It might be nice to name the store/restore functions "serialize" and
> > > "deserialize," to describe exactly what they are doing.
> > >
> > > - I might refer to "restore_finish" as "post_load" or "post_restore"
> > > or something similar to mimic how device migration functions are
> > > named. I think hbitmap_restore_data_finalize would also be fine; the
> > > key part here is clearly naming it relative to "restore_data."
> >
> > Hmm. Ok, what about the following set:
> >      hbitmap_serialize()
> >      hbitmap_deserialize_part()
> >      hbitmap_deserialize_finish()
>
> Looks good to me!
* hbitmap_serialize_part() is better to be similar with its pair function
> > > > diff --git a/util/hbitmap.c b/util/hbitmap.c
> > > > index 8aa7406..ac0323f 100644
> > > > --- a/util/hbitmap.c
> > > > +++ b/util/hbitmap.c
> > > > @@ -362,6 +362,90 @@ bool hbitmap_get(const HBitmap *hb, uint64_t 
> > > > item)
> > > >       return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] &
> > > > bit) != 0;
> > > >   }
> > > >
> > > > +uint64_t hbitmap_data_size(const HBitmap *hb, uint64_t count)
> > > > +{
> > > > +    uint64_t size;
> > > > +
> > > > +    if (count == 0) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    size = (((count - 1) >> hb->granularity) >> BITS_PER_LEVEL) + 1;
> > > > +
> > > > +    return size * sizeof(unsigned long);
> > > > +}
> > > > +
> > >
> > > This seems flawed to me: number of bits without an offset can't be
> > > mapped to a number of real bytes, because "two bits" may or may not
> > > cross a granularity boundary, depending on *WHERE* you start counting.
> > >
> > > e.g.
> > >
> > > granularity = 1 (i.e. 2^1 = 2 virtual bits per 1 real bit)
> > > virtual: 001100
> > > real:     0 1 0
> > >
> > > The amount of space required to hold "two bits" here could be as
> > > little as one bit, if the offset is k={0,2,4} but it could be as much
> > > as two bits if the offset is k={1,3}.
> > >
> > > You may never use the function in this way, but mapping virtual bits
> > > to an implementation byte-size seems like it is inviting an
> > > inconsistency.
> > I dislike this function too.. But unfortunately we need the size in
> > bytes used for serialization.
> >
> > Hmm. Ok, without loss of generality, let offset be less than
> > granularity. The precise formula should look like:
> >
> > size = (((offset+count-1) >> hb->granularity) >> BITS_PER_LEVEL);
> >
> > So,
> > size = ((((1 << hb->granularity) + count - 2) >> hb->granularity) >>
> > BITS_PER_LEVEL) + 1;
> > would be enough in any case. Ok?
>
> I think so, as long as when you deserialize the object it does so 
> correctly, regardless of whether or not you start on an even multiple 
> of the granularity.
May be, also rename hbitmap_data_size to hbitmap_serialize_size or even 
drop it and make function hbitmap_store_data() return the necessary size 
when *buf is NULL.