Re: [PATCH v4 00/11] Improve futex usage

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/05/27 1:51, Paolo Bonzini wrote:
> On 5/26/25 07:29, Akihiko Odaki wrote:
>> Changes in v4:
>> - Added patch "qemu-thread: Remove qatomic_read() in qemu_event_set()".
> 
> Hi Akihiko,
> 
> I'm not so confident about putting this patch before the other changes;
> I'm referring basically to this hunk:
> 
> diff --git a/util/event.c b/util/event.c
> index 366c77c90cf..663b7042b17 100644
> --- a/util/event.c
> +++ b/util/event.c
> @@ -48,22 +48,9 @@ void qemu_event_set(QemuEvent *ev)
>       assert(ev->initialized);
> 
>   #ifdef HAVE_FUTEX
> -    /*
> -     * Pairs with both qemu_event_reset() and qemu_event_wait().
> -     *
> -     * qemu_event_set has release semantics, but because it *loads*
> -     * ev->value we need a full memory barrier here.
> -     */
> -    smp_mb();
> -    if (qatomic_read(&ev->value) != EV_SET) {
> -        int old = qatomic_xchg(&ev->value, EV_SET);
> -
> -        /* Pairs with memory barrier in kernel futex_wait system call.  */
> -        smp_mb__after_rmw();
> -        if (old == EV_BUSY) {
> -            /* There were waiters, wake them up.  */
> -            qemu_futex_wake_all(ev);
> -        }
> +    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);
> 
> 
> ... feel free to resubmit that separately, also because it's missing
> a smp_mb__before_rmw().

I'd like to submit it with "[PATCH v4 05/11] qemu-thread: Avoid futex 
abstraction for non-Linux" because it aligns the implementations of 
Linux and non-Linux versions to rely on a store-release of EV_SET in 
qemu_event_set().

smp_mb__before_rmw() is removed with "[PATCH v4 01/11] futex: Check 
value after qemu_futex_wait()" because the fact mentioned with the 
comment "Pairs with memory barrier in kernel futex_wait system call" is 
no longer relevant; the patch makes QEMU always rely on 
qatomic_load_acquire() or qatomic_cmpxchg() to perform a load-acquire of 
EV_SET for ordering.

In either case, I can simply that say the ordering between 
qemu_event_set() and qemu_event_wait() are ensured by load-acquire and 
store-release operations of EV_SET, but to make the correctness 
absolutely sure, I recommend to look at my past explanation:
https://lore.kernel.org/qemu-devel/ab6b66d7-fa8c-4049-9a3b-975f7f9c06ab@daynix.com

The explanation is long but so comprehensive that it lists all memory 
accesses, ordering requirements, and features of atomic primitives and 
futex employed to satisfy the requirements.

> 
> 
> Also, I'm not sure what was your opinion of the more optimized version
> of qemu_event_reset:
> 
> diff --git a/util/event.c b/util/event.c
> index 366c77c90cf..663b7042b17 100644
> --- a/util/event.c
> +++ b/util/event.c
> @@ -78,6 +78,7 @@ void qemu_event_reset(QemuEvent *ev)
>   {
>       assert(ev->initialized);
> 
> +#ifdef HAVE_FUTEX
>       /*
>        * If there was a concurrent reset (or even reset+wait),
>        * do nothing.  Otherwise change EV_SET->EV_FREE.
> @@ -86,6 +87,28 @@ void qemu_event_reset(QemuEvent *ev)
>        */
>       smp_mb__after_rmw();
> +#else
> +    /*
> +     * If futexes are not available, there are no EV_FREE->EV_BUSY
> +     * transitions because wakeups are done entirely through the
> +     * condition variable.  Since qatomic_set() only writes EV_FREE,
> +     * the load seems useless but in reality, the acquire synchronizes
> +     * with qemu_event_set()'s store release: if qemu_event_reset()
> +     * sees EV_SET here, then the caller will certainly see a
> +     * successful condition and skip qemu_event_wait():
> +     *
> +     * done = 1;                 if (done == 0)
> +     * qemu_event_set() {          qemu_event_reset() {
> +     *   lock();
> +     *   ev->value = EV_SET ----->     load ev->value
> +     *                                 ev->value = old value | EV_FREE
> +     *   cond_broadcast()
> +     *   unlock();                 }
> +     * }                           if (done == 0)
> +     *                               // qemu_event_wait() not called
> +     */
> +    qatomic_set(&ev->value, qatomic_load_acquire(&ev->value) | EV_FREE);
> +#endif
>   }
> 
>   void qemu_event_wait(QemuEvent *ev)
> 
> 
> Do you think it's incorrect?  I'll wait for your answer before sending
> out the actual pull request.

It's correct, but I don't think it's worthwhile.

This code path is only used by platforms without a futex wrapper. 
Currently we only have one for Linux and this series adds one for 
Windows, but FreeBSD[1] and OpenBSD[2] have their own futex. macOS also 
gained one with version 14.4.[3] We can add wrappers for them too if 
their performance really matters.

So the only platforms listed in docs/about/build-platforms.rst that 
require the non-futex version are macOS older than 14.4 and NetBSD. 
macOS older than 14.4 will not be supported after June 5 since macOS 14 
was released June 5, 2023 and docs/about/build-platforms.rst says:

 > Support for the previous major version will be dropped 2 years after
 > the new major version is released or when the vendor itself drops
 > support, whichever comes first.

 > Within each major release, only the most recent minor release is
 > considered.

There are too few relevant platforms to justify the effort potentially 
needed for quality assurance.

Moreover, qemu_event_reset() is often followed by qemu_event_wait() or 
other barriers so probably relaxing ordering here does not affect the 
overall ordering constraint (and performance) much.

Regards,
Akihiko Odaki

[1] 
https://man.freebsd.org/cgi/man.cgi?query=_umtx_op&apropos=0&sektion=2&manpath=FreeBSD+14.2-RELEASE&arch=default&format=html
[2] https://man.openbsd.org/futex
[3] 
https://developer.apple.com/documentation/os/synchronization?language=objc#Futex-Conditional-Wait-Primitives