From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBLCq-0000L4-S0 for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:29:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBLCn-0001Gw-Jo for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:29:44 -0500 Received: from mx2.parallels.com ([199.115.105.18]:35893) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBLCn-0001Gr-Eh for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:29:41 -0500 Message-ID: <54B64508.70407@parallels.com> Date: Wed, 14 Jan 2015 13:29:28 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1418307457-25996-1-git-send-email-vsementsov@parallels.com> <1418307457-25996-5-git-send-email-vsementsov@parallels.com> <54AEF4E8.5070106@redhat.com> <54B516C6.7010403@parallels.com> <54B55118.1020607@redhat.com> In-Reply-To: <54B55118.1020607@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/9] hbitmap: store / restore List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, den@openvz.org, stefanha@redhat.com, "pbonzini >> Paolo Bonzini" 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.