| Message ID | 1464120374-8950-3-git-send-email-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/05/16 23:06, Emilio G. Cota wrote: > For correctness, smp_read_barrier_depends() is only required to > emit a barrier on Sparc hosts. However, we are currently emitting > a consume fence unconditionally. > > Fix it by keeping the consume fence if we're compiling with Thread > Sanitizer, since this might help prevent false warnings. Otherwise, > only emit the barrier for Sparc hosts. Note that we still guarantee > that smp_read_barrier_depends() is a compiler barrier. s/Sparc/Alpha/? Regards, Sergey > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > include/qemu/atomic.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 5bc4d6c..4a4f2fb 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -36,7 +36,14 @@ > #define smp_wmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); }) > #define smp_rmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); }) > > +#if defined(__SANITIZE_THREAD__) > #define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); }) > +#elsif defined(__alpha__) > +#define smp_read_barrier_depends() asm volatile("mb":::"memory") > +#else > +#define smp_read_barrier_depends() barrier() > +#endif > + > > /* Weak atomic operations prevent the compiler moving other > * loads/stores past the atomic operation load/store. However there is
On Tue, May 24, 2016 at 23:09:47 +0300, Sergey Fedorov wrote: > On 24/05/16 23:06, Emilio G. Cota wrote: > > For correctness, smp_read_barrier_depends() is only required to > > emit a barrier on Sparc hosts. However, we are currently emitting > > a consume fence unconditionally. > > > > Fix it by keeping the consume fence if we're compiling with Thread > > Sanitizer, since this might help prevent false warnings. Otherwise, > > only emit the barrier for Sparc hosts. Note that we still guarantee > > that smp_read_barrier_depends() is a compiler barrier. > > s/Sparc/Alpha/? Obviously :-) Typo also in patch 3 -- apologies. E.
On 24/05/2016 22:06, Emilio G. Cota wrote: > For correctness, smp_read_barrier_depends() is only required to > emit a barrier on Sparc hosts. However, we are currently emitting > a consume fence unconditionally. Let's say why this is suboptimal: ... and most compilers currently treat consume and acquire fences as equivalent. Likewise, let's add a comment like this: +/* Most compilers currently treat consume and acquire the same, but really + * no processors except Alpha need a barrier here. Leave it in if + * using Thread Sanitizer to avoid warnings, otherwise optimize it away. + */ If okay I can do the change myself. Thanks, Paolo > > Fix it by keeping the consume fence if we're compiling with Thread > Sanitizer, since this might help prevent false warnings. Otherwise, > only emit the barrier for Sparc hosts. Note that we still guarantee > that smp_read_barrier_depends() is a compiler barrier. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > include/qemu/atomic.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 5bc4d6c..4a4f2fb 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -36,7 +36,14 @@ > #define smp_wmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); }) > #define smp_rmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); }) > > +#if defined(__SANITIZE_THREAD__) > #define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); }) > +#elsif defined(__alpha__) > +#define smp_read_barrier_depends() asm volatile("mb":::"memory") > +#else > +#define smp_read_barrier_depends() barrier() > +#endif > + > > /* Weak atomic operations prevent the compiler moving other > * loads/stores past the atomic operation load/store. However there is >
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 5bc4d6c..4a4f2fb 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -36,7 +36,14 @@ #define smp_wmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); }) #define smp_rmb() ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); }) +#if defined(__SANITIZE_THREAD__) #define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); }) +#elsif defined(__alpha__) +#define smp_read_barrier_depends() asm volatile("mb":::"memory") +#else +#define smp_read_barrier_depends() barrier() +#endif + /* Weak atomic operations prevent the compiler moving other * loads/stores past the atomic operation load/store. However there is
For correctness, smp_read_barrier_depends() is only required to emit a barrier on Sparc hosts. However, we are currently emitting a consume fence unconditionally. Fix it by keeping the consume fence if we're compiling with Thread Sanitizer, since this might help prevent false warnings. Otherwise, only emit the barrier for Sparc hosts. Note that we still guarantee that smp_read_barrier_depends() is a compiler barrier. Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qemu/atomic.h | 7 +++++++ 1 file changed, 7 insertions(+)