qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] ff00be: qatomic: add smp_mb__before/after_rmw


From: Peter Maydell
Subject: [Qemu-commits] [qemu/qemu] ff00be: qatomic: add smp_mb__before/after_rmw()
Date: Thu, 09 Mar 2023 02:20:36 -0800

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: ff00bed1897c3d27adc5b0cec6f6eeb5a7d13176
      
https://github.com/qemu/qemu/commit/ff00bed1897c3d27adc5b0cec6f6eeb5a7d13176
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M docs/devel/atomics.rst
    M include/qemu/atomic.h

  Log Message:
  -----------
  qatomic: add smp_mb__before/after_rmw()

On ARM, seqcst loads and stores (which QEMU does not use) are compiled
respectively as LDAR and STLR instructions.  Even though LDAR is
also used for load-acquire operations, it also waits for all STLRs to
leave the store buffer.  Thus, LDAR and STLR alone are load-acquire
and store-release operations, but LDAR also provides store-against-load
ordering as long as the previous store is a STLR.

Compare this to ARMv7, where store-release is DMB+STR and load-acquire
is LDR+DMB, but an additional DMB is needed between store-seqcst and
load-seqcst (e.g. DMB+STR+DMB+LDR+DMB); or with x86, where MOV provides
load-acquire and store-release semantics and the two can be reordered.

Likewise, on ARM sequentially consistent read-modify-write operations only
need to use LDAXR and STLXR respectively for the load and the store, while
on x86 they need to use the stronger LOCK prefix.

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.

The new macro can already be put to use in qatomic_mb_set().

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 9586a1329f5dce6c1d7f4de53cf0536644d7e593
      
https://github.com/qemu/qemu/commit/9586a1329f5dce6c1d7f4de53cf0536644d7e593
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M util/qemu-thread-posix.c

  Log Message:
  -----------
  qemu-thread-posix: cleanup, fix, document QemuEvent

QemuEvent is currently broken on ARM due to missing memory barriers
after qatomic_*().  Apart from adding the memory barrier, a closer look
reveals some unpaired memory barriers too.  Document more clearly what
is going on.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 6c5df4b48f0c52a61342ecb307a43f4c2a3565c4
      
https://github.com/qemu/qemu/commit/6c5df4b48f0c52a61342ecb307a43f4c2a3565c4
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M util/qemu-thread-win32.c

  Log Message:
  -----------
  qemu-thread-win32: cleanup, fix, document QemuEvent

QemuEvent is currently broken on ARM due to missing memory barriers
after qatomic_*().  Apart from adding the memory barrier, a closer look
reveals some unpaired memory barriers that are not really needed and
complicated the functions unnecessarily.  Also, it is relying on
a memory barrier in ResetEvent(); the barrier _ought_ to be there
but there is really no documentation about it, so make it explicit.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 2482aeea4195ad84cf3d4e5b15b28ec5b420ed5a
      
https://github.com/qemu/qemu/commit/2482aeea4195ad84cf3d4e5b15b28ec5b420ed5a
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M hw/misc/edu.c

  Log Message:
  -----------
  edu: add smp_mb__after_rmw()

Ensure ordering between clearing the COMPUTING flag and checking
IRQFACT, and between setting the IRQFACT flag and checking
COMPUTING.  This ensures that no wakeups are lost.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: b532526a07ef3b903ead2e055fe6cc87b41057a3
      
https://github.com/qemu/qemu/commit/b532526a07ef3b903ead2e055fe6cc87b41057a3
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M include/block/aio-wait.h

  Log Message:
  -----------
  aio-wait: switch to smp_mb__after_rmw()

The barrier comes after an atomic increment, so it is enough to use
smp_mb__after_rmw(); this avoids a double barrier on x86 systems.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: e3a3b6ec8169eab2feb241b4982585001512cd55
      
https://github.com/qemu/qemu/commit/e3a3b6ec8169eab2feb241b4982585001512cd55
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M util/qemu-coroutine-lock.c

  Log Message:
  -----------
  qemu-coroutine-lock: add smp_mb__after_rmw()

mutex->from_push and mutex->handoff in qemu-coroutine-lock implement
the familiar pattern:

   write a                                  write b
   smp_mb()                                 smp_mb()
   read b                                   read a

The memory barrier is required by the C memory model even after a
SEQ_CST read-modify-write operation such as QSLIST_INSERT_HEAD_ATOMIC.
Add it and avoid the unclear qatomic_mb_read() operation.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 33828ca11da08436e1b32f3e79dabce3061a0427
      
