On 2025/05/16 20:09, 'Paolo Bonzini' via devel wrote:
Il mer 14 mag 2025, 08:57 Akihiko Odaki <akihiko.od...@daynix.com
<mailto:akihiko.od...@daynix.com>> ha scritto:
Honestly, I do not understand why smp_mb() is placed here even in the
futex case. The comment says:
> qemu_event_set has release semantics, but because it *loads*
> ev->value we need a full memory barrier here.
The barrier is indeed followed by a load: qatomic_read(&ev->value) !
= EV_SET
However, the caller of qemu_event_set() should not care whether the
event is already set or not
so I'm not sure if smp_mb() is needed in
the first place.
The barrier is needed to ensure correct ordering in all cases. You have
on one side
done=true
Set
Read ev->value
If not EV_SET, set the event+ wake up waiters
And on the other
Write EV_FREE
Write
If not done
Wait
If one that calls qemu_event_set and the other calls qemu_event_reset,
you need to avoid that set reads a previous EV_SET for the value *and*
the other side reads done equal to false. The only way to avoid this is
for both sides to use a memory barrier before the read.
qemu_event_set(): release *if the event is not already set*; otherwise
it has no effect on synchronization so we don't need a barrier either.
It needs to be release always. This ensures that, whenever the setter
declares the event to be set, the other side sees whatever the setter
did before the call.
That is what I misunderstood, and now I can also imagine why it should
always release. Thinking of the scenario with the "done" flag we have
discussed, if we have two setters, the resetter should acquire the state
from both of the two setters.
If the event is already set, we need to ensure all stores specified
before qatomic_read(&ev->value) != EV_SET should happen before
qatomic_read(&ev->value), which requires us to put a full barrier.
But I have a new question: do we really need "if
(qatomic_read(&ev->value) != EV_SET)"?
With git blame, I found it didn't have the full barrier until commit
374293ca6fb0 ("qemu-thread: use acquire/release to clarify semantics of
QemuEvent"), which added it.
atomic_mb_read() was used until the commit instead.
include/qemu/atomic.h at that time had the following comment:
> /* atomic_mb_read/set semantics map Java volatile variables. They are
> * less expensive on some platforms (notably POWER & ARMv7) than fully
> * sequentially consistent operations.
We place a full barrier anyway, so we no longer have a reason to have
qatomic_read() before performing qatomic_xchg().
I also found smp_mb__after_rmw() before qemu_futex_wake_all() is no
longer necessary. Summing up, I think the code should look like as follows:
#ifdef HAVE_FUTEX
if (qatomic_xchg(&ev->value, EV_SET) == EV_BUSY) {
/* There were waiters, wake them up. */
qemu_futex_wake_all(ev);
}
#else
pthread_mutex_lock(&ev->lock);
qatomic_store_release(&ev->value, EV_SET);
pthread_cond_broadcast(&ev->cond);
pthread_mutex_unlock(&ev->lock);
#endif
Regards,
Akihiko Odaki
