fix(vcores): hos_vcore_back ends an escape too, so no inner block takes a core early #2387

Merged

mooki000 merged 1 commit from fix/vcore-escape-unpaired-back into main

2026-11-11 19:49:52 +00:00

mooki000 commented

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

(Migrated from github.com)

What this is for: this makes the virtual-core escape fix of #2302 (issue #1929) safe against one kind of future change. @hckrman101 asked for it as an optional point in his review (comment). Nothing that runs today changes, because no call in the code reaches the case: it is there so that a later hos_vcore_back() without its hos_vcore_away() cannot leave an escape flag up. What is measured: the test with g++ and clang++, on this branch and against main's host/hos_thread.c, and the whole of tests/run_all.py on Arch Linux. Not measured: no game run, because no call reaches the changed case. Windows was not tried.

Problem

A guest thread that waits too long for a virtual core runs without one (an escape, vc_acquire), and vc_escaped is set. Since #2302 the thread's next hos_vcore_away() answers 1 and clears the flag, so the hos_vcore_back() paired with it takes a core again.
hos_vcore_back() itself leaves the flag alone. Today every back follows an away that answered 1, except the two at a thread's start (hos_boot_main_thread, thread_main). No escape can be up there yet: the thread is new and zeroed, and that back is its first wait for a core. So the case below cannot happen now.
Suppose a later change calls back without its away on a thread that has escaped. The thread would take a core and keep the flag. Its next outer block would give the core up as usual. A block nested inside that one would then answer 1 and take a core in the middle of the outer block, which is the opposite of what the nesting rule is for.

What changed

host/hos_thread.c hos_vcore_back: clears vc_escaped before vc_acquire, which sets it again if the thread escapes again. So after any back, the flag is up only when the thread is again running without a core. The comment on the struct field says this now.
Why back and not vc_take (the other place the review named): a thread waiting in the queue gets its core from vc_grant. vc_grant runs on the thread that gives the core up, so a clear in vc_take limited to t == cur would miss that path.
tests/test_vcore_escape.py has two new cases.
- An escaped thread calls back without an away, then nests two blocks. The inner away must answer 0, and no core may be taken inside the outer block.
- The same kind of back escapes again while both cores are held. The thread must still take a core after its next block.

How it was tested

Arch Linux, kernel 7.2.15, g++ 16.2.1, clang++ 23.1.3, 2026-11-11. Branch head b52842db on main b744b1ec.

The test on this branch, with g++ and with clang++: ok.
The same test against main's host/hos_thread.c (b744b1ec), with both compilers:

FAIL: after a back without an away, a block inside a block takes no core after it
FAIL: no core is taken inside the outer block after a back without an away

