Re: [PATCH] Fix atomic_full_barrier on x86 and x86_64.

[Rich Felker <dalias at libc dot org>]

On Wed, Oct 29, 2014 at 07:18:56PM +0100, Torvald Riegel wrote:
> Currently, x86 and x86_64 do not define any of the barriers.  The
> generic default for the barriers is that full barriers are just compiler
> barriers and read and write barriers default to using the full barrier
> definition.  On x86 and x86_64, however, only read/write barriers (ie,
> acquire and release fences in how glibc uses them) are just compiler
> barriers -- a full barrier needs an mfence instruction.  This patch
> defines atomic_full_barrier accordingly without making read/write
> barriers stronger than compiler barriers.

Can you explain why you claim these are needed? The architecture
inherently has strong memory ordering that prevents all reorderings
except for moving loads before stores when the addresses differ. Are
you concerned about creating a barrier for the loosely-ordered "nt"
sse operations or something? Or something else?

> The only use of atomic_full_barrier on x86/x86_64 is in
> sysdeps/nptl/fork.c, but there, ISTM that this actually only needs an
> acquire barrier.  So, this patch doesn't fix existing bugs in *current*
> code, and only makes fork a tiny tiny bit slower.
> 
> Nonetheless, there are other uses of full barriers in generic code
> (e.g., semaphores).  Those don't apply for x86/x86_64 because we
> currently have arch-specific code for those, but would if we start using
> the generic code on x86 too (and we couldn't do until we fix the x86
> full barrier).

Shouldn't the atomics for these already provide sufficient barriers?

> commit be14e33b607609462130a0264c7a9460596da3f8
> Author: Torvald Riegel <triegel@redhat.com>
> Date:   Wed Oct 29 10:34:36 2014 +0100
> 
>     Fix atomic_full_barrier on x86 and x86_64.
>     
>     	* sysdeps/x86_64/bits/atomic.h: (atomic_full_barrier,
>     	atomic_read_barrier, atomic_write_barrier): Define.
>     	* sysdeps/i386/i486/bits/atomic.h (atomic_full_barrier,
>     	atomic_read_barrier, atomic_write_barrier): Define.
> 
> diff --git a/sysdeps/i386/i486/bits/atomic.h b/sysdeps/i386/i486/bits/atomic.h
> index 76e0e8e..7c432f9 100644
> --- a/sysdeps/i386/i486/bits/atomic.h
> +++ b/sysdeps/i386/i486/bits/atomic.h
> @@ -532,3 +532,7 @@ typedef uintmax_t uatomic_max_t;
>  #define atomic_or(mem, mask) __arch_or_body (LOCK_PREFIX, mem, mask)
>  
>  #define catomic_or(mem, mask) __arch_or_body (__arch_cprefix, mem, mask)
> +
> +#define atomic_full_barrier() __asm ("mfence" ::: "memory")
> +#define atomic_read_barrier() __asm ("" ::: "memory")
> +#define atomic_write_barrier() __asm ("" ::: "memory")

Does glibc now require i686+sse? AFAIK mfence is SIGILL on all
previous x86 generations. An alternate portable (to earlier chips) but
slower approach is a useless xchg. It also _might_ work to perform a
store/load pair on the same address (I'm not clear on whether this
method is valid).

Rich