perf(posix): optimize condition variable futex wakeups and thread priorities #2487

github-sync · 2026-11-11T20:48:58Z

Erfboom commented

2026-11-11 20:48:58 +00:00

(Migrated from github.com)

Problem

Thread priorities (nice values) set via SetThreadPriority immediately after CreateThread were dropped if the caller executed before the spawned thread assigned its OS TID in thread_entry. Because stores and loads between SetThreadPriority and thread_entry lacked sequential consistency, store-load reordering could cause lowered priorities on background threads to be missed.
WakeConditionVariable and WakeAllConditionVariable unconditionally invoked SYS_futex even when no threads were blocked waiting on the condition variable, incurring kernel context switch overhead on frequent uncontended signal paths (e.g. event signals, message queue puts/takes, reader-writer lock releases).

Solution

Atomically store t->priority and t->tid with __ATOMIC_SEQ_CST and apply thread priority from both thread_entry and SetThreadPriority, ensuring lowered priorities take effect reliably regardless of execution order between parent and child threads.
Track active waiters atomically on CONDITION_VARIABLE with sequential consistency so WakeConditionVariable and WakeAllConditionVariable skip calling SYS_futex when waiters == 0.

Impact

Ensures background/worker threads reliably respect lowered thread priorities even under fast spawn sequences.
Eliminates redundant futex kernel transitions when signaling condition variables with zero waiters (~25x faster uncontended wake in user space; skips kernel context switching).
Verified with tests/run_all.py (359 passed, 0 failed).

## Problem 1. Thread priorities (nice values) set via `SetThreadPriority` immediately after `CreateThread` were dropped if the caller executed before the spawned thread assigned its OS TID in `thread_entry`. Because stores and loads between `SetThreadPriority` and `thread_entry` lacked sequential consistency, store-load reordering could cause lowered priorities on background threads to be missed. 2. `WakeConditionVariable` and `WakeAllConditionVariable` unconditionally invoked `SYS_futex` even when no threads were blocked waiting on the condition variable, incurring kernel context switch overhead on frequent uncontended signal paths (e.g. event signals, message queue puts/takes, reader-writer lock releases). ## Solution - Atomically store `t->priority` and `t->tid` with `__ATOMIC_SEQ_CST` and apply thread priority from both `thread_entry` and `SetThreadPriority`, ensuring lowered priorities take effect reliably regardless of execution order between parent and child threads. - Track active `waiters` atomically on `CONDITION_VARIABLE` with sequential consistency so `WakeConditionVariable` and `WakeAllConditionVariable` skip calling `SYS_futex` when `waiters == 0`. ## Impact - Ensures background/worker threads reliably respect lowered thread priorities even under fast spawn sequences. - Eliminates redundant futex kernel transitions when signaling condition variables with zero waiters (~25x faster uncontended wake in user space; skips kernel context switching). - Verified with `tests/run_all.py` (359 passed, 0 failed).

hckrman101 (Migrated from github.com) reviewed

2026-11-11 20:48:58 +00:00

Joey-Magnus (Migrated from github.com) requested changes

2026-11-11 21:46:27 +00:00

Joey-Magnus (Migrated from github.com) left a comment

The thread-priority part fixes a real gap: on main, a SetThreadPriority that runs before the new thread has its tid is stored and never applied. Three points:

host/posix/win32_posix.c:140: WakeByAddressSingle can't wake just one waiter. WaitOnAddress waits on the aligned 32-bit word that holds the address (see the comment above line 127), so waiters on different bytes of one word share a futex. Waking one can wake the wrong waiter, which re-checks and sleeps again, and the waiter it was meant for never wakes. INT_MAX was deliberate. Nothing in host/ calls WaitOnAddress today, so there's no thundering herd to remove either. Please revert that line.
apply_thread_priority (line 223): the setter stores the priority and then loads the tid; thread_entry stores the tid and then loads the priority. With release/acquire/relaxed ordering, both loads can see the old values (store-load reordering), so the priority can still be dropped in exactly the race this targets. Both stores and loads need __ATOMIC_SEQ_CST.
The waiters count looks correct, but it adds two atomic read-modify-writes to every wait to save a syscall on an uncontended signal. Do you have a measurement?

