Re: [PATCH v3 04/10] qemu-thread: Avoid futex abstraction for non-Linux

[Akihiko Odaki]

On 2025/05/16 20:09, 'Paolo Bonzini' via devel wrote:
> Il mer 14 mag 2025, 08:57 Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@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