Skip to content

serve: HRX takes the GTT save-area placement by default (engine#315, engine#715) - #743

Merged
bong-water-water-bong merged 8 commits into
mainfrom
1bit/hrx-svm-default
Nov 1, 2026
Merged

bong-water-water-bong merged 8 commits into
mainfrom
1bit/hrx-svm-default

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

PR body — 1bit/hrx-svm-default (engine, base main)

Title: serve: HRX takes the GTT save-area placement by default (engine#315, the 2026-10-31 A/B)

Branch: 1bit/hrx-svm-default · head 36fdd48 (2 commits; the second corrects the mode named in the first) ·
base main (8a3bcfd) · 5 files, +121/−61

Why

engine#315 candidate 1 (app/hrx_cwsr.h) put each HRX queue's CWSR context save area in GTT
(HSA_USE_SVM=0 HSA_USERPTR_FOR_PAGED_MEM=0 HRX_ALLOW_NO_SVM=1) only in auto, i.e. only when
/sys/module/amdgpu/parameters/cwsr_enable read 1. On strixhalo after the kernel rebuild
(7.3.0-rc4-1bit-perfopt+) the save area's pageable placement page-faults with CWSR off, which is the
shipped configuration, so auto added nothing and every HRX process died on its first graph replay — including the
c18 release tarball, i.e. it was not a build under test.

The measurement (same binary, same file, same argv, one gpu-hold claim, 32 tokens per arm)

llama-server from 1bit/hrx-vulkan-patched + #161, Qwen3-0.6B-Q4_K_M (sha256 ac2d9771…), --device HRX0 -ngl 99 -c 4096 -np 1, dmesg [gfxhub] page fault count per arm:

HSA runtime (sha256) SVM mode result new gfxhub faults
release-bundled libhsa f6ef689d… HSA_USE_SVM unset, HSA_USERPTR_FOR_PAGED_MEM=0 no HRX0 at all: error while handling argument "--device": invalid device: HRX0 0 (nothing ran)
release-bundled libhsa f6ef689d… the three vars no HRX0 at all, same error 0 (nothing ran)
TheRock nightly libhsa 21a66824… (10.3.0a20261031) HSA_USE_SVM unset, HSA_USERPTR_FOR_PAGED_MEM=0 server up, first completion HTTP 500, 31 fault lines +1
TheRock nightly libhsa 21a66824… the three vars HTTP 200, 260.1 tok/s, 18 prompt / 32 predicted 0

The failure, for the record: wait for HRX graph replay commands failed: …/system_event.c:122: ABORTED; AMDGPU memory access fault at device address 0x… (reason mask 0x0) in the server log, and in dmesg [gfxhub] page fault (src_id:0 ring:64 …), Faulty UTCL2 client ID: CPF (0x4), PERMISSION_FAULTS 0x8, WALKER_ERROR 0x0, MAPPING_ERROR 0x0.

Corroboration from the same session: plain HIP through the same runtime is clean (hipMalloc → H2D → memset → kernel → D2H → pinned H2D, HIP_PATH_OK, 0 faults), rocminfo reports XNACK enabled: NO and Coherent Host Access: FALSE
for gfx1151, and test-backend-ops -o TOP_K -b HRX0 is 484/484 on Backend HRX0, 0 fault lines with the
placement in place. cwsr_enable was 0 throughout — which is why auto can no longer key off it.

Which variable is actually responsible (added in the second commit)

A four-corner table on the same 0.6B file (same binary, cache limit 128) shows the fault needs the two knobs
together — libhsakmt takes the SVM save-area path only when pageable userptr is also disabled:

HSA_USE_SVM HSA_USERPTR_FOR_PAGED_MEM result new faults
unset (1) 0 first completion fails +1
unset (1) unset (1) — libhsakmt's real default ok 0
0 unset (1) ok 0
0 0 (this placement) ok 0

The faulting mode is SVM with pageable userptr disabled, which is what the fleet's shell harness had been setting
(HSA_USERPTR_FOR_PAGED_MEM=0, inherited from 612-hrx.sh) — so the earlier arms in this work all ran in it, and
"the box's HRX path is down" was never established. This placement is therefore one of three working modes, not
the only one. It stays the default serve picks because it is the mode the #315 soak covered and a box's true
defaults are not known to be clean; the true-defaults mode was clean on 0.6B and its 76 GiB measurement was still
pending when this landed. docs/hrx.md carries both tables and says so.

What changes

  1. app/hrx_cwsr.h — auto (the default) always places the trio, whatever cwsr_enable says; HRX_SVM=1 is the
    env opt-out and --hrx-cwsr-placement=off the flag opt-out; a value the user set still wins per variable; a build
    whose hrx-system has no HRX_ALLOW_NO_SVM gate still adds nothing (and on still refuses at start). The log line
    says which mode decided and what cwsr_enable was.
  2. app/serve.cpp — the --hrx-cwsr-placement help text describes the new default.
  3. scripts/hw-gate.sh — exports the trio for its own direct test-backend-ops / llama-bench / llama-server
    runs, which bypass serve; the caller's environment still wins.
  4. tests/hrx_cwsr_route.sh — the auto cases now assert the trio (gate present) or the "gate missing" line (no
    gate), plus the HRX_SVM=1 opt-out and the user-value-wins case; the cwsr_enable-dependent branch is gone.
  5. docs/hrx.md — the section states the new default and carries the A/B table above.

Verification

  • app/hrx_cwsr.h logic: compiled unit test, 11/11 checks pass — auto (gate, cwsr_enable 0 and 1) → all three;
    auto + HRX_SVM=1 → none + log; auto without gate → none + log; off → none; on → all three, and refuses
    without the gate; unknown mode string rejected.
  • The three help-string literals were compiled verbatim and render as intended.
  • bash -n scripts/hw-gate.sh, bash -n tests/hrx_cwsr_route.sh clean.
  • Not run here: the full engine build and tests/hrx_cwsr_route.sh end to end (this checkout's submodules are not
    initialised, so the 1bit binary was not built). CI or a lane with a configured tree should run the route test.

Risk / rollback

A box whose ROCr does not need the GTT placement changes behaviour by default: --hrx-cwsr-placement=off or
HRX_SVM=1 restores the old one without a rebuild. Nothing here touches the CWSR-off kernel/module configuration
itself (the shipped amdgpu.cwsr_enable=0 stays as it is).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

pi-prs: ready for review. Rebased onto current main (33775af) after opening, so the branch is now at 599fb9f (was 36fdd48); the only rebase conflict was scripts/hw-gate.sh, where main's comment reword was kept and my export block re-added — bash -n clean.

The two things engine#715 asks for are both here:

  • scripts/hw-gate.sh exports HSA_USE_SVM=0 HSA_USERPTR_FOR_PAGED_MEM=0 HRX_ALLOW_NO_SVM=1 for its own test-backend-ops / llama-bench / llama-server runs, which bypass serve; a caller's value still wins.
  • tests/hrx_cwsr_route.sh now asserts the new default: auto places the trio with the gate (whatever cwsr_enable says), auto + HRX_SVM=1 adds nothing and logs it, auto without the gate adds nothing and logs the missing gate, off adds nothing, on still refuses without the gate, and a user-set value wins per variable. The old cwsr_enable-dependent branch is gone.

One thing a reviewer should see before approving: the second commit narrows the claim. The 0.6B four-corner table shows the gfxhub fault needs HSA_USE_SVM on together with HSA_USERPTR_FOR_PAGED_MEM=0 (the combination the fleet's shell harness set), and libhsakmt's true defaults were clean:

HSA_USE_SVM HSA_USERPTR_FOR_PAGED_MEM result new faults
unset (1) 0 first completion fails +1
unset (1) unset (1) ok 0
0 unset (1) ok 0
0 0 (this PR) ok 0

So the GTT placement is one of three working modes, kept as serve's default because it is the mode the #315 soak covered and a box's true defaults are not known-clean. The 76 GiB measurement of the true-defaults mode was still pending when this landed (device contention); it does not change what this PR does, only how the reason is worded, and docs/hrx.md now says exactly that.

Verification actually run here: a compiled unit test of decide_cwsr_placement (11/11 — every mode above), the three edited help-string literals compiled verbatim, bash -n on both scripts. Not run: a full engine build and tests/hrx_cwsr_route.sh end to end, because the worktree has no submodule contents; CI or a configured tree should run that.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Preparing review...

…the 2026-10-31 A/B)

engine#315 candidate 1 used to place each HRX queue's CWSR save area in GTT only when amdgpu ran
with cwsr_enable=1 (--hrx-cwsr-placement=auto). On kernel 7.3.0-rc4-1bit-perfopt+ the pageable
placement page-faults with CWSR *off*, so auto now places unconditionally and the opt-out is
--hrx-cwsr-placement=off (or HRX_SVM=1 in the environment).

Same binary, same Qwen3-0.6B file, same argv, one gpu-hold claim, 32 tokens per arm, new gfxhub
page faults counted in dmesg per arm:

  release-bundled libhsa f6ef689d, default            no HRX0 at all (--device: invalid device)
  release-bundled libhsa f6ef689d, the three vars     no HRX0 at all (same)
  TheRock nightly libhsa 21a66824, default            HTTP 500, 31 fault lines      +1 fault
  TheRock nightly libhsa 21a66824, the three vars     HTTP 200, 260.1 tok/s          0 faults

The release-bundled libhsa cannot register HRX0 on a dev build; the nightly libhsa the engine picks
faults on the first graph replay of every process (the c18 release tarball included) unless the save
area goes to GTT. Plain HIP through the same runtime is clean, rocminfo reports XNACK NO and Coherent
Host Access FALSE for gfx1151, and test-backend-ops -o TOP_K -b HRX0 passes 484/484 with 0 faults
once the placement is in.

serve adds the three variables to every HRX backend (chat, RAG, Laya) before ROCr initialises, and
still lets a value the user set win. scripts/hw-gate.sh exports them for its own direct
test-backend-ops / llama-bench / llama-server runs, which bypass serve. docs/hrx.md carries the table.
…r disabled)

The 0.6B four-corner table changes what this change is for. The faulting mode is not "SVM": it is SVM
*with* HSA_USERPTR_FOR_PAGED_MEM=0, which is what the fleet's shell harness had been setting (inherited
from 612-hrx.sh) and what every earlier arm in this work therefore ran with. With both knobs at their
libhsakmt defaults the same file ran clean, and so did SVM off with userptr on.

  HSA_USE_SVM=1, userptr=0            first completion fails         +1 fault
  HSA_USE_SVM=1, userptr=1 (default)  ok                            0
  HSA_USE_SVM=0, userptr=1            ok                            0
  HSA_USE_SVM=0, userptr=0 (this PR)  ok                            0

