From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50774) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXU2y-0006cP-A6 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:59:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXU2t-0008VZ-Po for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:59:52 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:23443 helo=relay.sw.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXU2t-0008VT-CK for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:59:47 -0400 References: <1470668720-211300-1-git-send-email-vsementsov@virtuozzo.com> <1470668720-211300-2-git-send-email-vsementsov@virtuozzo.com> <20160810134106.GB4665@noname.redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <57AB3343.3000101@virtuozzo.com> Date: Wed, 10 Aug 2016 16:59:31 +0300 MIME-Version: 1.0 In-Reply-To: <20160810134106.GB4665@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/29] hbitmap: fix dirty iter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf 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 On 10.08.2016 16:41, Kevin Wolf wrote: > 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. Something like this, yes. > >> 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. Aha, cool. We've missed this. So this patch is a new feature but not a bug fix. I will update this comment. > > Kevin -- Best regards, Vladimir