https://github.com/qemu/qemu/commit/33828ca11da08436e1b32f3e79dabce3061a0427
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M softmmu/physmem.c

  Log Message:
  -----------
  physmem: add missing memory barrier

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 8dd48650b43dfde4ebea34191ac267e474bcc29e
      
https://github.com/qemu/qemu/commit/8dd48650b43dfde4ebea34191ac267e474bcc29e
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M util/async.c

  Log Message:
  -----------
  async: update documentation of the memory barriers

Ever since commit 8c6b0356b539 ("util/async: make bh_aio_poll() O(1)",
2020-02-22), synchronization between qemu_bh_schedule() and aio_bh_poll()
is happening when the bottom half is enqueued in the bh_list; not
when the flags are set.  Update the documentation to match.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 6229438cca037d42f44a96d38feb15cb102a444f
      
https://github.com/qemu/qemu/commit/6229438cca037d42f44a96d38feb15cb102a444f
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M util/async.c

  Log Message:
  -----------
  async: clarify usage of barriers in the polling case

Explain that aio_context_notifier_poll() relies on
aio_notify_accept() to catch all the memory writes that were
done before ctx->notified was set to true.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


  Commit: 7b0f0aa55fd292fa3489755a3a896e496c51ea86
      
https://github.com/qemu/qemu/commit/7b0f0aa55fd292fa3489755a3a896e496c51ea86
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2023-03-07 (Tue, 07 Mar 2023)

  Changed paths:
    M docs/devel/atomics.rst
    M hw/misc/edu.c
    M include/block/aio-wait.h
    M include/qemu/atomic.h
    M softmmu/physmem.c
    M util/async.c
    M util/qemu-coroutine-lock.c
    M util/qemu-thread-posix.c
    M util/qemu-thread-win32.c

  Log Message:
  -----------
  Merge tag 'for-upstream-mb' of https://gitlab.com/bonzini/qemu into staging

* Fix missing memory barriers
* Fix comments about memory ordering

# -----BEGIN PGP SIGNATURE-----
#
# iQFIBAABCAAyFiEE8TM4V0tmI4mGbHaCv/vSX3jHroMFAmQHIqoUHHBib256aW5p
# QHJlZGhhdC5jb20ACgkQv/vSX3jHroPYBwgArUaS0KGrBM1XmRUUpXnJokmA37n8
# ft477na+XW+p9VYi27B0R01P8j+AkCrAO0Ir1MLG7axjn5KiRMnbf2uBgqasEREv
# repJEXsqISoxA6vvAvnehKHAI9zu8b7frRc/30b6EOrrZpn0JKePSNRTyBu2seGO
# NFDXPVA2Wom+xXaNSEGt0dmoJ6AzEVIZKhUIwyvUWOC7MXuuIkRWn9/nySUdvEt0
# RIFPPk7JCjnEc32vb4Xnq/Ncsy20tMIM1hlDxMOVNq3brjeSCzS0PPPSjE/X5OtW
# Yn5YS0nCyD7wjP2dkXI4I1lUPxUUx6LvMz1aGbJCfyjSX41mNES/agoGgA==
# =KEUo
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue 07 Mar 2023 11:40:26 GMT
# gpg:                using RSA key F13338574B662389866C7682BFFBD25F78C7AE83
# gpg:                issuer "pbonzini@redhat.com"
# gpg: Good signature from "Paolo Bonzini <bonzini@gnu.org>" [full]
# gpg:                 aka "Paolo Bonzini <pbonzini@redhat.com>" [full]
# Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4  E2F7 7E15 100C CD36 69B1
#      Subkey fingerprint: F133 3857 4B66 2389 866C  7682 BFFB D25F 78C7 AE83

* tag 'for-upstream-mb' of https://gitlab.com/bonzini/qemu:
  async: clarify usage of barriers in the polling case
  async: update documentation of the memory barriers
  physmem: add missing memory barrier
  qemu-coroutine-lock: add smp_mb__after_rmw()
  aio-wait: switch to smp_mb__after_rmw()
  edu: add smp_mb__after_rmw()
  qemu-thread-win32: cleanup, fix, document QemuEvent
  qemu-thread-posix: cleanup, fix, document QemuEvent
  qatomic: add smp_mb__before/after_rmw()

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Compare: https://github.com/qemu/qemu/compare/9832009d9dd2...7b0f0aa55fd2



reply via email to

[Prev in Thread] Current Thread [Next in Thread]