[PATCH v4 00/11] Improve futex usage

Akihiko Odaki posted 11 patches 5 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250526-event-v4-0-5b784cc8e1de@daynix.com
Maintainers: Phil Dennis-Jordan <phil@philjordan.eu>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Weil <sw@weilnetz.de>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Hailiang Zhang <zhanghailiang@xfusion.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
There is a newer version of this series
meson.build                       |   2 +
include/qemu/futex.h              |  44 +++++++++++-
include/qemu/lockcnt.h            |   2 +-
include/qemu/thread-posix.h       |   9 ---
include/qemu/thread-win32.h       |   6 --
include/qemu/thread.h             |  11 ++-
migration/migration.h             |  12 ++--
migration/colo.c                  |  20 +++---
migration/migration.c             |  21 +++---
migration/postcopy-ram.c          |  10 +--
migration/savevm.c                |   2 +-
tests/unit/test-aio-multithread.c |   6 +-
util/event.c                      | 122 +++++++++++++++++++++++++++++++
util/lockcnt.c                    |   9 +--
util/qemu-thread-posix.c          | 148 --------------------------------------
util/qemu-thread-win32.c          | 129 ---------------------------------
hw/display/apple-gfx.m            |  10 +--
util/meson.build                  |   3 +-
18 files changed, 223 insertions(+), 343 deletions(-)
[PATCH v4 00/11] Improve futex usage
Posted by Akihiko Odaki 5 months, 3 weeks ago
In a recent discussion, Phil Dennis-Jordan pointed out a quirk in
QemuEvent destruction due to futex-like abstraction, which prevented
the usage of QemuEvent in new and existing code[1]. With some more
thoughts after this discussion, I also found other problem and room
of improvement in futex usage. Here is a stack of patches to resolve
them.

Patch "futex: Check value after qemu_futex_wait()" ensures
qemu_futex_wait() is used in loops as suggested in the man page.

Patch "futex: Support Windows" implements futex functions for Windows.

Patch "qemu-thread: Avoid futex abstraction for non-Linux" and
"qemu-thread: Use futex for QemuEvent on Windows" enable destroying
QemuEvent immediately after qemu_event_wait().

Patch "qemu-thread: Use futex for QemuEvent on Windows" and
"qemu-thread: Use futex if available for QemuLockCnt" make the use of
futex functions added for Windows.

Patches "migration: Replace QemuSemaphore with QemuEvent",
"migration/colo: Replace QemuSemaphore with QemuEvent",
"migration/postcopy: Replace QemuSemaphore with QemuEvent", and
"hw/display/apple-gfx: Replace QemuSemaphore with QemuEvent" replace
some QemuSemaphores with QemuEvents, which can utilize futex. Some of
them rely on that QemuEvent can be destroyed immediately after
qemu_event_wait().

[1]: https://lore.kernel.org/r/CAAibmn3HZeDeK8FrYhHa1GGwc+N8rBuB2VvMRm7LCt0mUGmsYQ@mail.gmail.com

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v4:
- Added patch "qemu-thread: Remove qatomic_read() in qemu_event_set()".
- Renamed patch "futex: Replace __linux__ with CONFIG_LINUX" to
  "qemu-thread: Replace __linux__ with CONFIG_LINUX".
- Reverted changes to convert rp_pong_acks to QemuEvent.
- Link to v3: https://lore.kernel.org/qemu-devel/20250511-event-v3-0-f7f69247d303@daynix.com

Changes in v3:
- Fixed race between qemu_event_reset() and qemu_event_set().
- Prepared for spurious pthread_cond_wait() wakeups.
- Added patch "futex: Replace __linux__ with CONFIG_LINUX".
- Link to v2: https://lore.kernel.org/qemu-devel/20250510-event-v2-0-7953177ce1b8@daynix.com

Changes in v2:
- Rebased.
- Added patch
  "hw/display/apple-gfx: Replace QemuSemaphore with QemuEvent".
- Link to v1: https://lore.kernel.org/r/20241225-event-v1-0-a58c8d63eb70@daynix.com

