|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-devel] [PATCH 4/9] hbitmap: store / restore |
Date: | Wed, 14 Jan 2015 13:29:28 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
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
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.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.
[Prev in Thread] | Current Thread | [Next in Thread] |