| Message ID | 1470668720-211300-2-git-send-email-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <vsementsov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- > 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
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 <vsementsov@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- >> 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
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); /* Check for end of iteration. We always use fewer than BITS_PER_LONG