The second case checks where the clear goes. With the clear placed after vc_acquire instead, the flag of an escape that has just happened is wiped, and 8 checks fail with both compilers.
tests/test_invalidate_vcore.py, which cuts hos_vcore_away and hos_vcore_back out of the source, passes with both compilers.
tests/run_all.py: 373 passed, 4 failed, 10 skipped. Two of the failures fail on this machine whatever the change: test_updater_asset_digest.py (only the .NET 10 SDK is installed here, and the fixture needs net8.0) and test_wayland_libdecor_hint.py (clang 23's -Wunused-but-set-global under -Werror). The other two, test_vcore_fs_open.py and test_vcore_video_acquire.py, fail on main itself until #2380 lands: since #2042 they no longer compile.

Not tested

No game run. The changed line runs at every back, but no call today has a flag up there, so I expect no difference in the game. That is an inference from the code, not a measurement.
Windows.

Risks

One store to the thread's own field at each back. The flag is touched only by its own thread, as before.

Closes #2386.

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

**What this is for:** this makes the virtual-core escape fix of #2302 (issue #1929) safe against one kind of future change. @hckrman101 asked for it as an optional point in his review ([comment](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2302)). Nothing that runs today changes, because no call in the code reaches the case: it is there so that a later `hos_vcore_back()` without its `hos_vcore_away()` cannot leave an escape flag up. **What is measured:** the test with g++ and clang++, on this branch and against main's `host/hos_thread.c`, and the whole of `tests/run_all.py` on Arch Linux. **Not measured:** no game run, because no call reaches the changed case. Windows was not tried. ## Problem - A guest thread that waits too long for a virtual core runs without one (an escape, `vc_acquire`), and `vc_escaped` is set. Since #2302 the thread's next `hos_vcore_away()` answers 1 and clears the flag, so the `hos_vcore_back()` paired with it takes a core again. - `hos_vcore_back()` itself leaves the flag alone. Today every `back` follows an `away` that answered 1, except the two at a thread's start (`hos_boot_main_thread`, `thread_main`). No escape can be up there yet: the thread is new and zeroed, and that `back` is its first wait for a core. So the case below cannot happen now. - Suppose a later change calls `back` without its `away` on a thread that has escaped. The thread would take a core and keep the flag. Its next outer block would give the core up as usual. A block nested inside that one would then answer 1 and take a core in the middle of the outer block, which is the opposite of what the nesting rule is for. ## What changed - `host/hos_thread.c` `hos_vcore_back`: clears `vc_escaped` before `vc_acquire`, which sets it again if the thread escapes again. So after any `back`, the flag is up only when the thread is again running without a core. The comment on the struct field says this now. - Why `back` and not `vc_take` (the other place the review named): a thread waiting in the queue gets its core from `vc_grant`. `vc_grant` runs on the thread that gives the core up, so a clear in `vc_take` limited to `t == cur` would miss that path. - `tests/test_vcore_escape.py` has two new cases. - An escaped thread calls `back` without an `away`, then nests two blocks. The inner `away` must answer 0, and no core may be taken inside the outer block. - The same kind of `back` escapes again while both cores are held. The thread must still take a core after its next block. ## How it was tested Arch Linux, kernel 7.2.15, g++ 16.2.1, clang++ 23.1.3, 2026-11-11. Branch head b52842db on main b744b1ec. - The test on this branch, with g++ and with clang++: `ok`. - The same test against main's `host/hos_thread.c` (b744b1ec), with both compilers: ``` FAIL: after a back without an away, a block inside a block takes no core after it FAIL: no core is taken inside the outer block after a back without an away ``` - The second case checks where the clear goes. With the clear placed after `vc_acquire` instead, the flag of an escape that has just happened is wiped, and 8 checks fail with both compilers. - `tests/test_invalidate_vcore.py`, which cuts `hos_vcore_away` and `hos_vcore_back` out of the source, passes with both compilers. - `tests/run_all.py`: 373 passed, 4 failed, 10 skipped. Two of the failures fail on this machine whatever the change: `test_updater_asset_digest.py` (only the .NET 10 SDK is installed here, and the fixture needs net8.0) and `test_wayland_libdecor_hint.py` (clang 23's `-Wunused-but-set-global` under `-Werror`). The other two, `test_vcore_fs_open.py` and `test_vcore_video_acquire.py`, fail on main itself until #2380 lands: since #2042 they no longer compile. ## Not tested - No game run. The changed line runs at every `back`, but no call today has a flag up there, so I expect no difference in the game. That is an inference from the code, not a measurement. - Windows. ## Risks - One store to the thread's own field at each `back`. The flag is touched only by its own thread, as before. Closes #2386. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <[email protected]>

mooki000 commented

2026-11-11 17:08:23 +00:00

(Migrated from github.com)

Rebased onto main b29967bf so the workflows run here: they had waited for an approval on the old head, and main's game-free tests are green again since #2380. The patch is unchanged (git range-diff: the commit is equal), and its test passes on the new base.

Rebased onto main `b29967bf` so the workflows run here: they had waited for an approval on the old head, and main's game-free tests are green again since #2380. The patch is unchanged (`git range-diff`: the commit is equal), and its test passes on the new base.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 17:35:07 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for following up on the #2302 nit. Clearing vc_escaped in hos_vcore_back before vc_acquire is the right spot: vc_acquire sets the flag again if the thread escapes again, so after any back the flag is up only while the thread really is running without a core. Your reason for not putting it in vc_take holds too, since vc_grant runs on the releasing thread.

What I checked:

Races: vc_escaped is still written only by its own thread (vc_acquire on t, hos_vcore_away, now hos_vcore_back). vc_grant/vc_release never touch it, so the extra store at each back adds no cross-thread access.
Nesting stays balanced: the flag can only be up while vc_held == 0, so the outermost away still answers 1 and clears it, inner ones answer 0, and only the outer back takes a core. The paired callers (hos_thread_wait, hos_cv_sleep, the other if (away) hos_vcore_back() sites, the #2301/#2322 users) behave exactly as before. The two unpaired backs at thread start see a zeroed flag, so nothing changes there today, as the description says.
vc_yield goes through vc_release/vc_acquire directly rather than back, so a re-escape there still leaves the flag up until the next block, as intended.
The two new test cases cover the right things, and the second one (re-escape inside back) guards against moving the clear after vc_acquire.

CI: test_vcore_escape.py and test_invalidate_vcore.py pass on Ubuntu and Windows. The two red jobs aren't from this PR: Windows part 1/2 fails in test_native_astc_texture_count.py (scanf deprecated under -Werror with clang on Windows), and appimage timed out fetching system packages.

Looks good to me.

Thanks for following up on the #2302 nit. Clearing `vc_escaped` in `hos_vcore_back` before `vc_acquire` is the right spot: `vc_acquire` sets the flag again if the thread escapes again, so after any `back` the flag is up only while the thread really is running without a core. Your reason for not putting it in `vc_take` holds too, since `vc_grant` runs on the releasing thread. What I checked: - Races: `vc_escaped` is still written only by its own thread (`vc_acquire` on `t`, `hos_vcore_away`, now `hos_vcore_back`). `vc_grant`/`vc_release` never touch it, so the extra store at each `back` adds no cross-thread access. - Nesting stays balanced: the flag can only be up while `vc_held == 0`, so the outermost `away` still answers 1 and clears it, inner ones answer 0, and only the outer `back` takes a core. The paired callers (`hos_thread_wait`, `hos_cv_sleep`, the other `if (away) hos_vcore_back()` sites, the #2301/#2322 users) behave exactly as before. The two unpaired `back`s at thread start see a zeroed flag, so nothing changes there today, as the description says. - `vc_yield` goes through `vc_release`/`vc_acquire` directly rather than `back`, so a re-escape there still leaves the flag up until the next block, as intended. - The two new test cases cover the right things, and the second one (re-escape inside `back`) guards against moving the clear after `vc_acquire`. CI: `test_vcore_escape.py` and `test_invalidate_vcore.py` pass on Ubuntu and Windows. The two red jobs aren't from this PR: Windows part 1/2 fails in `test_native_astc_texture_count.py` (`scanf` deprecated under `-Werror` with clang on Windows), and appimage timed out fetching system packages. Looks good to me.

mooki000 commented

2026-11-11 18:54:58 +00:00

(Migrated from github.com)

Thanks for reviewing the flag ownership and re-escape cases. The unrelated Windows ASTC fixture failure is tracked in #2397; no further scheduler change is needed for that CI failure.