Re: [PATCH 1/8] qatomic: add smp_mb__before/after_rmw()

[David Hildenbrand <david@redhat.com>]

On 03.03.23 18:19, Paolo Bonzini wrote:
> On ARM, seqcst loads and stores (which QEMU does not use) are compiled
> respectively as LDAR and STLR instructions.  Even though STLR is also
> used for store-release operations, STLR followed by LDAR provides
> store-against-load ordering, which is stronger than a store-release.
> Compare this to ARMv7, where store-release is DMB+STR and store-seqcst
> is DMB+STR+DMB.
> 
> This means that on ARM a sequence of
> 
>    qatomic_store_release(&y, ...);         // STLR
>    a = qatomic_load_acquire(&x);           // LDAR
> 
> provides stronger ordering at the processor level than the two MOV
> instructions you'd get on x86.
> 
> Likewise, on ARM sequentially consistent read-modify-write operations only
> need to use LDAXR and STLXR respectively for the load and the store, which
> is weaker than the LOCK prefix used on x86.
> 
> In a strange twist of events, however, the _stronger_ semantics
> of the ARM instructions can end up causing bugs on ARM, not on x86.
> The problems occur when seqcst atomics are mixed with relaxed atomics.
> 
> QEMU's atomics try to bridge the Linux API (that most of the developers
> are familiar with) and the C11 API, and the two have a substantial
> difference:
> 
> - in Linux, strongly-ordered atomics such as atomic_add_return() affect
>    the global ordering of _all_ memory operations, including for example
>    READ_ONCE()/WRITE_ONCE()
> 
> - in C11, sequentially consistent atomics (except for seq-cst fences)
>    only affect the ordering of sequentially consistent operations.
>    In particular, since relaxed loads are done with LDR on ARM, they are
>    not ordered against seqcst stores (which are done with STLR).
> 
> QEMU implements high-level synchronization primitives with the idea that
> the primitives contain the necessary memory barriers, and the callers can
> use relaxed atomics (qatomic_read/qatomic_set) or even regular accesses.
> This is very much incompatible with the C11 view that seqcst accesses
> are only ordered against other seqcst accesses, and requires using seqcst
> fences as in the following example:
> 
>     qatomic_set(&y, 1);            qatomic_set(&x, 1);
>     smp_mb();                      smp_mb();
>     ... qatomic_read(&x) ...       ... qatomic_read(&y) ...
> 
> When a qatomic_*() read-modify write operation is used instead of one
> or both stores, developers that are more familiar with the Linux API may
> be tempted to omit the smp_mb(), which will work on x86 but not on ARM.
> 
> This nasty difference between Linux and C11 read-modify-write operations
> has already caused issues in util/async.c and more are being found.
> Provide something similar to Linux smp_mb__before/after_atomic(); this
> has the double function of documenting clearly why there is a memory
> barrier, and avoiding a double barrier on x86 and s390x systems.
> 

Right, just like smp_mb__before_atomic()/smp_mb__after_atomic().


> The new macro can already be put to use in qatomic_mb_set().
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   docs/devel/atomics.rst | 26 +++++++++++++++++++++-----
>   include/qemu/atomic.h  | 17 ++++++++++++++++-
>   2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/devel/atomics.rst b/docs/devel/atomics.rst
> index 7957310071d9..898f5393c07a 100644
> --- a/docs/devel/atomics.rst
> +++ b/docs/devel/atomics.rst
> @@ -27,7 +27,8 @@ provides macros that fall in three camps:
>   
>   - weak atomic access and manual memory barriers: ``qatomic_read()``,
>     ``qatomic_set()``, ``smp_rmb()``, ``smp_wmb()``, ``smp_mb()``,
> -  ``smp_mb_acquire()``, ``smp_mb_release()``, ``smp_read_barrier_depends()``;
> +  ``smp_mb_acquire()``, ``smp_mb_release()``, ``smp_read_barrier_depends()``,
> +  ``smp_mb__before_rmw()``, ``smp_mb__after_rmw()``;
>   
>   - sequentially consistent atomic access: everything else.
>   
> @@ -472,7 +473,7 @@ and memory barriers, and the equivalents in QEMU:
>     sequential consistency.
>   
>   - in QEMU, ``qatomic_read()`` and ``qatomic_set()`` do not participate in
> -  the total ordering enforced by sequentially-consistent operations.
> +  the ordering enforced by read-modify-write operations.
>     This is because QEMU uses the C11 memory model.  The following example
>     is correct in Linux but not in QEMU:
>   
> @@ -488,9 +489,24 @@ and memory barriers, and the equivalents in QEMU:
>     because the read of ``y`` can be moved (by either the processor or the
>     compiler) before the write of ``x``.
>   
> -  Fixing this requires an ``smp_mb()`` memory barrier between the write
> -  of ``x`` and the read of ``y``.  In the common case where only one thread
> -  writes ``x``, it is also possible to write it like this:
> +  Fixing this requires a full memory barrier between the write of ``x`` and
> +  the read of ``y``.  QEMU provides ``smp_mb__before_rmw()`` and
> +  ``smp_mb__after_rmw()``; they act both as an optimization,
> +  avoiding the memory barrier on processors where it is unnecessary,
> +  and as a clarification of this corner case of the C11 memory model:
> +
> +      +--------------------------------+
> +      | QEMU (incorrect)               |

Just double-checking: shouldn't this be "QEMU (correct)" ?

Or am I confused? :)

-- 
Thanks,

David / dhildenb