From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49433) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4OHI-0002LA-Hk for qemu-devel@nongnu.org; Sun, 22 May 2016 03:58:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b4OHF-0007R9-Bj for qemu-devel@nongnu.org; Sun, 22 May 2016 03:58:24 -0400 Received: from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]:34929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4OHE-0007Q8-QS for qemu-devel@nongnu.org; Sun, 22 May 2016 03:58:21 -0400 Received: by mail-wm0-x22c.google.com with SMTP id i142so35315261wmf.0 for ; Sun, 22 May 2016 00:58:19 -0700 (PDT) References: <1463863336-28760-1-git-send-email-cota@braap.org> <1463863336-28760-2-git-send-email-cota@braap.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1463863336-28760-2-git-send-email-cota@braap.org> Date: Sun, 22 May 2016 08:58:51 +0100 Message-ID: <8737pah6bo.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/2] atomics: do not use __atomic primitives for RCU atomics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Emilio G. Cota" Cc: QEMU Developers , MTTCG Devel , Paolo Bonzini , Richard Henderson , Sergey Fedorov Emilio G. Cota writes: > Commit a0aa44b4 ("include/qemu/atomic.h: default to __atomic functions") > set all atomics to default (on recent GCC versions) to __atomic primitives. > > In the process, the atomic_rcu_read/set were converted to implement > consume/release semantics, respectively. This is inefficient; for > correctness and maximum performance we only need an smp_barrier_depends > for reads, and an smp_wmb for writes. Fix it by using the original > definition of these two primitives for all compilers. > > Note that in case these semantics were necessary to avoid false > positives under Thread Sanitizer, we could have them defined as such > under #ifdef __SANITIZE_THREAD__. I tried running QEMU under tsan > to explore this, but unfortunately I couldn't get it to work (tsan dies > with an "unexpected memory mapping"). I suspect though that the > atomic_read/set embedded in atomic_rcu_read/set is enough for tsan, > though. For tsan runs you need to re-build with: ./configure --cc=gcc --extra-cflags="-pie -fPIE -fsanitize=thread" --with-coroutine=gthread Specifically the coroutine ucontext messing really confuses TSAN. > > Signed-off-by: Emilio G. Cota > --- > include/qemu/atomic.h | 99 ++++++++++++++++++++++----------------------------- > 1 file changed, 43 insertions(+), 56 deletions(-) > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 5bc4d6c..4d62425 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -56,22 +56,6 @@ > __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \ > } while(0) > > -/* Atomic RCU operations imply weak memory barriers */ > - > -#define atomic_rcu_read(ptr) \ > - ({ \ > - QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ > - typeof(*ptr) _val; \ > - __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \ > - _val; \ > - }) > - > -#define atomic_rcu_set(ptr, i) do { \ > - QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ > - typeof(*ptr) _val = (i); \ > - __atomic_store(ptr, &_val, __ATOMIC_RELEASE); \ > -} while(0) > - > /* atomic_mb_read/set semantics map Java volatile variables. They are > * less expensive on some platforms (notably POWER & ARMv7) than fully > * sequentially consistent operations. > @@ -242,46 +226,6 @@ > #define atomic_read(ptr) (*(__typeof__(*ptr) volatile*) (ptr)) > #define atomic_set(ptr, i) ((*(__typeof__(*ptr) volatile*) (ptr)) = (i)) > > -/** > - * atomic_rcu_read - reads a RCU-protected pointer to a local variable > - * into a RCU read-side critical section. The pointer can later be safely > - * dereferenced within the critical section. > - * > - * This ensures that the pointer copy is invariant thorough the whole critical > - * section. > - * > - * Inserts memory barriers on architectures that require them (currently only > - * Alpha) and documents which pointers are protected by RCU. > - * > - * atomic_rcu_read also includes a compiler barrier to ensure that > - * value-speculative optimizations (e.g. VSS: Value Speculation > - * Scheduling) does not perform the data read before the pointer read > - * by speculating the value of the pointer. > - * > - * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg(). > - */ > -#define atomic_rcu_read(ptr) ({ \ > - typeof(*ptr) _val = atomic_read(ptr); \ > - smp_read_barrier_depends(); \ > - _val; \ > -}) > - > -/** > - * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure > - * meant to be read by RCU read-side critical sections. > - * > - * Documents which pointers will be dereferenced by RCU read-side critical > - * sections and adds the required memory barriers on architectures requiring > - * them. It also makes sure the compiler does not reorder code initializing the > - * data structure before its publication. > - * > - * Should match atomic_rcu_read(). > - */ > -#define atomic_rcu_set(ptr, i) do { \ > - smp_wmb(); \ > - atomic_set(ptr, i); \ > -} while (0) > - > /* These have the same semantics as Java volatile variables. > * See http://gee.cs.oswego.edu/dl/jmm/cookbook.html: > * "1. Issue a StoreStore barrier (wmb) before each volatile store." > @@ -345,4 +289,47 @@ > #define atomic_or(ptr, n) ((void) __sync_fetch_and_or(ptr, n)) > > #endif /* __ATOMIC_RELAXED */ > + > +/** > + * atomic_rcu_read - reads a RCU-protected pointer to a local variable > + * into a RCU read-side critical section. The pointer can later be safely > + * dereferenced within the critical section. > + * > + * This ensures that the pointer copy is invariant thorough the whole critical > + * section. > + * > + * Inserts memory barriers on architectures that require them (currently only > + * Alpha) and documents which pointers are protected by RCU. > + * > + * atomic_rcu_read also includes a compiler barrier to ensure that > + * value-speculative optimizations (e.g. VSS: Value Speculation > + * Scheduling) does not perform the data read before the pointer read > + * by speculating the value of the pointer. > + * > + * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg(). > + */ > +#define atomic_rcu_read(ptr) ({ \ > + QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ > + typeof(*ptr) _val = atomic_read(ptr); \ > + smp_read_barrier_depends(); \ > + _val; \ > +}) > + > +/** > + * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure > + * meant to be read by RCU read-side critical sections. > + * > + * Documents which pointers will be dereferenced by RCU read-side critical > + * sections and adds the required memory barriers on architectures requiring > + * them. It also makes sure the compiler does not reorder code initializing the > + * data structure before its publication. > + * > + * Should match atomic_rcu_read(). > + */ > +#define atomic_rcu_set(ptr, i) do { \ > + QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ > + smp_wmb(); \ > + atomic_set(ptr, i); \ > +} while (0) > + > #endif /* __QEMU_ATOMIC_H */ -- Alex Bennée