From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxE5f-00060Q-UR for qemu-devel@nongnu.org; Tue, 26 May 2015 08:36:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxE5a-0005tK-S5 for qemu-devel@nongnu.org; Tue, 26 May 2015 08:36:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41884) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxE5a-0005t5-Lt for qemu-devel@nongnu.org; Tue, 26 May 2015 08:36:10 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 5590C8E514 for ; Tue, 26 May 2015 12:36:10 +0000 (UTC) Date: Tue, 26 May 2015 20:36:07 +0800 From: Fam Zheng Message-ID: <20150526123607.GA21680@ad.nay.redhat.com> References: <1430152117-100558-1-git-send-email-pbonzini@redhat.com> <1430152117-100558-24-git-send-email-pbonzini@redhat.com> <20150526115416.GV13749@ad.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150526115416.GV13749@ad.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH 23/29] bitmap: add atomic set functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com On Tue, 05/26 19:54, Fam Zheng wrote: > On Mon, 04/27 18:28, Paolo Bonzini wrote: > > From: Stefan Hajnoczi > > > > 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 > > 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 > > --- > > 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 > > > > #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 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 > > > > >