So the GTT placement is one of three working modes, not the only one. It stays the default that serve
chooses, because it is the mode the #315 soak covered and a box's defaults are not known to be clean;
the true-defaults mode was clean on 0.6B and its 76 GiB measurement was still pending. docs/hrx.md now
carries both tables and says so, and the header comment no longer claims plain "pageable-SVM" faults.
`hrx_cwsr` (the C++ unit test over app/hrx_cwsr.h, ctest #27) still asserted the old contract — auto places
the trio only when cwsr_enable reads 1 and the user set none of the variables — so the build job failed with
nine FAILs while `hrx_cwsr_route` (the shell test) passed. This is the second half of the test update engine#715
asks for; the first was the route test in the previous commit.

What it asserts now:
- auto places all three whatever /sys/.../cwsr_enable says (1, 0, unreadable, junk, missing), and the log says
  "placed in GTT by default" plus both opt-outs;
- HRX_SVM=1 is the environment opt-out (with CWSR on and off) and HRX_SVM=0 is not;
- a value the user set still wins per variable — all three are offered, the child keeps the user's value, and the
  log names it ("kept from the environment: ..."), including the empty-value case;
- an hrx-system without the HRX_ALLOW_NO_SVM gate adds nothing, whatever cwsr_enable says.

Built and run locally: g++ -Iapp tests/hrx_cwsr_test.cpp -> "hrx_cwsr: all checks passed" (29 assertions).
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

pi-prs: pushed 28633e6 (3 commits, rebased onto current main 3722aa7, MERGEABLE) — the build failure you saw was mine and is fixed.

Why build failed: hrx_cwsr (ctest #27, the C++ unit test over app/hrx_cwsr.h) still asserted the old contract — auto places the trio only when cwsr_enable reads 1 and the user set none — so it produced nine FAILs; hrx_cwsr_route (the shell test, #18) passed. That C++ test is the second half of the "hrx_cwsr_test update" engine#715 asks for; I had only done the shell one. The new commit carries it and asserts:

  • auto places all three whatever cwsr_enable says (1, 0, unreadable, junk, missing), and the log says placed in GTT by default plus both opt-outs;
  • HRX_SVM=1 is the opt-out (with CWSR on and off), HRX_SVM=0 is not;
  • a user-set value still wins per variable — all three offered, child keeps the user's, log names it (kept from the environment: …), including the empty-value case;
  • an hrx-system without the HRX_ALLOW_NO_SVM gate adds nothing, whatever cwsr_enable says.

Verified locally rather than by pushing and hoping: g++ -std=gnu++20 -Wall -Wextra -Iapp tests/hrx_cwsr_test.cpp → "hrx_cwsr: all checks passed" (29 assertions), the same for my scratch test of the header (11/11), bash -n clean on hw-gate.sh and hrx_cwsr_route.sh, and the diff against origin/main is exactly the six intended files (+154/−83).

Note the pr_agent check also failed on the previous head; if it fails again on 28633e6 it is worth reading its report rather than the exit code, since the only change since is the test file. Full engine build + ctest -R hrx_cwsr still cannot run in this worktree (no submodule contents) — CI is the check for that, and it should be green now.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 1c7a5778, rebased after the merges ahead of it. The author patch is byte-identical to the head I reviewed (patch-id from the merge base unchanged before and after), so only main arrived and my review stands without re-reading.

Build state at post time: pending on this head — the merge is gated on it going green, and since the tree is patch-identical to the head whose build I did see green, nothing in this PR is what could turn it red. Nothing new to verify; everything in my earlier verdict on this PR, including what I recorded as not-verified-by-me, applies here unchanged.

tprs-ready stands.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 1c7a577

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 1c7a5778 — the live head. The author patch is byte-identical to the content I reviewed (patch-id from the merge base unchanged across the rebase), so only main arrived and the verdict carries rather than being re-derived.

The auto CWSR placement change: no more waiting on cwsr_enable, because the pageable-SVM save area page-faults on the first HRX graph replay even with CWSR off; opt-outs are --hrx-cwsr-placement=off / HRX_SVM=1. Its device-free gate passed (hrx_cwsr: all checks passed) and hw-gate.sh parses.

Nothing new to verify; everything in my earlier verdict on this PR, including what I recorded as not-verified-by-me, applies here unchanged.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 7f4c543f, and for any head carrying author patch-id d64a5793f6d2 — the content is the one I reviewed; every move since has been a rebase onto main with the author diff unchanged (verified by patch-id from the merge base), so only main arrives and the verdict carries rather than being re-derived.

Worth stating plainly, because heads are moving faster than a verdict can be posted: gh pr review records whatever head is live at post time, so a rebase in between makes the recorded commit id look stale even though the reviewed content is identical. The durable key here is the patch-id above, not the sha.

Nothing new to verify; everything in my earlier verdict on this PR — including what I recorded as not-verified-by-me — applies unchanged.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 7f4c543

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 29ec31e

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 29ec31e7 (author patch-id d64a5793f6d2). Clean rebase — patch-identical. auto CWSR placement no longer waits on cwsr_enable, because the pageable-SVM save area page-faults on the first HRX graph replay even with CWSR off; opt-outs --hrx-cwsr-placement=off / HRX_SVM=1. Its device-free gate passed (hrx_cwsr: all checks passed) and scripts/hw-gate.sh parses. Note: this changes every HRX backend\x27s default environment — boxes that do not need the GTT placement must now opt out.

@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 80d2212)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

315 - Partially compliant

Compliant requirements:

  • Addresses the intermittent HSA memory fault / NaN logits issue on HRX for GLM-4.7-Flash
  • Implements the default GTT save-area placement for HRX backends to avoid page faults
  • Updates documentation to reflect the new default behavior

Non-compliant requirements:

  • The PR does not include specific measurements or benchmarks to validate the fix

Requires further human verification:

  • Verification of the fix on actual hardware with the GLM-4.7-Flash model under varied-length requests
  • Confirmation that the page faults are resolved in the actual runtime environment

161 - Partially compliant

Compliant requirements:

  • Implements routing that prefers resident experts for MoE models
  • Pins llama.cpp-vulkan to the specified version
  • Updates documentation for MoE streaming with resident expert routing

Non-compliant requirements:

  • No specific performance measurements or benchmarks included for the MoE routing improvements

Requires further human verification:

  • Verification of MoE routing behavior in actual deployments
  • Confirmation of performance improvements with the specified R value (0.5)
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

CWSR Placement Logic Change

The change in decide_cwsr_placement function removes the dependency on cwsr_enable for the auto mode, which was a significant behavioral change. This could affect systems where CWSR is actually enabled but the new logic defaults to GTT placement regardless. The logic now assumes that the problematic SVM placement with pageable userptr disabled is the issue regardless of CWSR state, which may not be universally true.

if (mode == CwsrPlacement::Auto) {
    // engine#315 candidate 1 is the default since 2026-10-31: with the TheRock nightly libhsa the
    // pageable-SVM save area page-faults on the first HRX graph replay even with CWSR *off*, which is the
    // shipped configuration here, so `auto` can no longer key off cwsr_enable. The opt-outs are
    // --hrx-cwsr-placement=off and HRX_SVM=1 (a box whose ROCr does not need the GTT placement).
    const char* svm_opt_out = getenv_fn("HRX_SVM");
    if (svm_opt_out && std::string(svm_opt_out) == "1") {
        d.log = "HRX: HRX_SVM=1; CWSR save-area placement left to ROCr (engine#315)";
        return d;
    }
    if (!gate_built) {
        d.log = "HRX: this build's hrx-system has no HRX_ALLOW_NO_SVM gate; CWSR save-area placement "
                "unchanged (engine#315)";
        return d;
    }
    place();
    d.log = std::string("HRX: CWSR save area placed in GTT by default (engine#315; amdgpu cwsr_enable=") +
            (cwsr_enable == 1 ? "1" : cwsr_enable == 0 ? "0" : "unknown") +
            "; --hrx-cwsr-placement=off or HRX_SVM=1 opts out)";
    if (!user_set.empty()) d.log += "; kept from the environment: " + user_set;
    return d;
Documentation Inconsistency

The usage message in serve.cpp still refers to the old behavior where auto only applies when cwsr_enable is 1, but the actual implementation has changed to always apply the GTT placement by default. This inconsistency could confuse users who rely on the documentation.

"                  [--hrx-cwsr-placement=auto|on|off]   engine#315: HRX backends get HSA_USE_SVM=0\n"
"                  HSA_USERPTR_FOR_PAGED_MEM=0 HRX_ALLOW_NO_SVM=1, which puts the CWSR save area in\n"
"                  GTT. auto (the default) always does this; off, or HRX_SVM=1, turns it off (docs/hrx.md)\n"

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 8e12dbd

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 28633e651 (mergeable: true, clean). A serve change I had not reviewed before, so this is a first read plus a run of its own gate.

What it does: auto CWSR placement no longer waits for /sys/module/amdgpu/parameters/cwsr_enable to read 1. Since 2026-10-31 the combination the fleet's shell harness set — SVM placement with pageable userptr disabled — page-faults on the first HRX graph replay even with CWSR off, which is the shipped configuration here, so auto keyed on cwsr_enable added nothing and every HRX process died on its first replay. The default is now candidate 1, with --hrx-cwsr-placement=off and HRX_SVM=1 as opt-outs and an explicit variable still winning.

That reasoning matches what I have seen on this box: #718's hyperloom entrypoint sets exactly those three variables for the same reason (the nightly libhsa), so this is the serve-side counterpart of the same measured fact rather than a separate theory.

Ran its device-free gates:

g++-16 -std=c++17 -O1 -I . tests/hrx_cwsr_test.cpp -o t && ./t
  -> hrx_cwsr: all checks passed   (exit 0)

The test drives a fake cwsr_enable file and a fake environment through app/hrx_cwsr.h and asserts the decision — including the new rows ("placed in GTT by default", the auto log naming the default, and "--hrx-cwsr-placement=off or HRX_SVM=1 opts out"). bash -n scripts/hw-gate.sh clean (it gained 9 lines); hrx_cwsr_route is add_test()-wired with a fake backend, so CI covers the route without a GPU.

Not verified: the A/B itself — that the first HRX graph replay stops page-faulting with the new default on real hardware, and the two tables in docs/hrx.md. That is the lane's measurement and needs the GPU.

One thing to watch: this changes the default environment of every HRX backend, so any box whose ROCr genuinely does not need the GTT placement now has to opt out. The opt-outs exist and are tested; the release notes should mention it.

tprs-ready applied.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 80d2212

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 80d22123 — the final head after the branch was brought up to date with main, and build green. The author patch is unchanged (d64a5793f6d2), so this is a confirmation at the final head rather than a re-derivation.

The substance, from my first review: auto CWSR placement no longer waits for /sys/module/amdgpu/parameters/cwsr_enable to read 1. Since 2026-10-31 the combination the fleet's harness used — SVM placement with pageable userptr disabled — page-faults on the first HRX graph replay even with CWSR off, which is the shipped configuration here, so keying auto on cwsr_enable added nothing and every HRX process died on its first replay. The default is now candidate 1, with --hrx-cwsr-placement=off and HRX_SVM=1 as opt-outs and an explicit variable still winning.

Ran its device-free gate at that content: hrx_cwsr: all checks passed (a fake cwsr_enable file and a fake environment driven through app/hrx_cwsr.h), bash -n scripts/hw-gate.sh clean, and hrx_cwsr_route is add_test()-wired with a fake backend so CI covers the route without a GPU. The test asserts the new rows: "placed in GTT by default", the auto log naming the default, and "--hrx-cwsr-placement=off or HRX_SVM=1 opts out".

Two notes carried forward, neither blocking:

  • this changes every HRX backend's default environment, so a box whose ROCr does not need the GTT placement must now opt out — worth a release note;
  • the A/B itself (that the first replay stops page-faulting on hardware, and the two tables in docs/hrx.md) is the lane's measurement and needs the GPU.

tprs-ready is on it.

@bong-water-water-bong
bong-water-water-bong merged commit d54040f into main Nov 1, 2026
8 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/hrx-svm-default branch November 1, 2026 10:41
bong-water-water-bong pushed a commit that referenced this pull request Nov 2, 2026
…ate opt-out (engine#715)

The trio was already set for serve's own backends (#743) and in scripts/hw-gate.sh's
environment. Two gaps were left, and both cost a GPU reset rather than an error:

  * a child that exec()s with the *inherited* environment never got it. Child builds an
    explicit envp, but run_program and LayaDaemon exec with ours, so any helper started
    through them ran HRX without the trio;
  * nothing stopped a serve that had opted out of the placement from starting anyway.

What changes:

  * app/hrx_cwsr.h: kCwsrRecipe (the pastable line), cwsr_env_present() (is the trio in
    this NAME=VALUE list; the value is not checked, because a caller's value wins
    everywhere in the engine) and apply_cwsr_env_to_self() (put it in *our* environment,
    overwrite=0). Windows gets _putenv_s.
  * app/serve.cpp: apply it to serve's own environment right after option parsing, so
    every descendant inherits it however it is spawned and a future spawn site cannot
    forget it; and refuse an hrx/auto route when the user opted out
    (--hrx-cwsr-placement=off or HRX_SVM=1) and the trio is therefore absent, printing the
    recipe. ONEBIT_HRX_NO_TRIO_OK=1 is the acknowledgement.
  * scripts/hrx-env.sh: one definition for the shell side -- `source ... --check` sets the
    trio and refuses to continue without it -- and scripts/hw-gate.sh now sources it
    instead of carrying its own copy.

The refusal keys on the opt-out, not on "the trio happens to be empty". The first version
keyed on the empty trio and failed nine route tests in CI: those run `--device hrx` against
a stubbed backend, so no HRX ever initialises, and in a build whose hrx-system has no
HRX_ALLOW_NO_SVM gate the resolved trio is legitimately empty. Opt-out is a user decision,
the empty trio in a gateless build is not.

Verified without a device:
  * tests/hrx_cwsr_test.cpp: all checks passed, 12 new assertions over the recipe, the
    presence check (including a name that only starts with the same letters, and a
    caller's non-default values counting as present) and apply-to-self (adds only what is
    absent, a caller's value survives);
  * the block inserted into serve.cpp's main() compiled and run standalone against the real
    header, five ways: hrx with the trio -> one log line, no refusal; a build WITHOUT the
    gate (empty trio, no opt-out) -> not refused, which is the CI case that regressed;
    --hrx-cwsr-placement=off -> refused with the recipe; the same plus
    ONEBIT_HRX_NO_TRIO_OK=1 -> proceeds; --device cpu with off -> never refused;
  * bash -n clean on scripts/hrx-env.sh, scripts/hw-gate.sh and tests/hrx_cwsr_route.sh.

NOT verified here: the full engine build and `ctest -R hrx_cwsr` / tests/hrx_cwsr_route.sh
end to end -- this worktree has no submodule contents, so there is no 1bit binary to run
the route test against. CI is that check.
bong-water-water-bong pushed a commit that referenced this pull request Nov 2, 2026
…ate opt-out (engine#715)

The trio was already set for serve's own backends (#743) and in scripts/hw-gate.sh's
environment. Two gaps were left, and both cost a GPU reset rather than an error:

  * a child that exec()s with the *inherited* environment never got it. Child builds an
    explicit envp, but run_program and LayaDaemon exec with ours, so any helper started
    through them ran HRX without the trio;
  * nothing stopped a serve that had opted out of the placement from starting anyway.

What changes:

  * app/hrx_cwsr.h: kCwsrRecipe (the pastable line), cwsr_env_present() (is the trio in
    this NAME=VALUE list; the value is not checked, because a caller's value wins
    everywhere in the engine) and apply_cwsr_env_to_self() (put it in *our* environment,
    overwrite=0). Windows gets _putenv_s.
  * app/serve.cpp: the trio goes into serve's own environment on an hrx/auto route, so
    every descendant inherits it however it is spawned and a future spawn site cannot
    forget it; and an hrx/auto route is refused when the user opted out
    (--hrx-cwsr-placement=off or HRX_SVM=1) so the trio is absent, with the recipe in the
    message. ONEBIT_HRX_NO_TRIO_OK=1 is the acknowledgement.
  * scripts/hrx-env.sh: one definition for the shell side -- `source ... --check` sets the
    trio and refuses to continue without it -- and scripts/hw-gate.sh now sources it
    instead of carrying its own copy.

Two things the earlier heads got wrong, both caught by CI and both fixed by narrowing what
is printed and when:

  1. The refusal keyed on an empty trio, which also fires in a build whose hrx-system has no
     HRX_ALLOW_NO_SVM gate (nine route tests, 41366068194). It now keys on the opt-out.
  2. hrx_cwsr_env()'s function-local static printed the decision line on first call, so any
     caller printed it -- including `--device cpu`, which made the memory refusal two lines
     and failed mem_admit_route (tests/mem_admit_route.sh counts lines). The decision is now
     computed silently and announced once at the first HRX backend that uses it
     (announce_cwsr_env), the self-environment step is silent (the decision line already
     says it), and the refusal message is a single line.

Verified without a device:
  * tests/hrx_cwsr_test.cpp: all checks passed, 12 assertions over the recipe, the presence
    check (including a name that only starts with the same letters, and a caller's
    non-default values counting as present) and apply-to-self (adds only what is absent, a
    caller's value survives);
  * the main() block compiled and run standalone against the real header behind main.cpp's
    own one-line handler, and the line counts are the ones the tests assert:
    --device cpu -> 0 extra lines (the mem_admit_route case), a gateless build on auto with
    no opt-out -> 0, an hrx route -> 0 beyond the announced decision, and an explicit
    --hrx-cwsr-placement=off -> exactly 1, the single-line refusal with the recipe inline;
  * bash -n clean on scripts/hrx-env.sh, scripts/hw-gate.sh, tests/hrx_cwsr_route.sh.

NOT verified here: the full engine build and `ctest` end to end -- this worktree has no
submodule contents, so there is no 1bit binary to run tests/mem_admit_route.sh against.
CI is that check; the line-count invariants it asserts were reproduced against the same
handler instead.
bong-water-water-bong added a commit that referenced this pull request Nov 2, 2026
…without it (engine#715) (#794)

* serve: the save-area trio for every HRX process, and refuse a deliberate opt-out (engine#715)

The trio was already set for serve's own backends (#743) and in scripts/hw-gate.sh's
environment. Two gaps were left, and both cost a GPU reset rather than an error:

  * a child that exec()s with the *inherited* environment never got it. Child builds an
    explicit envp, but run_program and LayaDaemon exec with ours, so any helper started
    through them ran HRX without the trio;
  * nothing stopped a serve that had opted out of the placement from starting anyway.

What changes:

  * app/hrx_cwsr.h: kCwsrRecipe (the pastable line), cwsr_env_present() (is the trio in
    this NAME=VALUE list; the value is not checked, because a caller's value wins
    everywhere in the engine) and apply_cwsr_env_to_self() (put it in *our* environment,
    overwrite=0). Windows gets _putenv_s.
  * app/serve.cpp: apply it to serve's own environment right after option parsing, so
    every descendant inherits it however it is spawned and a future spawn site cannot
    forget it; and refuse an hrx/auto route when the user opted out
    (--hrx-cwsr-placement=off or HRX_SVM=1) and the trio is therefore absent, printing the
    recipe. ONEBIT_HRX_NO_TRIO_OK=1 is the acknowledgement.
  * scripts/hrx-env.sh: one definition for the shell side -- `source ... --check` sets the
    trio and refuses to continue without it -- and scripts/hw-gate.sh now sources it
    instead of carrying its own copy.

The refusal keys on the opt-out, not on "the trio happens to be empty". The first version
keyed on the empty trio and failed nine route tests in CI: those run `--device hrx` against
a stubbed backend, so no HRX ever initialises, and in a build whose hrx-system has no
HRX_ALLOW_NO_SVM gate the resolved trio is legitimately empty. Opt-out is a user decision,
the empty trio in a gateless build is not.

Verified without a device:
  * tests/hrx_cwsr_test.cpp: all checks passed, 12 new assertions over the recipe, the
    presence check (including a name that only starts with the same letters, and a
    caller's non-default values counting as present) and apply-to-self (adds only what is
    absent, a caller's value survives);
  * the block inserted into serve.cpp's main() compiled and run standalone against the real
    header, five ways: hrx with the trio -> one log line, no refusal; a build WITHOUT the
    gate (empty trio, no opt-out) -> not refused, which is the CI case that regressed;
    --hrx-cwsr-placement=off -> refused with the recipe; the same plus
    ONEBIT_HRX_NO_TRIO_OK=1 -> proceeds; --device cpu with off -> never refused;
  * bash -n clean on scripts/hrx-env.sh, scripts/hw-gate.sh and tests/hrx_cwsr_route.sh.

NOT verified here: the full engine build and `ctest -R hrx_cwsr` / tests/hrx_cwsr_route.sh
end to end -- this worktree has no submodule contents, so there is no 1bit binary to run
the route test against. CI is that check.

* serve: the save-area trio for every HRX process, and refuse a deliberate opt-out (engine#715)

The trio was already set for serve's own backends (#743) and in scripts/hw-gate.sh's
environment. Two gaps were left, and both cost a GPU reset rather than an error:

  * a child that exec()s with the *inherited* environment never got it. Child builds an
    explicit envp, but run_program and LayaDaemon exec with ours, so any helper started
    through them ran HRX without the trio;
  * nothing stopped a serve that had opted out of the placement from starting anyway.

What changes:

  * app/hrx_cwsr.h: kCwsrRecipe (the pastable line), cwsr_env_present() (is the trio in
    this NAME=VALUE list; the value is not checked, because a caller's value wins
    everywhere in the engine) and apply_cwsr_env_to_self() (put it in *our* environment,
    overwrite=0). Windows gets _putenv_s.
  * app/serve.cpp: the trio goes into serve's own environment on an hrx/auto route, so
    every descendant inherits it however it is spawned and a future spawn site cannot
    forget it; and an hrx/auto route is refused when the user opted out
    (--hrx-cwsr-placement=off or HRX_SVM=1) so the trio is absent, with the recipe in the
    message. ONEBIT_HRX_NO_TRIO_OK=1 is the acknowledgement.
  * scripts/hrx-env.sh: one definition for the shell side -- `source ... --check` sets the
    trio and refuses to continue without it -- and scripts/hw-gate.sh now sources it
    instead of carrying its own copy.

Two things the earlier heads got wrong, both caught by CI and both fixed by narrowing what
is printed and when:

  1. The refusal keyed on an empty trio, which also fires in a build whose hrx-system has no
     HRX_ALLOW_NO_SVM gate (nine route tests, 41366068194). It now keys on the opt-out.
  2. hrx_cwsr_env()'s function-local static printed the decision line on first call, so any
     caller printed it -- including `--device cpu`, which made the memory refusal two lines
     and failed mem_admit_route (tests/mem_admit_route.sh counts lines). The decision is now
     computed silently and announced once at the first HRX backend that uses it
     (announce_cwsr_env), the self-environment step is silent (the decision line already
     says it), and the refusal message is a single line.

Verified without a device:
  * tests/hrx_cwsr_test.cpp: all checks passed, 12 assertions over the recipe, the presence
    check (including a name that only starts with the same letters, and a caller's
    non-default values counting as present) and apply-to-self (adds only what is absent, a
    caller's value survives);
  * the main() block compiled and run standalone against the real header behind main.cpp's
    own one-line handler, and the line counts are the ones the tests assert:
    --device cpu -> 0 extra lines (the mem_admit_route case), a gateless build on auto with
    no opt-out -> 0, an hrx route -> 0 beyond the announced decision, and an explicit
    --hrx-cwsr-placement=off -> exactly 1, the single-line refusal with the recipe inline;
  * bash -n clean on scripts/hrx-env.sh, scripts/hw-gate.sh, tests/hrx_cwsr_route.sh.

NOT verified here: the full engine build and `ctest` end to end -- this worktree has no
submodule contents, so there is no 1bit binary to run tests/mem_admit_route.sh against.
CI is that check; the line-count invariants it asserts were reproduced against the same
handler instead.

---------

Co-authored-by: pi-attn <pi-attn@1bit.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 4/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant