[03/21] hbitmap: improve dirty iter
diff mbox

Message ID 1478715476-132280-4-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Nov. 9, 2016, 6:17 p.m. UTC
Make dirty iter resistant to resetting bits in corresponding HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/hbitmap.h | 25 +++----------------------
 util/hbitmap.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 23 deletions(-)

Comments

John Snow Nov. 14, 2016, 11:47 p.m. UTC | #1
On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make dirty iter resistant to resetting bits in corresponding HBitmap.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qemu/hbitmap.h | 25 +++----------------------
>  util/hbitmap.c         | 23 ++++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index eb46475..594f6f8 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -243,10 +243,8 @@ void hbitmap_free(HBitmap *hb);
>   * the lowest-numbered bit that is set in @hb, starting at @first.
>   *
>   * 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.
> + * iteration to miss some of those bits. Concurrent resetting of bits is OK,
> + * too.
>   */

Minor bikeshed: I might remove "too" because the concurrent resetting of 
bits is perfectly ok in /contrast/ to the concurrent setting of bits, 
which has undesirable side-effects.

I may simply strongly word this as:

"The concurrent resetting of bits is OK."

It's not worth a rewrite on its own.

Reviewed-by: John Snow <jsnow@redhat.com>

>  void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
>
> @@ -285,24 +283,7 @@ void hbitmap_free_meta(HBitmap *hb);
>   * Return the next bit that is set in @hbi's associated HBitmap,
>   * or -1 if all remaining bits are zero.
>   */
> -static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
> -{
> -    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
> -    int64_t item;
> -
> -    if (cur == 0) {
> -        cur = hbitmap_iter_skip_words(hbi);
> -        if (cur == 0) {
> -            return -1;
> -        }
> -    }
> -
> -    /* The next call will resume work from the next bit.  */
> -    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
> -    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
> -
> -    return item << hbi->granularity;
> -}
> +int64_t hbitmap_iter_next(HBitmapIter *hbi);
>
>  /**
>   * hbitmap_iter_next_word:
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 5d1a21c..48d8b2d 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
> @@ -139,6 +140,26 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
>      return cur;
>  }
>
> +int64_t hbitmap_iter_next(HBitmapIter *hbi)
> +{
> +    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
> +            hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
> +    int64_t item;
> +
> +    if (cur == 0) {
> +        cur = hbitmap_iter_skip_words(hbi);
> +        if (cur == 0) {
> +            return -1;
> +        }
> +    }
> +
> +    /* The next call will resume work from the next bit.  */
> +    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
> +    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
> +
> +    return item << hbi->granularity;
> +}
> +
>  void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
>  {
>      unsigned i, bit;
>

Patch
diff mbox

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index eb46475..594f6f8 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -243,10 +243,8 @@  void hbitmap_free(HBitmap *hb);
  * the lowest-numbered bit that is set in @hb, starting at @first.
  *
  * 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.
+ * iteration to miss some of those bits. Concurrent resetting of bits is OK,
+ * too.
  */
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
 
@@ -285,24 +283,7 @@  void hbitmap_free_meta(HBitmap *hb);
  * Return the next bit that is set in @hbi's associated HBitmap,
  * or -1 if all remaining bits are zero.
  */
-static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
-{
-    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-    int64_t item;
-
-    if (cur == 0) {
-        cur = hbitmap_iter_skip_words(hbi);
-        if (cur == 0) {
-            return -1;
-        }
-    }
-
-    /* The next call will resume work from the next bit.  */
-    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
-    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
-
-    return item << hbi->granularity;
-}
+int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
 /**
  * hbitmap_iter_next_word:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 5d1a21c..48d8b2d 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
@@ -139,6 +140,26 @@  unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
     return cur;
 }
 
+int64_t hbitmap_iter_next(HBitmapIter *hbi)
+{
+    unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
+            hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
+    int64_t item;
+
+    if (cur == 0) {
+        cur = hbitmap_iter_skip_words(hbi);
+        if (cur == 0) {
+            return -1;
+        }
+    }
+
+    /* The next call will resume work from the next bit.  */
+    hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
+
+    return item << hbi->granularity;
+}
+
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
 {
     unsigned i, bit;