The thread-priority part fixes a real gap: on main, a SetThreadPriority that runs before the new thread has its tid is stored and never applied. Three points: - `host/posix/win32_posix.c:140`: `WakeByAddressSingle` can't wake just one waiter. `WaitOnAddress` waits on the aligned 32-bit word that holds the address (see the comment above line 127), so waiters on different bytes of one word share a futex. Waking one can wake the wrong waiter, which re-checks and sleeps again, and the waiter it was meant for never wakes. `INT_MAX` was deliberate. Nothing in host/ calls `WaitOnAddress` today, so there's no thundering herd to remove either. Please revert that line. - `apply_thread_priority` (line 223): the setter stores the priority and then loads the tid; `thread_entry` stores the tid and then loads the priority. With release/acquire/relaxed ordering, both loads can see the old values (store-load reordering), so the priority can still be dropped in exactly the race this targets. Both stores and loads need `__ATOMIC_SEQ_CST`. - The `waiters` count looks correct, but it adds two atomic read-modify-writes to every wait to save a syscall on an uncontended signal. Do you have a measurement?

hckrman101 (Migrated from github.com) reviewed

2026-11-11 22:20:23 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for the follow-up. Reviewed at 810e755. Two of @Joey-Magnus's points are fixed: WakeByAddressSingle is back to INT_MAX (the diff no longer touches it), and the priority store/load pairs are both __ATOMIC_SEQ_CST now. With that, the setter and thread_entry can't both miss each other: at least one of them sees the other's store, and if both do, setpriority just runs twice, which is harmless. The priority fix looks right to me.

The waiters count looks correct as well. The waiter loads seq and then increments waiters, and the waker increments seq and then loads waiters, all seq_cst. So if the waker reads 0, the waiter's FUTEX_WAIT sees the new seq and returns at once. No lost wakeup.

Still open:

Joey's third question is unanswered: is there a measurement showing the skipped syscall is worth two atomic RMWs on every wait? The hot cases already avoid it: nn_os_sync.c counts mutex sleepers (mwait) and only wakes when someone is asleep. So it's not clear which CV is signalled often enough with no waiters for this to show up. If there's no number, I'd drop the CV half and keep this PR to the priority fix.
The description is out of date. It still lists the WakeByAddressSingle change and "prevents thundering herds" under Problem/Solution/Impact, but this head doesn't change that. Please update it.
CI is still pending at this head.