---
Akihiko Odaki (11):
      futex: Check value after qemu_futex_wait()
      futex: Support Windows
      qemu-thread: Remove qatomic_read() in qemu_event_set()
      qemu-thread: Replace __linux__ with CONFIG_LINUX
      qemu-thread: Avoid futex abstraction for non-Linux
      qemu-thread: Use futex for QemuEvent on Windows
      qemu-thread: Use futex if available for QemuLockCnt
      migration: Replace QemuSemaphore with QemuEvent
      migration/colo: Replace QemuSemaphore with QemuEvent
      migration/postcopy: Replace QemuSemaphore with QemuEvent
      hw/display/apple-gfx: Replace QemuSemaphore with QemuEvent

 meson.build                       |   2 +
 include/qemu/futex.h              |  44 +++++++++++-
 include/qemu/lockcnt.h            |   2 +-
 include/qemu/thread-posix.h       |   9 ---
 include/qemu/thread-win32.h       |   6 --
 include/qemu/thread.h             |  11 ++-
 migration/migration.h             |  12 ++--
 migration/colo.c                  |  20 +++---
 migration/migration.c             |  21 +++---
 migration/postcopy-ram.c          |  10 +--
 migration/savevm.c                |   2 +-
 tests/unit/test-aio-multithread.c |   6 +-
 util/event.c                      | 122 +++++++++++++++++++++++++++++++
 util/lockcnt.c                    |   9 +--
 util/qemu-thread-posix.c          | 148 --------------------------------------
 util/qemu-thread-win32.c          | 129 ---------------------------------
 hw/display/apple-gfx.m            |  10 +--
 util/meson.build                  |   3 +-
 18 files changed, 223 insertions(+), 343 deletions(-)
---
base-commit: f0737158b483e7ec2b2512145aeab888b85cc1f7
change-id: 20241031-event-785a2f0dda4a

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>
Re: [PATCH v4 00/11] Improve futex usage
Posted by Peter Xu 5 months, 3 weeks ago
On Mon, May 26, 2025 at 02:29:10PM +0900, Akihiko Odaki wrote:
> Akihiko Odaki (11):
>       futex: Check value after qemu_futex_wait()
>       futex: Support Windows
>       qemu-thread: Remove qatomic_read() in qemu_event_set()
>       qemu-thread: Replace __linux__ with CONFIG_LINUX
>       qemu-thread: Avoid futex abstraction for non-Linux
>       qemu-thread: Use futex for QemuEvent on Windows
>       qemu-thread: Use futex if available for QemuLockCnt
>       migration: Replace QemuSemaphore with QemuEvent
>       migration/colo: Replace QemuSemaphore with QemuEvent
>       migration/postcopy: Replace QemuSemaphore with QemuEvent

In case it makes things easier.. I queued the three migration patches;
AFAIU they look like standalone to go even without prior patches, meanwhile
it shouldn't be an issue if they're queued in two pulls.

I am still not sure whether patch 1 is needed at all, but I'll leave that
to others to decide.

Thanks!

>       hw/display/apple-gfx: Replace QemuSemaphore with QemuEvent

-- 
Peter Xu
Re: [PATCH v4 00/11] Improve futex usage
Posted by Akihiko Odaki 5 months, 3 weeks ago
On 2025/05/26 23:48, Peter Xu wrote:
> On Mon, May 26, 2025 at 02:29:10PM +0900, Akihiko Odaki wrote:
>> Akihiko Odaki (11):
>>        futex: Check value after qemu_futex_wait()
>>        futex: Support Windows
>>        qemu-thread: Remove qatomic_read() in qemu_event_set()
>>        qemu-thread: Replace __linux__ with CONFIG_LINUX
>>        qemu-thread: Avoid futex abstraction for non-Linux
>>        qemu-thread: Use futex for QemuEvent on Windows
>>        qemu-thread: Use futex if available for QemuLockCnt
>>        migration: Replace QemuSemaphore with QemuEvent
>>        migration/colo: Replace QemuSemaphore with QemuEvent
>>        migration/postcopy: Replace QemuSemaphore with QemuEvent
> 
> In case it makes things easier.. I queued the three migration patches;
> AFAIU they look like standalone to go even without prior patches, meanwhile
> it shouldn't be an issue if they're queued in two pulls.
> 
> I am still not sure whether patch 1 is needed at all, but I'll leave that
> to others to decide.

