Re: [Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions

[Fam Zheng <famz@redhat.com>]

On Tue, 05/26 19:54, Fam Zheng wrote:
> On Mon, 04/27 18:28, Paolo Bonzini wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Use atomic_or() for atomic bitmaps where several threads may set bits at
> > the same time.  This avoids the race condition between threads loading
> > an element, bitwise ORing, and then storing the element.
> > 
> > When setting all bits in a word we can avoid atomic ops and instead just
> > use an smp_mb() at the end.
> > 
> > Most bitmap users don't need atomicity so introduce new functions.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Message-Id: <1417519399-3166-2-git-send-email-stefanha@redhat.com>
> > [Avoid barrier in the single word case, use full barrier instead of write.
> >  - Paolo]
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  include/qemu/bitmap.h |  2 ++
> >  include/qemu/bitops.h | 14 ++++++++++++++
> >  util/bitmap.c         | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> > 
> > diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> > index f0273c9..3e0a4f3 100644
> > --- a/include/qemu/bitmap.h
> > +++ b/include/qemu/bitmap.h
> > @@ -39,6 +39,7 @@
> >   * bitmap_empty(src, nbits)			Are all bits zero in *src?
> >   * bitmap_full(src, nbits)			Are all bits set in *src?
> >   * bitmap_set(dst, pos, nbits)			Set specified bit area
> > + * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic ops
> >   * bitmap_clear(dst, pos, nbits)		Clear specified bit area
> >   * bitmap_find_next_zero_area(buf, len, pos, n, mask)	Find bit free area
> >   */
> > @@ -226,6 +227,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
> >  }
> >  
> >  void bitmap_set(unsigned long *map, long i, long len);
> > +void bitmap_set_atomic(unsigned long *map, long i, long len);
> >  void bitmap_clear(unsigned long *map, long start, long nr);
> >  unsigned long bitmap_find_next_zero_area(unsigned long *map,
> >                                           unsigned long size,
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index 90ca8df..95f98fb 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> > @@ -16,6 +16,7 @@
> >  #include <assert.h>
> >  
> >  #include "host-utils.h"
> > +#include "atomic.h"
> >  
> >  #define BITS_PER_BYTE           CHAR_BIT
> >  #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
> > @@ -39,6 +40,19 @@ static inline void set_bit(long nr, unsigned long *addr)
> >  }
> >  
> >  /**
> > + * set_bit_atomic - Set a bit in memory atomically
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + */
> > +static inline void set_bit_atomic(long nr, unsigned long *addr)
> > +{
> > +    unsigned long mask = BIT_MASK(nr);
> > +    unsigned long *p = addr + BIT_WORD(nr);
> > +
> > +    atomic_or(p, mask);
> > +}
> > +
> > +/**
> >   * clear_bit - Clears a bit in memory
> >   * @nr: Bit to clear
> >   * @addr: Address to start counting from
> > diff --git a/util/bitmap.c b/util/bitmap.c
> > index 9c6bb52..6838d49 100644
> > --- a/util/bitmap.c
> > +++ b/util/bitmap.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include "qemu/bitops.h"
> >  #include "qemu/bitmap.h"
> > +#include "qemu/atomic.h"
> >  
> >  /*
> >   * bitmaps provide an array of bits, implemented using an an
> > @@ -177,6 +178,42 @@ void bitmap_set(unsigned long *map, long start, long nr)
> >      }
> >  }
> >  
> > +void bitmap_set_atomic(unsigned long *map, long start, long nr)
> > +{
> > +    unsigned long *p = map + BIT_WORD(start);
> > +    const long size = start + nr;
> 
> s/size/end/ ?
> 
> Otherwise,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

NACK

It's broken the same way as bitmap_test_and_clear_atomic. See my reply to patch
24.

Fam

> 
> > +    int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +    unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +    /* First word */
> > +    if (nr - bits_to_set > 0) {
> > +        atomic_or(p, mask_to_set);
> > +        nr -= bits_to_set;
> > +        bits_to_set = BITS_PER_LONG;
> > +        mask_to_set = ~0UL;
> > +        p++;
> > +    }
> > +
> > +    /* Full words */
> > +    while (nr - bits_to_set >= 0) {
> > +        *p = ~0UL;
> > +        nr -= bits_to_set;
> > +        mask_to_set = ~0UL;
> > +        p++;
> > +    }
> 
> For full words, will memset be faster by any chance?
> 
> > +
> > +    /* Last word */
> > +    if (nr) {
> > +        mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> > +        atomic_or(p, mask_to_set);
> > +    } else {
> > +        /* If we avoided the full barrier in atomic_or(), issue a
> > +         * barrier to account for the assignments in the while loop.
> > +         */
> > +        smp_mb();
> > +    }
> > +}
> > +
> >  void bitmap_clear(unsigned long *map, long start, long nr)
> >  {
> >      unsigned long *p = map + BIT_WORD(start);
> > -- 
> > 1.8.3.1
> > 
> > 
>