Repository navigation
fix(host): BroadcastConditionVariable and the timed semaphore acquires reach the HLE's objects - #2765
klimovich008 wants to merge 3 commits into
Conversation
74a429c to
563a184
Compare
…uires on the HLE's objects The HLE replaces the nn::os condition variable and semaphore functions with host objects, but left some siblings to the game's own SDK code, which acts on guest words the HLE never writes and signals kernel keys no HLE waiter sleeps on. - BroadcastConditionVariable woke nobody (the movie framework and NVIDIA's OS layer call it, with untimed waiters). It now moves the generation and wakes all, and each waiter not yet owed the mutex is owed it (the EvilOOT hand-off; HOS_NO_HANDOFF=1: none). - TimedAcquireSemaphore, TimedAcquireLightSemaphore and TryAcquireLightSemaphore take the HLE's count (sema_acquire_for). Its deadline saturates, and its waits are noted for HOS_CHAIN. - wait_until sleeps in bounded steps: a finite 0xFFFFFFFF ms is INFINITE on Windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Joey-Magnus
left a comment
There was a problem hiding this comment.
I merged this into current main. tests/test_os_lifted_siblings.py and tests/test_multiwait_wake.py pass. The full tests/run_all.py passes except tests/test_lift_cache.py, which fails on main here too, and test_recorder_sync, which timed out under load.
The code looks right. The Broadcast HLE (host/nn_os_sync.c) bumps gen under cv->lk and wakes all waiters, makes the same hand-off Signal does (capped at cv_reserve's 8), and respects HOS_NO_HANDOFF. sema_acquire_for saturates the deadline and tries once for a timeout of 0 or less, and the 0x7FFFFFFF ms cap in wait_until avoids INFINITE.
My concern is reach. Until now, every Broadcast caller (ALooper, LiveDataSource, NvOs) slept until some unrelated Signal. Now they wake on time. That's the fix, but it changes timing on every movie and NVN OS-layer path, and nothing has been run in game. Please boot to the title, play a scene with a movie (the opening, or one of the reported hangs), and add that run to the description. Then I'd merge it.
ghost
left a comment
There was a problem hiding this comment.
Thanks, this is a real gap and the code holds up. I went through it for lost and spurious wake-ups and the timeout arithmetic, and agree with Joey-Magnus's read:
- Broadcast:
genmoves undercv->lkand the wake-all comes after, so a waiter that snapshotted the old generation can't miss it. The reservations balance: one per waiter counted at the broadcast, capped at 8, and every one of those waiters leaves under the same lock (a timed waiter that is timing out included), so nothing stays owed to nobody. Mixed mutexes andHOS_NO_HANDOFF=1fall back the same way Signal does. sema_acquire_for: it checks the count under the lock, loops on spurious and vcore wake-ups, takes a release that races the deadline, and a timeout of 0 or less tries once. The saturated deadline can't wrap.wait_until: clamping to 0x7FFFFFFF ms fixes both platforms. Windows treats 0xFFFFFFFF asINFINITE, and so does the POSIX shim (ms == INFINITE). All four callers loop back to their deadline.- CI is green and the branch merges cleanly.
What's missing is the run in the game, the same thing Joey-Magnus asked for. Every Broadcast caller (the movie framework's queues, NVIDIA's OS layer) used to sleep until some unrelated Signal and now wakes on time. That is the fix, but it changes timing on paths nothing here has run. Before this goes in, please:
- Boot to the title and play at least one scene with a movie (the opening, or one where a hang was reported). Say whether it plays through on Windows and, if you can, on Linux.
- Show that the new HLEs are actually reached in that run, e.g. with a temporary count of Broadcast and TimedAcquireSemaphore calls, or
HOS_CHAINnotes on a movie condition variable. If a hang reproduces on main, show it gone with the change.
A question and two small nits:
- The plain
TryAcquireSemaphore(not the light one) isn't HLE'd either. Is it absent from the image's imports, or was it left out on purpose? If it is imported, it has the same gap as the light try. - nit:
TimedWaitConditionVariable,event_waitand the queue's timed paths still computehos_now_ns() + timeoutwithout saturating, so a huge TimeSpan wraps negative and times out at once. A small shareddeadline_after(ns)used by all of them would fix that. It's older than this PR, so a follow-up is fine. - nit: the fixture stubs
wait_until, so the new clamp isn't covered. A line check on the0x7FFFFFFFcap would pin it.
Overlap: #2708 touches the code right next to this (the Signal early-out when nothing waits, and an atomic cv_waiting) but doesn't conflict textually. Either order works. Once both are in, Broadcast could take the same early-out for consistency. #2709 (spin) and #2487 (POSIX futex waiter count) don't touch this code, and #2487's wake skip is safe for the wake-all here.
| /* an acquire that waits at most ns (<= 0: does not wait); whether it got one */ | ||
| static int sema_acquire_for(Obj *s, int64_t ns) { | ||
| const int64_t now = hos_now_ns(); | ||
| const int64_t deadline = ns <= 0 ? now : ns > INT64_MAX - now ? INT64_MAX : now + ns; /* (a huge TimeSpan: no wrap) */ |
There was a problem hiding this comment.
nit: this saturation is right. The other timed waits in this file (TimedWaitConditionVariable, event_wait, the queue) still wrap on a huge TimeSpan. A shared helper would cover them all (could be a follow-up).
|
Thanks both. The in-game run is now in the description (In game). On Windows (RX 9070 XT, base main 4678050) the build boots to the title and plays the prologue movie from a new game:
hckrman101's points:
🤖 Generated with Claude Code |
…s by role, not by symbol The fixture stubs wait_until, so a line check now pins its 0x7FFFFFFF ms cap (hckrman101's review). The comments in host/nn_os_sync.c and the test's docstring name the Broadcast and timed-acquire callers by role (the movie framework's reply and buffer queues, NVIDIA's OS layer, the C library) rather than by the image's symbols, as the description does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The HLE replaces the nn::os condition variable and semaphore functions with host objects, but a few siblings were left to the game's own SDK code. That code acts on guest words the HLE never writes, and signals kernel keys that no HLE waiter sleeps on. Some of these siblings have callers.
The problem
BroadcastConditionVariablewoke nobody.svcSignalProcessWideKey). The HLE'sWaitConditionVariablesleeps on its host object until the object's generation moves, and only the HLE's Signal moves it.TimedAcquireSemaphorenever saw an HLE release. The C library's timed semaphore wait and NVIDIA's OS layer call it. The light-semaphore timed and try acquires have the same gap but no direct callers.The change
host/nn_os_sync.c):cv->lk, it moves the generation and wakes all waiters.cv_reserve): one mutex for all of them, at most 8 owed.HOS_NO_HANDOFF=1turns it off, as for Signal.TimedAcquireSemaphore,TimedAcquireLightSemaphore,TryAcquireLightSemaphore: these take the HLE's count throughsema_acquire_for, which waits up to the timeout. A timeout of 0 or less tries once.wait_until: sleeps in steps of at most 0x7FFFFFFF ms, because a finite wait of 0xFFFFFFFF ms isINFINITEon Windows. Its callers already wait again until their deadline.HOS_CHAIN, likesema_acquire's. Its deadline saturates for a huge TimeSpan.How these were found: the image's exports were compared with the HLE's list, per sync object type. Siblings still left to the SDK that have direct callers:
StopTimerEvent(the C library's timers);RegisterSystemWorkerHandler.The HLE timer has no separate "signalled" state, so a correct Stop needs a model change; it is left for a follow-up.
Tested
tests/test_os_lifted_siblings.py(new) runs the real code cut from the source (cv_enter,cv_leave,cv_reserve, the Broadcast HLE,sema_acquire_for) over counting stubs and a fake clock.HOS_NO_HANDOFFunset,1and0.HOS_CHAINnotes.sema_acquire_for.tests/run_all.py: 425 passed. The 5 failures are this PC's toolchain and fail the same way on main:nx2/test_nup_paths.py: symlinks on Windows;test_spawn_emit,test_fpx_sticky:<algorithm>not found;test_pipeline_play_cache,test_prepare_stall:NOMINMAXredefined.host/*.c; a gpt-6.1-sol review (three rounds, last clean).In game
Boot to title and prologue movie OK; Broadcast is not reached in this scene, so the movie-hang link is not shown.
Not tested
🤖 Generated with Claude Code