[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC] Move to C11 Atomics
From: |
Stefan Hajnoczi |
Subject: |
Re: [RFC] Move to C11 Atomics |
Date: |
Mon, 21 Sep 2020 14:44:23 +0100 |
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?
I'm surprised that this isn't documented more prominently. Seq-cst
operations are the first type of atomic operation documented. It would
help to move them to the end of the document if they shouldn't be used.
> 1) in almost all cases where we do message passing between threads, 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).
> 2) when we want a full memory barrier, it's usually better to write the
> whole construct by hand, to avoid issues like the ones we had with
> ->notify_me.
>
> In addition, atomics are complicated enough that you almost always want
> a sign that you are using them, even at the cost of heavier-looking
> code. For example "atomic_read" straight ahead tells you "this is
> complicated but somebody has thought about it", while a naked access to
> a global variable or a struct field tells you "check which mutex is held
> when this function is called". By allowing shortcuts for seq-cst loads
> and stores, C11 atomics fool you into thinking that seq-cst accesses are
> all you need, while in reality they create more problems than they solve. :(
Good point. But it's easy to error out on 'atomic_fetch_add()' and
insist on 'atomic_fetch_add_explicit()' (where the user specifies the
memory order) in checkpatch.pl.
atomic_load_explicit()/atomic_store_explicit() can be used instead of
bare variable accesses. Although here I don't know how to enforce that
except via typedef struct { atomic_int i; } AtomicInt.
Thanks for suggesting the namespace cleanup I'll give that a try.
> > 1. Reimplement everything in terms of atomic_fetch_*() and other C11
> > Atomic APIs. For example atomic_fetch_inc() becomes
> > atomic_fetch_add(ptr, 1).
> >
> > 2. atomic_*_fetch() is not available in C11 Atomics so emulate it by
> > performing the operation twice (once atomic, then again non-atomic
> > using the fetched old atomic value). Yuck!
>
> They're hidden in plain sight if you are okay with seq-cst operations
> (which we almost always are for RMW operations, unlike loads and
> stores): "x += 3" is an atomic_add_fetch(&x, 3). However, they share
> with loads and stores the problem of being too magic; if you see "x +=
> 3" you don't think of it as something that's thread safe.
Ah, I see!
Stefan
signature.asc
Description: PGP signature