ompi icon indicating copy to clipboard operation
ompi copied to clipboard

C11 Atomics: add relaxed load and store to avoid superfluous atomic instructions

Open devreal opened this issue 2 years ago • 3 comments

The C11 _Atomic qualifier requires accesses to such variables to be sequentially consistent [1], leading to unnecessary atomic operations in code paths where there cannot be concurrent accesses by other threads (e.g., initialization of data structures such as opal_object_t and the non-atomic fallbacks in the opal_thread_* functions). In fact, the whole shabeng about avoiding atomics in opal_thread_* calls is moot with C11.

This PR introduces OPAL_ATOMIC_RELAXED_STORE and OPAL_ATOMIC_RELAXED_LOAD to avoid atomic operations in paths where atomicity and memory ordering is not required. On my machine, a sequence of irecv - send - wait takes 0.22us on current main and 0.16us with this patch. I'm sure there are plenty of other places where we should use OPAL_ATOMIC_RELAXED_STORE instead of simple assignment but finding them is tedious.

Alternatively, we may consider removing _Atomic from the opal_atomic_* types and use volatile instead (to make sure polling on variables is not optimized out). That way, we don't have to litter ugly macros like OPAL_ATOMIC_RELAXED_STORE throughout the code and keep the C11 atomic backend consistent with the other backends. Plus, we won't run the chance that there are stray atomic operations that were not caught. After all, all code in Open MPI relies on the opal_atomic* functions to perform atomic operations and direct accesses to atomic variable are not expected to be atomic or ordered. I wasn't around when the decision was made to use _Atomic so I don't know what the arguments were back then though.

For the record: a simple atomic load/store interface for opal atomic was requested in https://github.com/open-mpi/ompi/issues/9722, which is what this PR adds.

[1] https://en.cppreference.com/w/c/language/atomic

Signed-off-by: Joseph Schuchart [email protected]

devreal avatar Jun 19 '22 21:06 devreal

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/a8b3319abf9c97d5d0ac7d1f57c3eac9

ibm-ompi avatar Jun 19 '22 21:06 ibm-ompi

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/7d3cdb42f2e690a6ac5e84c2a30bcf77

ibm-ompi avatar Jun 19 '22 21:06 ibm-ompi

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/a9ec65e552ace7c62d57961517834ba9

ibm-ompi avatar Jun 19 '22 21:06 ibm-ompi

Closing in favor of https://github.com/open-mpi/ompi/pull/10613

devreal avatar Aug 16 '22 15:08 devreal