Re: [Qemu-devel] [PATCH v2 2/3] atomics: emit an smp_read_barrier_depends() barrier only for Sparc and Thread Sanitizer

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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
>