From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXTkz-0007VV-R8 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:41:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXTky-0002UP-LK for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:41:17 -0400 Date: Wed, 10 Aug 2016 15:41:06 +0200 From: Kevin Wolf Message-ID: <20160810134106.GB4665@noname.redhat.com> References: <1470668720-211300-1-git-send-email-vsementsov@virtuozzo.com> <1470668720-211300-2-git-send-email-vsementsov@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470668720-211300-2-git-send-email-vsementsov@virtuozzo.com> Subject: Re: [Qemu-devel] [PATCH 01/29] hbitmap: fix dirty iter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben: > If dirty bitmap was cleared during iterator life, we can went to zero > current in hbitmap_iter_skip_words, starting from saved (and currently > wrong hbi->cur[...]). I was going to suggest improved grammar, but actually I find this description hard to understand even with correct grammar. If I understand correctly, hbi->cur contains a copy of hb->levels that isn't updated after the iterator entered the subtree, because this way we avoid going backwards if concurrent operations set a new bit. However, in the opposite case of clearing a bit, we actually want to use the new value because we wouldn't find anything to return when going to the lower levels. This is what the & does in your patch. Is this explanation reasonably correct? If so, maybe you can try to make these points in the commit message in the next version. > Signed-off-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Denis V. Lunev > --- > util/hbitmap.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/util/hbitmap.c b/util/hbitmap.c > index 6a13c12..f807f64 100644 > --- a/util/hbitmap.c > +++ b/util/hbitmap.c > @@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi) > > unsigned long cur; > do { > - cur = hbi->cur[--i]; > + i--; > pos >>= BITS_PER_LEVEL; > + cur = hbi->cur[i] & hb->levels[i][pos]; > } while (cur == 0); I think you will want to update the comment for hbitmap_iter_init. This is what it says today: * Concurrent setting of bits is acceptable, and will at worst cause the * iteration to miss some of those bits. Resetting bits before the current * position of the iterator is also okay. However, concurrent resetting of * bits can lead to unexpected behavior if the iterator has not yet reached * those bits. Kevin