The migration patches shouldn't be applied before patches "futex: Check 
value after qemu_futex_wait()" and "qemu-thread: Avoid futex abstraction 
for non-Linux" as they fix QemuEvent. Merging migration patches earlier 
can trigger bugs just like one we faced with hw/display/apple-gfx*

Regarding patch 1 ("futex: Check value after qemu_futex_wait()"), I can 
argue that we should have it to comply the generic "upstream-first" 
principle; the upstream (man page) says to make a loop, so the 
downstream (QEMU) should follow that until the upstream says otherwise. 
But I think it's a good idea to spend efforts to understand the 
reasoning behind the man page since it's so important that it affects 
not only QEMU but also any userspace program that uses libpthread (i.e., 
practically everything).

Regards,
Akihiko Odaki

* 
https://lore.kernel.org/qemu-devel/a8b4472c-8741-4f80-87e5-b406c56d1acd@daynix.com/
Re: [PATCH v4 00/11] Improve futex usage
Posted by Peter Xu 5 months, 3 weeks ago
On Tue, May 27, 2025 at 11:09:08AM +0900, Akihiko Odaki wrote:
> On 2025/05/26 23:48, Peter Xu wrote:
> > On Mon, May 26, 2025 at 02:29:10PM +0900, Akihiko Odaki wrote:
> > > Akihiko Odaki (11):
> > >        futex: Check value after qemu_futex_wait()
> > >        futex: Support Windows
> > >        qemu-thread: Remove qatomic_read() in qemu_event_set()
> > >        qemu-thread: Replace __linux__ with CONFIG_LINUX
> > >        qemu-thread: Avoid futex abstraction for non-Linux
> > >        qemu-thread: Use futex for QemuEvent on Windows
> > >        qemu-thread: Use futex if available for QemuLockCnt
> > >        migration: Replace QemuSemaphore with QemuEvent
> > >        migration/colo: Replace QemuSemaphore with QemuEvent
> > >        migration/postcopy: Replace QemuSemaphore with QemuEvent
> > 
> > In case it makes things easier.. I queued the three migration patches;
> > AFAIU they look like standalone to go even without prior patches, meanwhile
> > it shouldn't be an issue if they're queued in two pulls.
> > 
> > I am still not sure whether patch 1 is needed at all, but I'll leave that
> > to others to decide.
> 
> The migration patches shouldn't be applied before patches "futex: Check
> value after qemu_futex_wait()" and "qemu-thread: Avoid futex abstraction for
> non-Linux" as they fix QemuEvent. Merging migration patches earlier can
> trigger bugs just like one we faced with hw/display/apple-gfx*

I didn't see anything like it mentioned in either cover letter or the
apple-gfx patch.  Could you provide a pointer?

> 
> Regarding patch 1 ("futex: Check value after qemu_futex_wait()"), I can
> argue that we should have it to comply the generic "upstream-first"
> principle; the upstream (man page) says to make a loop, so the downstream
> (QEMU) should follow that until the upstream says otherwise. But I think
> it's a good idea to spend efforts to understand the reasoning behind the man
> page since it's so important that it affects not only QEMU but also any
> userspace program that uses libpthread (i.e., practically everything).

IMHO it's not how we define upstream/downstream, but maybe it's me who
missed something.

It's fine - as long as someone agrees with you (Paolo? :), it could likely
be that I was wrong and I just didn't realize that.

Thanks for the update anyway, I will drop the migration patches regardless.

-- 
Peter Xu
Re: [PATCH v4 00/11] Improve futex usage
Posted by Akihiko Odaki 5 months, 3 weeks ago
On 2025/05/27 22:46, Peter Xu wrote:
> On Tue, May 27, 2025 at 11:09:08AM +0900, Akihiko Odaki wrote:
>> On 2025/05/26 23:48, Peter Xu wrote:
>>> On Mon, May 26, 2025 at 02:29:10PM +0900, Akihiko Odaki wrote:
>>>> Akihiko Odaki (11):
>>>>         futex: Check value after qemu_futex_wait()
>>>>         futex: Support Windows
>>>>         qemu-thread: Remove qatomic_read() in qemu_event_set()
>>>>         qemu-thread: Replace __linux__ with CONFIG_LINUX
>>>>         qemu-thread: Avoid futex abstraction for non-Linux
>>>>         qemu-thread: Use futex for QemuEvent on Windows
>>>>         qemu-thread: Use futex if available for QemuLockCnt
>>>>         migration: Replace QemuSemaphore with QemuEvent
>>>>         migration/colo: Replace QemuSemaphore with QemuEvent
>>>>         migration/postcopy: Replace QemuSemaphore with QemuEvent
>>>
>>> In case it makes things easier.. I queued the three migration patches;
>>> AFAIU they look like standalone to go even without prior patches, meanwhile
>>> it shouldn't be an issue if they're queued in two pulls.
>>>
>>> I am still not sure whether patch 1 is needed at all, but I'll leave that
>>> to others to decide.
>>
>> The migration patches shouldn't be applied before patches "futex: Check
>> value after qemu_futex_wait()" and "qemu-thread: Avoid futex abstraction for
>> non-Linux" as they fix QemuEvent. Merging migration patches earlier can
>> trigger bugs just like one we faced with hw/display/apple-gfx*
> 
> I didn't see anything like it mentioned in either cover letter or the
> apple-gfx patch.  Could you provide a pointer?
The previous email had a URL at the bottom:
https://lore.kernel.org/qemu-devel/a8b4472c-8741-4f80-87e5-b406c56d1acd@daynix.com/

This thread discussed a bug of QemuEvent and reached to a conclusion to 
use QemuSemaphore instead to avoid it. This patch series intends to fix it.

Regards,
Akihiko Odaki
Re: [PATCH v4 00/11] Improve futex usage
Posted by Paolo Bonzini 5 months, 3 weeks ago
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().


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.

Paolo
Re: [PATCH v4 00/11] Improve futex usage
Posted by Akihiko Odaki 5 months, 3 weeks ago
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

Re: [PATCH v4 00/11] Improve futex usage
Posted by Paolo Bonzini 5 months, 3 weeks ago
On Tue, May 27, 2025 at 5:01 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 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().

Ok, I see what you mean - you would like the xchg to be an
xchg_release essentially.

There is actually one case in which skipping the xchg has an effect.
If you have the following:

- one side does

  s.foo = 1;
  qemu_event_set(&s.ev);

- the other side never reaches the qemu_event_reset(&s.ev)

then skipping the xchg might allow the cacheline for ev to remain
shared. This is unlikely to *make* a difference, though it does
*exist* as a difference, so I will review the patch, but I really
prefer to place it last.  It's safer to take a known-working
algorithm, apply it to all OSes (or at least Linux and Windows), and
only then you refine it. It also makes my queue shorter.

> > 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:
>
> There are too few relevant platforms to justify the effort potentially
> needed for quality assurance.

Ok, nice.  So it's really just NetBSD in the end.

> 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.

Understood.  For me it wasn't really about performance, but more about
understanding exactly which reorderings can happen and what
synchronizes with what. Load-acquire/store-release are simpler to
understand in that respect, especially since this use of condvar,
without the mutex in reset, is different from everything else that
I've ever seen.

Paolo
Re: [PATCH v4 00/11] Improve futex usage
Posted by Akihiko Odaki 5 months, 3 weeks ago
On 2025/05/28 0:01, Paolo Bonzini wrote:
> On Tue, May 27, 2025 at 5:01 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>> 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().
> 
> Ok, I see what you mean - you would like the xchg to be an
> xchg_release essentially.
> 
> There is actually one case in which skipping the xchg has an effect.
> If you have the following:
> 
> - one side does
> 
>    s.foo = 1;
>    qemu_event_set(&s.ev);
> 
> - the other side never reaches the qemu_event_reset(&s.ev)
> 
> then skipping the xchg might allow the cacheline for ev to remain
> shared. This is unlikely to *make* a difference, though it does
> *exist* as a difference, so I will review the patch, but I really
> prefer to place it last.  It's safer to take a known-working
> algorithm, apply it to all OSes (or at least Linux and Windows), and
> only then you refine it. It also makes my queue shorter.

Compilers and processors are free to make such an optimization so the 
contract between us and them does not change in that sense.

On the other hand, the removal of smb_mb() does change the contract; now 
compilers and processors are free to reorder loads and stores specified 
before qemu_event_set() after the load of ev->value, which was not 
possible in the past.

I'm confident that the change of the contract is safe, but it makes 
sense to pay extra caution.  I'll reorder the patches with the next version.

Regards,
Akihiko Odaki