[Erfboom]

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

[Joey-Magnus]

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?

[ghost]

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

[ghost]

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]

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

[Erfboom]

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.

[Joey-Magnus]

All three of my points are addressed at 969bc80: 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.