| Message ID | 1432659305-54578-24-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 05/26 18:54, 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 | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 54 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 The tab/space mix is not consistent. Fixing can be on top. > * 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 8abdcf9..8164225 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); > +} Where is this function used? Fam
On 27/05/2015 09:58, Fam Zheng wrote: > On Tue, 05/26 18:54, 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 | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 54 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 > > The tab/space mix is not consistent. Fixing can be on top. > >> * 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 8abdcf9..8164225 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); >> +} > > Where is this function used? In cpu_physical_memory_set_dirty_flag (patch 25). Paolo
On Tue, 05/26 18:54, 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 | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 54 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 8abdcf9..8164225 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..39994af 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,43 @@ 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; > + 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 */ > + if (bits_to_set == BITS_PER_LONG) { > + while (nr >= BITS_PER_LONG) { > + *p = ~0UL; > + nr -= BITS_PER_LONG; > + p++; Out of curiosity: why not use a memset here? Reviewed-by: Fam Zheng <famz@redhat.com> > + } > + } > + > + /* 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 > > >
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 8abdcf9..8164225 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..39994af 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,43 @@ 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; + 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 */ + if (bits_to_set == BITS_PER_LONG) { + while (nr >= BITS_PER_LONG) { + *p = ~0UL; + nr -= BITS_PER_LONG; + p++; + } + } + + /* 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);