On 3/7/23 18:26, Richard Henderson wrote:
On 3/7/23 09:00, Paolo Bonzini wrote:
while QSLIST_REMOVE_HEAD in the dequeuing thread is not ordered at all:
y.store(0, mo_relaxed); // QSLIST_REMOVE_HEAD
x.store(0, mo_release); // fetch_and
As I read aio_bh_queue, this is exactly the situation you describe in
patch 1 justifying the introduction of the new barriers.
Only store-store reordering is required between QSLIST_REMOVE_HEAD and
atomic_fetch_and(); and that one *is* blocked by atomic_fetch_and(),
since mo_seq_cst is a superset of both mo_release. The new barriers
are needed for store-load reordering.
In patch 1 you say:
# in C11, sequentially consistent atomics (except for seq-cst fences)
# only affect the ordering of sequentially consistent operations.
and the store in QSLIST_REMOVE_HEAD is not a sequentially consistent
operation.
Therefore by your own logic we must have a separate barrier here.
You're right that the comment is contradictory.
It's the comment that is wrong. The right text should be
---
in C11, with the exception of seq-cst fences, the order established by
sequentially consistent atomics does not propagate to other memory
accesses on either side of the seq-cst atomic. As far as those are
concerned, loads performed by a seq-cst atomic are just acquire loads,
and stores are just release stores. Even though loads that occur after
a RMW operation cannot move above the load, they can still sneak above
the store!
---
The first sentence is a more accurate way to say the same thing. The
rest sentence says what happens (mo_seq_cst downgrades to mo_acq_rel as
far as those other accesses are concerned) and I missed the opportunity
to include it because it didn't matter for the cases where the series
added barriers---except now it matters for *not* adding barriers.
Now, I also said that I don't understand mo_acq_rel and that is indeed
true. Instead, in this case I'm basically treating mo_seq_cst as a
superset of both mo_release (on the dequeue side) or mo_acquire (on the
enqueue side), individually.
I wonder if your definition/description of smp_mb__before_rmw() isn't
actively misleading in this case.
- We could drop it entirely and be less confusing, by not having to
explain it.
- We could define it as signal_barrier() for all hosts, simply to fix the
compiler-theoretic reordering problem.
The case that I was imagining for smp_mb__before_rmw() is something like
this:
wake_me = true;
smp_mb__before_rmw();
if (qatomic_xchg(&can_sleep, true)) { ... }
where you really need a full barrier.
Paolo
