Re: [RFC] Move to C11 Atomics

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2470 bytes --]

On 21/09/20 15:44, Stefan Hajnoczi wrote:
> On Mon, Sep 21, 2020 at 01:15:30PM +0200, Paolo Bonzini wrote:
>> On 21/09/20 12:41, Stefan Hajnoczi wrote:
>>> The upshot is that all atomic variables in QEMU need to use C11 Atomic
>>> atomic_* types. This is a big change!
>>
>> The main issue with this is that C11 atomic types do too much magic,
>> including defaulting to seq-cst operations for loads and stores. As
>> documented in docs/devel/atomics.rst, seq-cst loads and stores are
>> almost never what you want (they're only a little below volatile :)):
> 
> I can't find where atomics.rst says seq-cst operations are almost never
> what you want?

Note that I'm talking only about loads and stores.  They are so much
never what you want that we don't even provide a wrapper for them in
qemu/atomic.h.

  ``qemu/atomic.h`` also provides loads and stores that cannot be
  reordered with each other::

    typeof(*ptr) atomic_mb_read(ptr)
    void         atomic_mb_set(ptr, val)

  However these do not provide sequential consistency and, in
  particular, they do not participate in the total ordering enforced by
  sequentially-consistent operations.  For this reason they are
  deprecated.  They should instead be replaced with any of the following
  (ordered from easiest to hardest):

  - accesses inside a mutex or spinlock

  - lightweight synchronization primitives such as ``QemuEvent``

  - RCU operations (``atomic_rcu_read``, ``atomic_rcu_set``) when
  publishing or accessing a new version of a data structure

  - other atomic accesses: ``atomic_read`` and ``atomic_load_acquire``
  for loads, ``atomic_set`` and ``atomic_store_release`` for stores,
  ``smp_mb`` to forbid reordering subsequent loads before a store.

where seq-cst loads and stores are again completely missing.  smp_mb is
there to replace them, as we did in commit 5710a3e0 ("async: use
explicit memory barriers").

>> we can use store-release/load-acquire
> 
> They don't provide atomic arithmetic/logic operations. The only
> non-seq-cst ALU operation I see in atomic.h is
> atomic_fetch_inc_nonzero(), and it's a cmpxchg() loop (ugly compared to
> an atomic ALU instruction).

Seq-cst is fine for RMW operations (arithmetic/logic, xchg, cmpxchg),
also because they're usually less performance critical than loads and
stores.  It's only loads and stores that give a false sense of
correctness as in the above commit.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]