From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56842) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNlVa-0000sk-Ge for qemu-devel@nongnu.org; Mon, 25 Jan 2016 13:04:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNlVV-0006Kz-KJ for qemu-devel@nongnu.org; Mon, 25 Jan 2016 13:04:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58097) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNlVV-0006Ku-Cm for qemu-devel@nongnu.org; Mon, 25 Jan 2016 13:04:53 -0500 References: <1453740558-16303-1-git-send-email-alex.bennee@linaro.org> <1453740558-16303-4-git-send-email-alex.bennee@linaro.org> From: Paolo Bonzini Message-ID: <56A663C0.80806@redhat.com> Date: Mon, 25 Jan 2016 19:04:48 +0100 MIME-Version: 1.0 In-Reply-To: <1453740558-16303-4-git-send-email-alex.bennee@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH 3/4] include/qemu/atomic.h: default to __atomic functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , qemu-devel@nongnu.org Cc: mttcg@greensocs.com, peter.maydell@linaro.org, mark.burton@greensocs.com, a.rigo@virtualopensystems.com, fred.konrad@greensocs.com On 25/01/2016 17:49, Alex Benn=C3=A9e wrote: > The __atomic primitives have been available since GCC 4.7 and provide > a richer interface for describing memory ordering requirements. As a > bonus by using the primitives instead of hand-rolled functions we can > use tools such as the AddressSanitizer which need the use of well > defined APIs for its analysis. >=20 > If we have __ATOMIC defines we exclusively use the __atomic primitives > for all our atomic access. Otherwise we fall back to the mixture of > __sync and hand-rolled barrier cases. >=20 > Signed-off-by: Alex Benn=C3=A9e > --- > include/qemu/atomic.h | 126 ++++++++++++++++++++++++++++++++----------= -------- > 1 file changed, 82 insertions(+), 44 deletions(-) >=20 > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index bd2c075..414c81a 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -15,12 +15,90 @@ > =20 > #include "qemu/compiler.h" > =20 > -/* For C11 atomic ops */ > =20 > /* Compiler barrier */ > #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) > =20 > -#ifndef __ATOMIC_RELAXED > +#ifdef __ATOMIC_RELAXED > +/* For C11 atomic ops */ > + > +/* Manual memory barriers > + * > + *__atomic_thread_fence does not include a compiler barrier; instead, > + * the barrier is part of __atomic_load/__atomic_store's "volatile-lik= e" > + * semantics. If smp_wmb() is a no-op, absence of the barrier means th= at > + * the compiler is free to reorder stores on each side of the barrier. > + * Add one here, and similarly in smp_rmb() and smp_read_barrier_depen= ds(). > + */ > + > +#define smp_mb() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSU= ME); barrier(); }) This should be seq_cst. > +#define smp_wmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEA= SE); barrier(); }) > +#define smp_rmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUI= RE); barrier(); }) > + > +#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence= (__ATOMIC_CONSUME); barrier(); }) > + > +/* Weak atomic operations prevent the compiler moving other > + * loads/stores past the atomic operation load/store. > + */ > +#define atomic_read(ptr) \ > + ({ \ > + typeof(*ptr) _val; \ > + __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \ This should be relaxed. > + _val; \ > + }) > + > +#define atomic_rcu_read(ptr) atomic_read(ptr) This should be consume. > + > +#define atomic_set(ptr, i) do { \ > + typeof(*ptr) _val =3D (i); \ > + __atomic_store(ptr, &_val, __ATOMIC_RELEASE); \ This should be relaxed. > +} while(0) > + > +#define atomic_rcu_set(ptr, i) atomic_set(ptr, i) This should be release. > +/* Sequentially consistent atomic access */ > + > +#define atomic_xchg(ptr, i) ({ \ > + typeof(*ptr) _new =3D (i), _old; \ > + __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \ > + _old; \ > +}) > + > +/* Returns the eventual value, failed or not */ > +#define atomic_cmpxchg(ptr, old, new) = \ > + ({ = \ > + typeof(*ptr) _old =3D (old), _new =3D (new); = \ > + __atomic_compare_exchange(ptr, &_old, &_new, false, = \ > + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); = \ > + *ptr; /* can this race if cmpxchg not used elsewhere? */ = \ If I read the manual correctly, you can return _old here (that's why it is a pointer). > + }) > + > +#define atomic_mb_set(ptr, i) ((void)atomic_xchg(ptr, i)) > +#define atomic_mb_read(ptr) \ > + ({ \ > + typeof(*ptr) _val; \ > + __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE); \ > + _val; \ > + }) Please leave these defined in terms of relaxed accesses and memory barrie= rs. Paolo