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

[Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>]

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.