Implementation of opal atomic barriers in x86_64

[gkatev]

Hi,
I'm wondering about the implementation of memory barriers (opal_atomic_) on x86_64.
With the gcc builtins, I see that opal_atomic_rmb() is implemented as:

static inline void opal_atomic_rmb(void)
{
#if OPAL_ASSEMBLY_ARCH == OPAL_X86_64
    /* work around a bug in older gcc versions where ACQUIRE seems to get
     * treated as a no-op instead of being equivalent to
     * __asm__ __volatile__("": : :"memory") */
    __atomic_thread_fence (__ATOMIC_SEQ_CST);
#else
    __atomic_thread_fence (__ATOMIC_ACQUIRE);
#endif
}

Related: #6014

On a x86_64 VM with GCC 10.2.0, OpenMPI 4.1.0 (git, debug on) I can see that the generated assembly contains an mfence(aka full memory fence):

$ (gdb) x/10i opal_atomic_rmb
   0x4262 <opal_atomic_rmb>:	push   %rbp
   0x4263 <opal_atomic_rmb+1>:	mov    %rsp,%rbp
   0x4266 <opal_atomic_rmb+4>:	mfence 
   0x4269 <opal_atomic_rmb+7>:	nop
   0x426a <opal_atomic_rmb+8>:	pop    %rbp
   0x426b <opal_atomic_rmb+9>:	retq   

checking for __atomic builtin atomics... yes

(I also observed inline mfences on another system with an older gcc (4.8.5 I think) and 4.1.0 (non-debug) )

It is my understanding that by default, x86_64 does not reorder load-load and load-store(the two cases for which opal_atomic_rmb() is intended?), so is an mfence required or should just a compiler barrier be enough?
(I haven't checked for performance differences with/without mfence.)

Furthermore, in the non-gcc-builtin implementation of opal_atomic_mb() for x86_64, a compiler barrier is utilized:

#define MB() __asm__ __volatile__("": : :"memory")
static inline void opal_atomic_mb(void)
{
    MB();
}

Considering opal_atomic_mb() is intended as a full barrier(?), will this be enough? Or is an mfence required here, to protect again store-load reordering, which AFAIK x86 can/will do.

Some of my sources:
https://preshing.com/20120710/memory-barriers-are-like-source-control-operations/
https://preshing.com/20120515/memory-reordering-caught-in-the-act/

[hjelmn]

No, a full barrier was not intended. We were aiming for the compiler barrier as some versions of gcc incorrectly handle:

__atomic_thread_fence (__ATOMIC_ACQUIRE);

and end up re-ordering instructions across the barrier. Newer versions of gcc (6.0.0 and newer) do not have this bug.

The quickest fix may be to just go ahead and change it to __asm__ __volatile__("": : :"memory").

Note the gcc builtins have been essentially dropped in v5.0.0 in favor of C11 atomics so while this change can be applied to master/v5.0.x it is not as relevant there.

[gkatev]

Yes that sounds like a good and simple solution, and I see it was merged in #8623.

Regarding the second part of my question, is opal_atomic_mb() intended as a full barrier?
In that case, the asm implementation for x86 should include an mfence instruction (?). Currently, only a compiler fence is utilized for MB().

[awlauria]

It looks like the solution to fix this was done in:

master: #8621
v5.0.x: #8622
v4.1.x: #8623
v4.0.x: #8624

which means this is the same issue as #8603.

Closing - if I am wrong please re-open.

[gkatev]

It was actually a two-scale issue, it probably was not clear enough and kind of phrased more as a question.
The problem that is also the subject of #8603 is fixed.

The second problem I believe I have spotted is in the x86-asm implementation of the full memory barrier:

#define MB() __asm__ __volatile__("": : :"memory")
static inline void opal_atomic_mb(void)
{
    MB();
}

It is my undestanding that a full memory barrier (opal_atomic_mb = full memory barrier?), in x86, requires an mfence or similar instruction, and a compiler barrier (MB()) is not sufficient. Please correct me if I'm wrong.

Code location:

ompi/opal/include/opal/sys/x86_64/atomic.h

Line 53 in dff15b3

static inline void opal_atomic_mb(void)

Edit: I cannot seem to find the button to reopen this, if you could please do so for me. (considering you also think it is still outstanding..)

[awlauria]

Thanks @gkatev - reopened.

@hjelmn fyi

[bosilca]

Yes, opal_atomic_mb should be a fully fledged memory barrier (as implemented on all the other architectures).

[awlauria]

This just looks like a one liner change if I am reading this right, anyone have time to do this and test?