MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vcores): an escaped thread takes a core again after its next block (#1929) #2302

Merged

mooki000 merged 1 commit from vcore-escape into main

2026-11-11 16:40:33 +00:00

mooki000 commented

2026-11-11 14:07:25 +00:00

(Migrated from github.com)

What changed

host/hos_thread.c: an escape (vc_acquire letting a waiter run without a core) marks the thread (vc_escaped).
hos_vcore_away answers 1 for such a thread at its next block, so the if (away) hos_vcore_back(); after that block
takes a core again, as for every other thread. Before, it answered 0 (the thread held no core to give up), every later
hos_vcore_back was skipped, and the thread ran coreless, reporting its ideal core's number, until it ended. Only the
outermost of nested blocks takes the core again.
tests/test_vcore_escape.py (new): compiles the actual HosThread, hos_thread_core and the virtual-core block of
hos_thread.c with a single-threaded harness and checks: an escape, then a block through hos_vcore_away/back and
through hos_cv_sleep; nested blocks; a thread that never escaped. It fails on main (7 checks) and passes with this,
with g++ and clang++.
tests/invalidate_vcore_test.cpp: its fixture thread gets the new field.

This does not change when an escape happens (#2127 moved that to 2 s); it bounds how long one lasts.

How it was tested

Linux (Arch), RTX 4090, NVIDIA 615.78.08, headless, the issue's steps (fresh profile, Start about 95 s into the title,
165 s per run), three runs per arm interleaved in one sitting, escape at 250 ms so that escapes happen (at 2 s there were
none in three runs on main). The game was built with this patch on #2127's commit. Not a quiet machine: load average 4-8
at the start of each run except one main run (66).

	escapes per run	crashes	title fps / 1% low	after Start fps / 1% low / worst frame
main's code at 250 ms	7, 8, 2	0	56.4 / 27.3	59.9 / 37.1 / 104 ms
this change at 250 ms	16, 20, 27	0	60.0 / 30.9	60.0 / 37.6 / 47 ms

There are more escapes with the change, and that is expected from what it does: on main a thread escapes once and then
never waits for a core again; with the change it waits again after its next block and can escape again. The same
low-priority threads (bake::ModuleMgr and BakeProbe at 28, PreActorModelCompose 26, tera::FileResourceMgr and
aglTextureDataMgr::Compaction 25) escaped again and again in one run (by name up to 11 times), in the scene load after Start, mostly while
BaseProcCreateAndDeleteThread (23) held their core. Frame times were the same within the spread (the main runs' title
numbers include the run started at load 66). (Corrected 2026-11-11 16:19 UTC: I had written "up to four times each in one run". Several threads share one name, so these logs give no count per thread; by name the most was PreActorModelCompose, 11 times in the first of the three runs.)

tests/run_all.py on this branch's head (4608fb49, on main 49f75dc8): 363 passed, 2 failed, 10 skipped. The two
failures are the ones this machine has on every branch: test_updater_asset_digest.py (only the .NET 10 SDK here, the
fixture needs .NET 8) and test_wayland_libdecor_hint.py (clang 23's -Wunused-but-set-global under -Werror).

Not tested

Windows, AMD, the Steam Deck; anything past the first minute after Start. With the 2 s default, how often a thread
escapes twice was not measured (no escapes in three runs).

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 [email protected]

A thread that escapes a long wait for a virtual core keeps running without one for the rest of its life, not just for the one wait, so it shares the per-core state of whichever thread holds the core it reports (#1929). This ends the escape at the thread's next block. ## What changed - `host/hos_thread.c`: an escape (`vc_acquire` letting a waiter run without a core) marks the thread (`vc_escaped`). `hos_vcore_away` answers 1 for such a thread at its next block, so the `if (away) hos_vcore_back();` after that block takes a core again, as for every other thread. Before, it answered 0 (the thread held no core to give up), every later `hos_vcore_back` was skipped, and the thread ran coreless, reporting its ideal core's number, until it ended. Only the outermost of nested blocks takes the core again. - `tests/test_vcore_escape.py` (new): compiles the actual `HosThread`, `hos_thread_core` and the virtual-core block of `hos_thread.c` with a single-threaded harness and checks: an escape, then a block through `hos_vcore_away`/`back` and through `hos_cv_sleep`; nested blocks; a thread that never escaped. It fails on main (7 checks) and passes with this, with g++ and clang++. - `tests/invalidate_vcore_test.cpp`: its fixture thread gets the new field. This does not change when an escape happens (#2127 moved that to 2 s); it bounds how long one lasts. ## How it was tested Linux (Arch), RTX 4090, NVIDIA 615.78.08, headless, the issue's steps (fresh profile, Start about 95 s into the title, 165 s per run), three runs per arm interleaved in one sitting, escape at 250 ms so that escapes happen (at 2 s there were none in three runs on main). The game was built with this patch on #2127's commit. Not a quiet machine: load average 4-8 at the start of each run except one main run (66). | | escapes per run | crashes | title fps / 1% low | after Start fps / 1% low / worst frame | |---|---|---|---|---| | main's code at 250 ms | 7, 8, 2 | 0 | 56.4 / 27.3 | 59.9 / 37.1 / 104 ms | | this change at 250 ms | 16, 20, 27 | 0 | 60.0 / 30.9 | 60.0 / 37.6 / 47 ms | There are more escapes with the change, and that is expected from what it does: on main a thread escapes once and then never waits for a core again; with the change it waits again after its next block and can escape again. The same low-priority threads (bake::ModuleMgr and BakeProbe at 28, PreActorModelCompose 26, tera::FileResourceMgr and aglTextureDataMgr::Compaction 25) escaped again and again in one run (by name up to 11 times), in the scene load after Start, mostly while BaseProcCreateAndDeleteThread (23) held their core. Frame times were the same within the spread (the main runs' title numbers include the run started at load 66). *(Corrected 2026-11-11 16:19 UTC: I had written "up to four times each in one run". Several threads share one name, so these logs give no count per thread; by name the most was `PreActorModelCompose`, 11 times in the first of the three runs.)* `tests/run_all.py` on this branch's head (4608fb49, on main 49f75dc8): 363 passed, 2 failed, 10 skipped. The two failures are the ones this machine has on every branch: `test_updater_asset_digest.py` (only the .NET 10 SDK here, the fixture needs .NET 8) and `test_wayland_libdecor_hint.py` (clang 23's `-Wunused-but-set-global` under `-Werror`). ## Not tested Windows, AMD, the Steam Deck; anything past the first minute after Start. With the 2 s default, how often a thread escapes twice was not measured (no escapes in three runs). Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <[email protected]>

CIB commented

2026-11-11 15:34:16 +00:00

(Migrated from github.com)

Two things from our side that bear on this PR.

1. The “escape never ends” finding matches our instrumentation. An escaped thread keeps running coreless with the core number it cached at thread start; hos_vcore_away() returns 0 for it, so hos_vcore_back() never re-takes a core. Bounding the escape to one block is a clear improvement for any escape that remains.

2. It does not remove the corruption in the first concurrent interval. The escaped waiter still runs concurrently with the holder of the core it reports. Our host-stack capture at the 250 ms escape shows the holder is a progressing guest decompressor (priority 19 vs the waiter's 26), polling at every back-edge (26 polls, 18 compiled). Because the holder is better priority, vc_kick is never set (vc_acquire only kicks a worse or equal holder), so every poll is a no-op; the worse waiter escapes at 250 ms and shares the decompressor’s per-core data.

We have a complementary condition in vc_acquire: record the last checkpoint per thread; escape only if no holder on the waiter’s allowed cores has checkpointed since the waiter started. A progressing holder keeps its core; a holder stuck in host code (no checkpoints) still triggers the escape. On Linux main + anonymous memory at 250 ms that removes the decompressor escape (9 logged → 0) and the no-escape arm is 2/2 clean. Your branch has #2127’s 2 s default, so these counts need re-baselining rather than direct comparison.

The two compose: progress-aware decides whether, vc_escaped bounds how long. We plan to rebase our patch on top of #2302. A test that the escaped thread re-takes a core at its next block (you have it) plus one for the progress condition would cover both.

🤖 Generated with opencode using DeepSeek V4.1 Flash.

Two things from our side that bear on this PR. **1. The “escape never ends” finding matches our instrumentation.** An escaped thread keeps running coreless with the core number it cached at thread start; `hos_vcore_away()` returns 0 for it, so `hos_vcore_back()` never re-takes a core. Bounding the escape to one block is a clear improvement for any escape that remains. **2. It does not remove the corruption in the first concurrent interval.** The escaped waiter still runs concurrently with the holder of the core it reports. Our host-stack capture at the 250 ms escape shows the holder is a *progressing* guest decompressor (priority 19 vs the waiter's 26), polling at every back-edge (26 polls, 18 compiled). Because the holder is better priority, `vc_kick` is never set (`vc_acquire` only kicks a worse or equal holder), so every poll is a no-op; the worse waiter escapes at 250 ms and shares the decompressor’s per-core data. We have a complementary condition in `vc_acquire`: record the last checkpoint per thread; escape only if **no holder on the waiter’s allowed cores has checkpointed since the waiter started**. A progressing holder keeps its core; a holder stuck in host code (no checkpoints) still triggers the escape. On Linux `main` + anonymous memory at 250 ms that removes the decompressor escape (9 logged → 0) and the no-escape arm is 2/2 clean. Your branch has #2127’s 2 s default, so these counts need re-baselining rather than direct comparison. The two compose: progress-aware decides *whether*, `vc_escaped` bounds *how long*. We plan to rebase our patch on top of #2302. A test that the escaped thread re-takes a core at its next block (you have it) plus one for the progress condition would cover both. --- 🤖 Generated with [opencode](https://opencode.ai) using DeepSeek V4.1 Flash.

mooki000 commented

2026-11-11 16:19:44 +00:00

(Migrated from github.com)

Thanks. I read the two the same way: yours decides whether a thread escapes, this one bounds how long. Rebasing yours on top is fine with me.

Our counts are at 250 ms already, so they compare with yours directly. The runs set HOS_VCORE_ESCAPE_US=250000 HOS_VCORE_ESCAPE_EQ_US=250000 in the environment (vc_acquire reads both: host/hos_thread.c:1363-1364). The base was 880cf67f, #2127's commit; its only difference from the main before it (364d1994) is those two defaults and the comment. The tested binary was this patch on 880cf67f; the branch is the same patch on 49f75dc8 (I compared the hunks).

A finding that bears on your rule. With this change at 250 ms the escapes per run went up: 16, 20, 27 against 7, 8, 2 on main's code (3 runs each, the issue's steps, 165 s). Each one is short now, but the same thread names escape again after their next block. Who held the core, from the escape lines of those three runs (63 escapes, one holder named at each):

in 57 the holder had a better priority than the waiter: BaseProcCreateAndDeleteThread (23) in 37; ParallelDecompThread (19) in 4, with PreActorModelCompose (26) and GeneralFormatAsync_ (24) waiting on cores 4 and 5, which is your case; sead::MainThread, Resource Memory, the movie input thread and others in the rest;
in 6 the holder was worse and had been asked: the movie input thread inside the decoder (what #2322 removes).

In a diagnostic run on main's scheduler, BaseProcCreateAndDeleteThread was the holder at 5 escapes of that same group, and each time it had held the core for only 0.1 to 2.2 ms, in game code: the core was changing hands among better threads while the worse ones waited their 250 ms. That is a holder making progress, so I expect your rule to keep exactly these waiters waiting, and they are the escapes this change multiplies. That is expected, not yet run: a local reading of your sketch, on main's code and on main with our three changes, is queued here, and its counts go to #1929 when it has run.

A correction to my own text. I wrote that these threads escaped "up to four times each in one run". By name, PreActorModelCompose escaped 11 times in the first of the three runs. Names are not unique (several threads are called PreActorModelCompose; on main's code, where a thread can escape only once, that name escapes up to 4 times in a run), so these logs cannot give a per-thread count. I have corrected the sentence in the description and in my comment on #1929, each with a dated note.

On the tests: this branch has the one for the next block (tests/test_vcore_escape.py); the one for the progress condition belongs with your patch.

Thanks. I read the two the same way: yours decides whether a thread escapes, this one bounds how long. Rebasing yours on top is fine with me. **Our counts are at 250 ms already, so they compare with yours directly.** The runs set `HOS_VCORE_ESCAPE_US=250000 HOS_VCORE_ESCAPE_EQ_US=250000` in the environment (`vc_acquire` reads both: `host/hos_thread.c:1363-1364`). The base was 880cf67f, #2127's commit; its only difference from the main before it (364d1994) is those two defaults and the comment. The tested binary was this patch on 880cf67f; the branch is the same patch on 49f75dc8 (I compared the hunks). **A finding that bears on your rule.** With this change at 250 ms the escapes per run went up: 16, 20, 27 against 7, 8, 2 on main's code (3 runs each, the issue's steps, 165 s). Each one is short now, but the same thread names escape again after their next block. Who held the core, from the escape lines of those three runs (63 escapes, one holder named at each): - in 57 the holder had a better priority than the waiter: `BaseProcCreateAndDeleteThread` (23) in 37; `ParallelDecompThread` (19) in 4, with `PreActorModelCompose` (26) and `GeneralFormatAsync_` (24) waiting on cores 4 and 5, which is your case; `sead::MainThread`, `Resource Memory`, the movie input thread and others in the rest; - in 6 the holder was worse and had been asked: the movie input thread inside the decoder (what #2322 removes). In a diagnostic run on main's scheduler, `BaseProcCreateAndDeleteThread` was the holder at 5 escapes of that same group, and each time it had held the core for only 0.1 to 2.2 ms, in game code: the core was changing hands among better threads while the worse ones waited their 250 ms. That is a holder making progress, so I expect your rule to keep exactly these waiters waiting, and they are the escapes this change multiplies. That is expected, not yet run: a local reading of your sketch, on main's code and on main with our three changes, is queued here, and its counts go to #1929 when it has run. **A correction to my own text.** I wrote that these threads escaped "up to four times each in one run". By name, `PreActorModelCompose` escaped 11 times in the first of the three runs. Names are not unique (several threads are called `PreActorModelCompose`; on main's code, where a thread can escape only once, that name escapes up to 4 times in a run), so these logs cannot give a per-thread count. I have corrected the sentence in the description and in my comment on #1929, each with a dated note. On the tests: this branch has the one for the next block (`tests/test_vcore_escape.py`); the one for the progress condition belongs with your patch.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 16:30:09 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks, this is a small, well-aimed fix. Before, hos_vcore_away answered 0 for a thread with no core, so after an escape every if (away) hos_vcore_back(); was skipped and the thread ran coreless until it exited. Answering 1 once at the next block makes the escape last one stretch of guest code, which is what the scheduler comment already says the escape is meant to be (a deadlock breaker).

I went through the accounting and found no race:

vc_escaped is only ever touched by its own thread: it is set in vc_acquire (under vc_lock, on the waiter's own thread) and read and cleared in hos_vcore_away. vc_grant only hands cores to queued threads, and the escaped thread was dequeued, so no other thread can give it a core while the flag is set. That means vc_escaped implies !vc_held for as long as the flag is up.
Nested blocks work as before. The outer away answers 1 and clears the flag, inner ones answer 0, and only the outer back takes a core. That covers socket_poll -> hos_sleep_ns, for example.
vc_yield and hos_vcore_checkpoint return early for a coreless thread, so a yield doesn't cut the escape short. Only a real block does, which is right.
Exit paths (thread_main, svcExitThread) call hos_vcore_away() and ignore the result. Clearing the flag there does no harm.
hos_cv_sleep now drops and retakes lk around the core wait for an escaped thread, the same as for any other holder. The spurious wake-up is already documented there.

One thing to keep an eye on, not a blocker. When an escape really was breaking a stall (the holder keeps the core until the waiter makes progress), the old code paid the wait once. Now the waiter can pay it again after every block. With the 2 s default and #1929's numbers (no escapes at 2 s in three runs) I expect this to be rare. If someone later reports repeated ~2 s hitches with a ran without a core line per hitch for the same thread, this is where to look. It also fits CIB's progress-aware rule, which would decide whether to escape, and you've both agreed to stack that on top of this.

A side note on the issue's evidence, also not for this PR. The CORETLS ... cached_core_plus1=5 live_core=4 line suggests the game also keys some state on the core number it cached at thread start. A thread that takes a core again prefers vc_last, but it can land on another core in its mask, so this bounds the escape without guaranteeing the cached index is exclusive. That is the same for any thread that changes cores today, so it is out of scope here.

The test is good: it compiles the real HosThread and the whole vcore block rather than a copy, and it covers away/back, hos_cv_sleep, nesting and the never-escaped case. The invalidate_vcore_test.cpp fixture update is needed and correct.

On the siblings: #2301 (nn_fs.c OpenFile) and #2322 (nn_video.c dec_acquire) touch different files and don't conflict with this textually or in meaning. Each early return there pairs its back with the away, which is what this change relies on: an escaped thread that goes through those paths takes a core again after them. They can land in any order. Landing this one first just means their new away points also end an escape.

CI: the test workflows (Game-free source regressions, Linux AppImage, Windows builder) are waiting for a maintainer to approve the run, so tests/test_vcore_escape.py hasn't run on the Windows (llvm-mingw clang++) runner yet. Worth approving the workflows before merging. NotRexed hasn't weighed in yet either.

Thanks, this is a small, well-aimed fix. Before, `hos_vcore_away` answered 0 for a thread with no core, so after an escape every `if (away) hos_vcore_back();` was skipped and the thread ran coreless until it exited. Answering 1 once at the next block makes the escape last one stretch of guest code, which is what the scheduler comment already says the escape is meant to be (a deadlock breaker). I went through the accounting and found no race: - `vc_escaped` is only ever touched by its own thread: it is set in `vc_acquire` (under `vc_lock`, on the waiter's own thread) and read and cleared in `hos_vcore_away`. `vc_grant` only hands cores to queued threads, and the escaped thread was dequeued, so no other thread can give it a core while the flag is set. That means `vc_escaped` implies `!vc_held` for as long as the flag is up. - Nested blocks work as before. The outer `away` answers 1 and clears the flag, inner ones answer 0, and only the outer `back` takes a core. That covers `socket_poll` -> `hos_sleep_ns`, for example. - `vc_yield` and `hos_vcore_checkpoint` return early for a coreless thread, so a yield doesn't cut the escape short. Only a real block does, which is right. - Exit paths (`thread_main`, `svcExitThread`) call `hos_vcore_away()` and ignore the result. Clearing the flag there does no harm. - `hos_cv_sleep` now drops and retakes `lk` around the core wait for an escaped thread, the same as for any other holder. The spurious wake-up is already documented there. One thing to keep an eye on, not a blocker. When an escape really was breaking a stall (the holder keeps the core until the waiter makes progress), the old code paid the wait once. Now the waiter can pay it again after every block. With the 2 s default and #1929's numbers (no escapes at 2 s in three runs) I expect this to be rare. If someone later reports repeated ~2 s hitches with a `ran without a core` line per hitch for the same thread, this is where to look. It also fits CIB's progress-aware rule, which would decide *whether* to escape, and you've both agreed to stack that on top of this. A side note on the issue's evidence, also not for this PR. The `CORETLS ... cached_core_plus1=5 live_core=4` line suggests the game also keys some state on the core number it cached at thread start. A thread that takes a core again prefers `vc_last`, but it can land on another core in its mask, so this bounds the escape without guaranteeing the cached index is exclusive. That is the same for any thread that changes cores today, so it is out of scope here. The test is good: it compiles the real `HosThread` and the whole vcore block rather than a copy, and it covers `away`/`back`, `hos_cv_sleep`, nesting and the never-escaped case. The `invalidate_vcore_test.cpp` fixture update is needed and correct. On the siblings: #2301 (`nn_fs.c` OpenFile) and #2322 (`nn_video.c` `dec_acquire`) touch different files and don't conflict with this textually or in meaning. Each early return there pairs its `back` with the `away`, which is what this change relies on: an escaped thread that goes through those paths takes a core again after them. They can land in any order. Landing this one first just means their new `away` points also end an escape. CI: the test workflows (Game-free source regressions, Linux AppImage, Windows builder) are waiting for a maintainer to approve the run, so `tests/test_vcore_escape.py` hasn't run on the Windows (llvm-mingw clang++) runner yet. Worth approving the workflows before merging. NotRexed hasn't weighed in yet either.

host/hos_thread.c

Nit, optional: the invariant here (vc_escaped set implies no core held) holds today because only the thread-start calls use hos_vcore_back() without an away. Clearing t->vc_escaped in hos_vcore_back too, or in vc_take when t is the current thread, would keep a future unpaired back from leaving a stale flag. A stale flag would make a later inner away answer 1 and take a core in the middle of an outer block. It isn't reachable now.

Nit, optional: the invariant here (`vc_escaped` set implies no core held) holds today because only the thread-start calls use `hos_vcore_back()` without an `away`. Clearing `t->vc_escaped` in `hos_vcore_back` too, or in `vc_take` when `t` is the current thread, would keep a future unpaired `back` from leaving a stale flag. A stale flag would make a later inner `away` answer 1 and take a core in the middle of an outer block. It isn't reachable now.

mooki000 (Migrated from github.com) reviewed

2026-11-11 16:49:36 +00:00

host/hos_thread.c

Done in #2387, as its own change because this one was merged first: hos_vcore_back now clears vc_escaped before vc_acquire, which sets the flag again if the thread escapes again. The new case in tests/test_vcore_escape.py calls back without an away on an escaped thread and then nests two blocks; the inner away must answer 0. It fails on main and passes with the change, with g++ and with clang++. I put the clear in back and not in vc_take, because a queued thread gets its core from vc_grant, which runs on the thread that releases the core, and a t == cur check there would leave the flag up.

Done in #2387, as its own change because this one was merged first: `hos_vcore_back` now clears `vc_escaped` before `vc_acquire`, which sets the flag again if the thread escapes again. The new case in `tests/test_vcore_escape.py` calls `back` without an `away` on an escaped thread and then nests two blocks; the inner `away` must answer 0. It fails on main and passes with the change, with g++ and with clang++. I put the clear in `back` and not in `vc_take`, because a queued thread gets its core from `vc_grant`, which runs on the thread that releases the core, and a `t == cur` check there would leave the flag up.