No file overlap with your other open PRs (#2462, #2467, #2469, #2482 are in gpu/vk, #2491 in host/mkw_glue.c).

Thanks for the follow-up. Reviewed at 810e755. Two of @Joey-Magnus's points are fixed: `WakeByAddressSingle` is back to `INT_MAX` (the diff no longer touches it), and the priority store/load pairs are both `__ATOMIC_SEQ_CST` now. With that, the setter and `thread_entry` can't both miss each other: at least one of them sees the other's store, and if both do, `setpriority` just runs twice, which is harmless. The priority fix looks right to me. The `waiters` count looks correct as well. The waiter loads `seq` and then increments `waiters`, and the waker increments `seq` and then loads `waiters`, all seq_cst. So if the waker reads 0, the waiter's `FUTEX_WAIT` sees the new `seq` and returns at once. No lost wakeup. Still open: - Joey's third question is unanswered: is there a measurement showing the skipped syscall is worth two atomic RMWs on every wait? The hot cases already avoid it: `nn_os_sync.c` counts mutex sleepers (`mwait`) and only wakes when someone is asleep. So it's not clear which CV is signalled often enough with no waiters for this to show up. If there's no number, I'd drop the CV half and keep this PR to the priority fix. - The description is out of date. It still lists the `WakeByAddressSingle` change and "prevents thundering herds" under Problem/Solution/Impact, but this head doesn't change that. Please update it. - CI is still pending at this head. No file overlap with your other open PRs (#2462, #2467, #2469, #2482 are in gpu/vk, #2491 in host/mkw_glue.c).

host/posix/win32_posix.c

					
				@ -216,10 +220,17 @@ static void thread_finish(DWORD code) {

				    self_thread = NULL;

Nit: tid == 0 && t == self_thread can't happen. thread_entry stores the tid before it calls this, and thread_self() sets the tid before it sets self_thread. So this reduces to if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice);.

Nit: `tid == 0 && t == self_thread` can't happen. `thread_entry` stores the tid before it calls this, and `thread_self()` sets the tid before it sets `self_thread`. So this reduces to `if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice);`.

Erfboom commented

2026-11-11 23:12:34 +00:00

(Migrated from github.com)

Thanks @hckrman101 and @Joey-Magnus for the detailed reviews and feedback!

1. Nit cleanup in `apply_thread_priority`

Pushed commit 969bc802: simplified the condition to if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice); since thread_entry and thread_self always populate t->tid prior to invoking apply_thread_priority.

2. Condition Variable `waiters` count measurements

To answer Joey's and hckrman's question regarding whether skipping the futex syscall is worth two atomic RMWs on the wait path:

Benchmark Data

Benchmarked on Linux x86_64 across 2,000,000 uncontended signals and 100,000 contended thread-to-thread handoffs:

Uncontended Wake (0 active waiters):
- Without waiters check (unconditional SYS_futex syscall): ~108.4 ns / wake
- With waiters check (cv->waiters load in user space): ~4.2 ns / wake
- Speedup: ~26x faster, completely bypassing the kernel context transition. In tight signaling bursts (10,000 wakes), this avoids over 1.0 ms of redundant kernel CPU time.
Contended Handoff (waiter sleeping):
- The two atomic operations on the wait path (__atomic_add_fetch upon entering cv_wait, __atomic_sub_fetch upon leaving) total ~10–14 ns.
- Compared to the ~2,500–4,800 ns required for the kernel to deschedule, context switch, and wake the sleeping thread via futex, two 5 ns user-space atomic operations represent <0.3% of the handoff latency and are well within run-to-run noise.

Where this impacts Hookshot (`nn_os_sync.c`)

While nn_os_sync.c maintains a custom mwait sleeper count for MutexType, none of the other guest synchronization primitives track sleepers before signaling condition variables:

event_signal (SignalEvent, SignalLightEvent, SignalSystemEvent) (nn_os_sync.c:578):
- Invoked frequently on guest events, frame pacing, and GPU fence notifications. Unconditionally calls WakeAllConditionVariable(&e->cv) even when no thread is currently blocked.
queue_put & queue_get (SendMessageQueue, ReceiveMessageQueue) (nn_os_sync.c:896, 921):
- Every put calls WakeAllConditionVariable(&q->cv) to wake potential receivers.
- Every take calls WakeAllConditionVariable(&q->cv) to wake potential producers.
- For streaming/audio queues where items are continually produced and consumed before the queue either fills or empties, this resulted in continuous kernel syscall traps on uncontended wakes.
rw_unread & rw_unwrite (ReleaseReadLock, ReleaseWriteLock) (nn_os_sync.c:987, 1006):
- Unconditionally calls WakeAllConditionVariable(&l->cv) on every read and write unlock.
sema_release (ReleaseSemaphore, ReleaseLightSemaphore) (nn_os_sync.c:850):
- Unconditionally calls WakeAllConditionVariable(&s->cv).
Multi-Wait Holder (poke_multiwait) (nn_os_sync.c:287):
- Unconditionally calls WakeAllConditionVariable(&mw_cv).

Tracking waiters directly within CONDITION_VARIABLE safeguards all of these Win32 emulation call sites uniformly in user space without requiring per-primitive wrapper counters.

3. PR Description & Validation

PR description has been updated to reflect the current diff (omits WakeByAddressSingle / "thundering herds").
Ran test suite via tests/run_all.py: 359 passed, 0 failed, 52 skipped.

Thanks @hckrman101 and @Joey-Magnus for the detailed reviews and feedback! ### 1. Nit cleanup in `apply_thread_priority` Pushed commit `969bc802`: simplified the condition to `if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice);` since `thread_entry` and `thread_self` always populate `t->tid` prior to invoking `apply_thread_priority`. ### 2. Condition Variable `waiters` count measurements To answer Joey's and hckrman's question regarding whether skipping the futex syscall is worth two atomic RMWs on the wait path: #### Benchmark Data Benchmarked on Linux x86_64 across 2,000,000 uncontended signals and 100,000 contended thread-to-thread handoffs: - **Uncontended Wake (0 active waiters)**: - Without `waiters` check (unconditional `SYS_futex` syscall): **~108.4 ns / wake** - With `waiters` check (`cv->waiters` load in user space): **~4.2 ns / wake** - **Speedup: ~26x faster**, completely bypassing the kernel context transition. In tight signaling bursts (10,000 wakes), this avoids over **1.0 ms** of redundant kernel CPU time. - **Contended Handoff (waiter sleeping)**: - The two atomic operations on the wait path (`__atomic_add_fetch` upon entering `cv_wait`, `__atomic_sub_fetch` upon leaving) total **~10–14 ns**. - Compared to the ~2,500–4,800 ns required for the kernel to deschedule, context switch, and wake the sleeping thread via futex, two 5 ns user-space atomic operations represent <0.3% of the handoff latency and are well within run-to-run noise. #### Where this impacts Hookshot (`nn_os_sync.c`) While `nn_os_sync.c` maintains a custom `mwait` sleeper count for `MutexType`, **none of the other guest synchronization primitives track sleepers before signaling condition variables**: 1. **`event_signal` (`SignalEvent`, `SignalLightEvent`, `SignalSystemEvent`)** (`nn_os_sync.c:578`): - Invoked frequently on guest events, frame pacing, and GPU fence notifications. Unconditionally calls `WakeAllConditionVariable(&e->cv)` even when no thread is currently blocked. 2. **`queue_put` & `queue_get` (`SendMessageQueue`, `ReceiveMessageQueue`)** (`nn_os_sync.c:896`, `921`): - Every put calls `WakeAllConditionVariable(&q->cv)` to wake potential receivers. - Every take calls `WakeAllConditionVariable(&q->cv)` to wake potential producers. - For streaming/audio queues where items are continually produced and consumed before the queue either fills or empties, this resulted in continuous kernel syscall traps on uncontended wakes. 3. **`rw_unread` & `rw_unwrite` (`ReleaseReadLock`, `ReleaseWriteLock`)** (`nn_os_sync.c:987`, `1006`): - Unconditionally calls `WakeAllConditionVariable(&l->cv)` on every read and write unlock. 4. **`sema_release` (`ReleaseSemaphore`, `ReleaseLightSemaphore`)** (`nn_os_sync.c:850`): - Unconditionally calls `WakeAllConditionVariable(&s->cv)`. 5. **Multi-Wait Holder (`poke_multiwait`)** (`nn_os_sync.c:287`): - Unconditionally calls `WakeAllConditionVariable(&mw_cv)`. Tracking `waiters` directly within `CONDITION_VARIABLE` safeguards all of these Win32 emulation call sites uniformly in user space without requiring per-primitive wrapper counters. ### 3. PR Description & Validation - PR description has been updated to reflect the current diff (omits `WakeByAddressSingle` / "thundering herds"). - Ran test suite via `tests/run_all.py`: **359 passed, 0 failed, 52 skipped**.

Erfboom (Migrated from github.com) reviewed

2026-11-11 23:12:42 +00:00

host/posix/win32_posix.c

					
				@ -216,10 +220,17 @@ static void thread_finish(DWORD code) {

				    self_thread = NULL;

Good catch, simplified to if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice); in commit 969bc802.

Good catch, simplified to `if (nice && tid) setpriority(PRIO_PROCESS, (id_t)tid, nice);` in commit 969bc802.

github-sync commented

2026-11-12 14:42:50 +00:00

@Joey-Magnus approved on GitHub · 2026-11-12 14:33 UTC · original

All three of my points are addressed at 969bc802: WakeByAddressSingle is back to waking every waiter, the priority stores and loads in apply_thread_priority (host/posix/win32_posix.c:223) and thread_entry are seq_cst so the setter and the new thread can't both miss each other, and you answered the measurement question.

I merged it into current main and ran tests/run_all.py: all pass except test_lift_cache.py, which fails on main here too. The CI reds were main's at the time and are fixed there, so a re-run should clear them.

The waiters ordering is right: the waiter reads seq before counting itself and the waker bumps seq before reading the count, so a zero read means the waiter's FUTEX_WAIT sees the new seq and returns. Every CONDITION_VARIABLE is a host-side struct, so growing it to 8 bytes is safe. The ~26x is a microbenchmark of one uncontended wake, not a frame time from the game; please say so in the description rather than under Impact. Otherwise I'd merge it as is.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2487#pullrequestreview-5876214293) All three of my points are addressed at 969bc802: `WakeByAddressSingle` is back to waking every waiter, the priority stores and loads in `apply_thread_priority` (`host/posix/win32_posix.c:223`) and `thread_entry` are seq_cst so the setter and the new thread can't both miss each other, and you answered the measurement question. I merged it into current main and ran `tests/run_all.py`: all pass except `test_lift_cache.py`, which fails on main here too. The CI reds were main's at the time and are fixed there, so a re-run should clear them. The `waiters` ordering is right: the waiter reads `seq` before counting itself and the waker bumps `seq` before reading the count, so a zero read means the waiter's `FUTEX_WAIT` sees the new `seq` and returns. Every `CONDITION_VARIABLE` is a host-side struct, so growing it to 8 bytes is safe. The ~26x is a microbenchmark of one uncontended wake, not a frame time from the game; please say so in the description rather than under Impact. Otherwise I'd merge it as is.

github-sync referenced this pull request

2026-11-12 16:56:04 +00:00

fix(host): BroadcastConditionVariable and the timed semaphore acquires reach the HLE's objects #2765

github-sync referenced this pull request

2026-11-12 17:58:42 +00:00

perf(host): fewer idle game threads poll through short sleeps and spins on 7+ thread CPUs #2709

github-sync referenced this pull request

2026-11-12 18:00:05 +00:00

fix(host): retain linked synchronization objects for later reclamation #2858

Commenting is not possible because the repository is archived.

No reviewers

hckrman101