os: no vcore checkpoint in WaitAny's scan while it holds mw_lock #1696

Merged

klimovich008 merged 1 commit from os-waitany-scan-no-checkpoint into main

2026-11-10 16:01:14 +00:00

klimovich008 commented

2026-11-10 13:45:55 +00:00

(Migrated from github.com)

What this fixes

Since #1353, the game occasionally freezes for about a quarter of a second (~270 ms). WaitAny's scan could give its virtual core up while holding mw_lock; signalling threads then block on that lock, and the scanner waits for the 250 ms vcore escape.

Why merge

Keeps #1353's CPU saving without its stalls.
The scan's lookups and event reads take no checkpoint under the lock. WaitAny still checkpoints once before taking it, and its sleep releases the lock first.

Change

host/nn_os_sync.c: obj_nockpt (lookup without the checkpoint) and event_peek (event_wait(e, 0, 1)'s try path) for every read under mw_lock.
tests/test_multiwait_wake.py: runs the real scan against counting stubs in every wait mode and event state; allowlist check that nothing under mw_lock, or holding a lock the scan takes, can wait for a core.

Tested

python -I -B tests/test_multiwait_wake.py --cxx g++ (and clang++): fails on main at wait_any (holding mw_lock), passes here.
RX 9070 XT, driver 32.0.32015.12012, Kokiri and Hyrule, 32:9, same binary with HOS_MW_WAKE=all as control. Frames over 140 ms: 0 of 18 runs, against 8 of 29 with #1353 alone. No frame over 100 ms. CPU ms/frame −8 to −12% (Kokiri 14, Hyrule 4 pairs), fps unchanged.

Not covered

Windows AMD only. Other ~250 ms vcore escapes outside the multi-wait (#1420, #995) are separate.

🤖 Generated with Claude Code

## What this fixes Since #1353, the game occasionally freezes for about a quarter of a second (~270 ms). WaitAny's scan could give its virtual core up while holding `mw_lock`; signalling threads then block on that lock, and the scanner waits for the 250 ms vcore escape. ## Why merge - Keeps #1353's CPU saving without its stalls. - The scan's lookups and event reads take no checkpoint under the lock. WaitAny still checkpoints once before taking it, and its sleep releases the lock first. ## Change - `host/nn_os_sync.c`: `obj_nockpt` (lookup without the checkpoint) and `event_peek` (`event_wait(e, 0, 1)`'s try path) for every read under `mw_lock`. - `tests/test_multiwait_wake.py`: runs the real scan against counting stubs in every wait mode and event state; allowlist check that nothing under `mw_lock`, or holding a lock the scan takes, can wait for a core. ## Tested - `python -I -B tests/test_multiwait_wake.py --cxx g++` (and clang++): fails on main at `wait_any (holding mw_lock)`, passes here. - RX 9070 XT, driver 32.0.32015.12012, Kokiri and Hyrule, 32:9, same binary with `HOS_MW_WAKE=all` as control. Frames over 140 ms: 0 of 18 runs, against 8 of 29 with #1353 alone. No frame over 100 ms. CPU ms/frame −8 to −12% (Kokiri 14, Hyrule 4 pairs), fps unchanged. ## Not covered - Windows AMD only. Other ~250 ms vcore escapes outside the multi-wait (#1420, #995) are separate. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 15:56:32 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks right to me. The analysis checks out: obj()'s checkpoint can go into vc_yield -> vc_release/vc_acquire while the scanner holds mw_lock, and a signaller that holds a core then blocks in poke_multiwait on that SRW lock without giving the core back, so the scanner only gets going again at the 250 ms escape. With this change nothing under mw_lock can wait for a core. obj_nockpt can still take table_lock on a miss, but nobody holding table_lock checkpoints. event_peek takes e->lk inside mw_lock, the same order event_wait's try path already used, and event_signal drops e->lk before it pokes. The only place that gives the core up is hos_cv_sleep, which releases the lock before hos_vcore_back.

No starvation that I can see. WaitAny still checkpoints once at entry (obj(multiwait)), the scan is bounded by nlinks, and if a kick lands during the scan it stays set on vc_kick. It gets honoured at the next sleep (the core is released) or at the next nn::os call. Wakeups behave as before: the scan reads exactly what it read before, minus the checkpoints. The change is platform-neutral, so Windows and Linux behave the same. The test is already in run_all.py (--cxx {cxx}), and CI is green on both OSes.

One thing to coordinate: draft #1543 (mikaba-eu) makes the same fix in nn_os_sync.c. It splits obj() into obj() + obj_cached() and switches holder_ready/wait_any to it, and adds tests/test_multiwait_checkpoint.py. Whichever lands second will conflict and should drop its copy. The test here covers more, so I'd keep this one.

Looks right to me. The analysis checks out: `obj()`'s checkpoint can go into `vc_yield` -> `vc_release`/`vc_acquire` while the scanner holds `mw_lock`, and a signaller that holds a core then blocks in `poke_multiwait` on that SRW lock without giving the core back, so the scanner only gets going again at the 250 ms escape. With this change nothing under `mw_lock` can wait for a core. `obj_nockpt` can still take `table_lock` on a miss, but nobody holding `table_lock` checkpoints. `event_peek` takes `e->lk` inside `mw_lock`, the same order `event_wait`'s try path already used, and `event_signal` drops `e->lk` before it pokes. The only place that gives the core up is `hos_cv_sleep`, which releases the lock before `hos_vcore_back`. No starvation that I can see. WaitAny still checkpoints once at entry (`obj(multiwait)`), the scan is bounded by `nlinks`, and if a kick lands during the scan it stays set on `vc_kick`. It gets honoured at the next sleep (the core is released) or at the next nn::os call. Wakeups behave as before: the scan reads exactly what it read before, minus the checkpoints. The change is platform-neutral, so Windows and Linux behave the same. The test is already in `run_all.py` (`--cxx {cxx}`), and CI is green on both OSes. One thing to coordinate: draft #1543 (mikaba-eu) makes the same fix in `nn_os_sync.c`. It splits `obj()` into `obj()` + `obj_cached()` and switches `holder_ready`/`wait_any` to it, and adds `tests/test_multiwait_checkpoint.py`. Whichever lands second will conflict and should drop its copy. The test here covers more, so I'd keep this one.

host/nn_os_sync.c

Nit: as far as I can tell this behaves exactly like event_wait(e, 0, 1). With try_only set, the spin, the sleep and wake_lat_note are all skipped already. So it doesn't change behaviour, it just makes the guarantee explicit (and easier for the test's call-graph check to see). Fine to keep. The comment could say it's equivalent today and that it exists so the scan never picks up a scheduling point if event_wait changes later.

Nit: as far as I can tell this behaves exactly like `event_wait(e, 0, 1)`. With `try_only` set, the spin, the sleep and `wake_lat_note` are all skipped already. So it doesn't change behaviour, it just makes the guarantee explicit (and easier for the test's call-graph check to see). Fine to keep. The comment could say it's equivalent today and that it exists so the scan never picks up a scheduling point if `event_wait` changes later.