feat(csrc): AMD ROCm logical-slicing shim, and a verification suite that can fail - #86
feat(csrc): AMD ROCm logical-slicing shim, and a verification suite that can fail#86thxCode wants to merge 25 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for AMD ROCm logical-slicing verification, introducing the libvrocm.so shim, monitor and mask check tools, along with seven comprehensive test cases (amd-case-1.sh through amd-case-7.sh). It also refactors existing Ascend and THead test cases to use a centralized verdict helper (xb_verdict) to ensure robust failure detection. The review feedback highlights several improvement opportunities, including handling potential unbound variables under set -u in amd-case-5.sh, skipping leading whitespace when parsing negative quotas in vrocm_quota.c, adding defensive NULL pointer checks in hip_mem.c to prevent segmentation faults, optimizing the bare spin lock in vrocm_ledger.c with a CPU pause hint, and setting LC_ALL=C for deterministic sorting in amd-case-1.sh.
There was a problem hiding this comment.
Pull request overview
This PR introduces the AMD ROCm logical-slicing shim (csrc/amd/rocm-slicing-shim) to enforce per-card VRAM quotas via HIP symbol interposition, along with companion tools/tests and extensions to the gpustack-operator-xbuild-and-verify skill to build and validate the shim across environments (including PTY-only SSH targets) and to harden the case verdict logic.
Changes:
- Add
libvrocm.soHIP interposers (memory allocation families + reported-capacity queries) backed by a shared, lock-coordinated usage ledger format. - Add shim-adjacent tooling and test artifacts (
rocm-monitor,hip_props_probe,ledger_lifecycle) to validate ledger behavior and query/binding surfaces. - Extend the verification harness/scripts with robust verdict parsing (
xb_fails/xb_verdict), AMD ROCm preflight checks, and AMD xbuild support (including a PTY-only target mode).
Reviewed changes
Copilot reviewed 53 out of 54 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| csrc/amd/rocm-slicing-shim/tools/rocm_monitor.c | Adds a lock-free reader for the shared usage ledger, intended for operators/scrapers. |
| csrc/amd/rocm-slicing-shim/testing/ledger_lifecycle.c | Adds a two-process lifecycle test driver for stranded-charge reclaim behavior. |
| csrc/amd/rocm-slicing-shim/testing/hip_props_probe.c | Adds a probe to report which HIP symbols bind and what capacity is reported via each path. |
| csrc/amd/rocm-slicing-shim/README.md | Documents design goals, boundaries, build modes, and operational usage for the shim + tools. |
| csrc/amd/rocm-slicing-shim/hip/hip_vmm.c | Interposes VMM allocation/free entry points (hipMemCreate/hipMemRelease) for quota enforcement. |
| csrc/amd/rocm-slicing-shim/hip/hip_table.h | Declares shared per-entry counters/registration macros for verification and diagnostics. |
| csrc/amd/rocm-slicing-shim/hip/hip_table.c | Implements entry tracking and constructor/destructor hooks for quota validation + counter dumps. |
| csrc/amd/rocm-slicing-shim/hip/hip_resolve.h | Defines symbol resolution interface/macros used by all HIP wrappers. |
| csrc/amd/rocm-slicing-shim/hip/hip_resolve.c | Implements resolver fallback strategy (RTLD_NEXT then RTLD_NOLOAD by SONAME) and caller attribution. |
| csrc/amd/rocm-slicing-shim/hip/hip_query.c | Interposes reported-capacity entry points to report quota instead of physical VRAM. |
| csrc/amd/rocm-slicing-shim/hip/hip_pool.c | Interposes stream-ordered/pool allocation paths to close non-hipMalloc* quota bypasses. |
| csrc/amd/rocm-slicing-shim/device/vrocm_hwid.h | Provides shared device-side HW-ID decoding for occupancy-based CU/WGP mask verification. |
| csrc/amd/rocm-slicing-shim/common/vrocm.h | Introduces common vocabulary and fixed bounds used across the shim. |
| csrc/amd/rocm-slicing-shim/common/vrocm_quota.h | Declares quota/env parsing and validation APIs for “fail closed” enforcement. |
| csrc/amd/rocm-slicing-shim/common/vrocm_quota.c | Implements quota/env parsing + validation and latches usability at load time. |
| csrc/amd/rocm-slicing-shim/common/vrocm_log.h | Defines the shim’s single log channel and level contract used by cases. |
| csrc/amd/rocm-slicing-shim/common/vrocm_log.c | Implements latched log-level parsing with safe fallback behavior. |
| csrc/amd/rocm-slicing-shim/common/vrocm_ledger.h | Defines the shared MAP_SHARED ledger/lock layout and admission/release contract. |
| .claude/skills/gpustack-operator-xbuild-and-verify/scripts/preflight.sh | Adds AMD ROCm target preflight checks and enforces consistent verdict parsing across cases. |
| .claude/skills/gpustack-operator-xbuild-and-verify/scripts/lib.sh | Adds PTY-only target mode and centralizes verdict parsing helpers used by all cases. |
| .claude/skills/gpustack-operator-xbuild-and-verify/scripts/build.sh | Adds xbuild-amd-rocm support with containerized and in-place ROCm build paths. |
| .claude/skills/gpustack-operator-xbuild-and-verify/references/troubleshooting.md | Documents AMD ROCm failure modes and debugging workflow for the shim/tools. |
| .claude/skills/gpustack-operator-xbuild-and-verify/references/amd-cumask-conformance.md | Adds measured CU-mask conformance tables and regeneration instructions. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-1.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-2.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-3.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-4.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-5.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-6.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/thead-case-7.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/nvidia-case-1.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/nvidia-case-2.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/nvidia-case-3.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/ascend-case-1.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/ascend-case-2.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/ascend-case-3.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/ascend-case-4.sh | Switches case verdict to centralized xb_verdict/xb_fails. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-2.sh | Adds AMD injection + reported-capacity surface verification with control interposers. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-3.sh | Adds AMD allocation-path completeness case across allocation families/entry points. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-4.sh | Adds AMD CU-mask conformance validation (positive + fail-open negative rows). |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-5.sh | Adds compute-quota semantics validation using occupancy + throughput under barriers. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-6.sh | Adds common unit suite relay + multi-process quota + multi-card per-card keying checks. |
| .claude/skills/gpustack-operator-xbuild-and-verify/cases/amd-case-7.sh | Adds stranded-charge reclaim case + cross-version reach verification (same shim, two ROCm versions). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
05fc3b9 to
867849f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
csrc/amd/rocm-slicing-shim/hip/hip_resolve.c:7
hip_resolve.ccallsfprintf(stderr, ...)but does not include<stdio.h>, which relies on an implicit declaration and can break builds under stricter compiler settings (or produce incorrect prototypes).
AMD is the last of the nine supported manufacturers without logical slicing. This spec ships the library that closes it -- csrc/amd/ rocm-slicing-shim, building libvrocm.so -- plus the compute-quota contract the operator side must satisfy and the verification surface that keeps proving both. The detector, allocator, pack/ stage and the Go mask derivation are deliberate Non-Goals, handed off with stated reasons. Everything in it was measured on real hardware, on one card of each compute architecture: a Radeon RX 7800 XT (gfx1101 / RDNA3, 1 XCC) and an Instinct MI300X (gfx942 / CDNA3, 8 XCC). Eight PoC gates, all PASS. The findings that shape the design: - A plain LD_PRELOAD is sufficient; LD_AUDIT was built, measured and rejected. It needs a two-step resolve -- RTLD_NEXT, then dlopen(RTLD_NOLOAD) -- because a dlopen'ing framework puts the real symbol out of RTLD_NEXT's reach, and it must never fabricate a status, since the natural placeholder 1 is hipErrorInvalidValue. - One build serves every ROCm version: the library links no ROCm object. A 7.2.4-built shim enforced a 4 GiB cap end to end inside PyTorch 2.9.1+rocm6.4, interposing the framework's own bundled 6.4 runtime. - A hipMalloc-level ledger is not a memory quota. The pool family, plus hipMallocManaged, hipExtMallocWithFlags and hipMallocPitch, are each a measured open door. - Reported capacity has three entry points and the obvious one is never called: only hipGetDevicePropertiesR0600 binds under ROCm 6+. - The CU mask derivation branches on NUM_XCC and the branches share no arithmetic. RDNA allocates WGP pairs aligned to the shader-engine count; CDNA has no pairing but interleaves bits across XCCs, making the atom NUM_XCC CUs and full XCC coverage mandatory. A mask that misses an XCC leaves it unmasked -- 0:0 was measured occupying 267 of 304 CUs while reporting a believable 3.7 % of throughput, so the self-check probe must judge by occupancy and not by throughput. - A CU mask carries no memory-bandwidth isolation: half the CUs reach 97.7 % of the card's bandwidth, so .sliced is a compute ceiling and must never be documented as a QoS. Status: Planned, with an eleven-task DAG. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add csrc/amd/rocm-slicing-shim with build.sh and the whole of common/ - build.sh declares every artifact the tree will carry, including the ones whose sources are not written yet, so the five tasks that build into it next never contend on this file; lib/tool/test fail until they land, and unit/list/check work today - common/ names no hip*/hsa* type and calls no pthread_*/sem_*, which is what makes it testable with no ROCm and no device, and what keeps the product's ceiling at GLIBC_2.4 - put the whole admission in vrocm_ledger_admit(): the caller passes its real allocation as a callback, so check-allocate-charge provably happens under ONE acquisition of the card's lock and the sequence is unit-testable rather than something a reviewer has to trust the caller to get right - reserve the tracking slot BEFORE calling the runtime, so a charge that cannot be recorded is refused with no successful allocation to undo - re-read the quota on every admission rather than freezing the region creator's figure, and sweep dead processes' charges before refusing - add 36 named unit cases, plus `build.sh unit mutants`, which re-runs them against four builds each broken in exactly the way one ledger property forbids; every mutant must make its named row FAIL Verified in a plain ubuntu:22.04 container with no ROCm and no /dev/kfd: `build.sh unit` silent and exit 0, 36/36 cases pass, `build.sh list` non-empty for all eight declared artifacts, and the mutation run silent. `make test` green. Task 1 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…nnot be split The plan cut hip/ into a resolver-and-query task and an allocating-families task, then asked the first one to verify with `build.sh lib` and `build.sh check`. Neither can pass until BOTH land: libvrocm.so does not link until every one of its translation units exists. That left the first task unable to prove the thing it was for. Its three .symver pins exist precisely so `build.sh check`'s GLIBC_2.4 assertion holds, and that assertion could not run until the task after it. The split also bought nothing. The second task was blocked by the first, nothing else depended on the first alone, and the stated reason for splitting -- that one family editing another's table is a write conflict wearing a dependency's clothes -- only applies to tasks that run at the same time. These never could. - merge T2 and T3 into one T2 that owns SHIM/hip/**, and renumber the rest contiguously; the DAG is now ten tasks with the same five-way fan-out after T1 - keep each family in its own translation unit, for blast radius rather than for write contention, and say which reason it is now - record dladdr as a third .symver pin: it arrives with the caller-origin diagnostic rather than with the resolver, which is what makes it the one to miss - correct T1's note, which already admitted `build.sh lib` needs more than one task, and the checkpoint that named the wrong pair Signed-off-by: thxCode <thxcode0824@gmail.com>
867849f to
6214e73
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
csrc/amd/rocm-slicing-shim/hip/hip_table.c:38
vrocm_entry_hit()updatesentry->tracedwith a plain increment while wrappers can be called concurrently from many threads. This introduces a C data race (undefined behavior). Use an atomic fetch-add fortracedand make the trace-limit decision based on the returned prior value.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated no new comments.
Suppressed comments (2)
csrc/amd/rocm-slicing-shim/tools/rocm_monitor.c:162
mmap(sizeof(*region))assumes the ledger file is at leastsizeof(struct vrocm_region). If the file is truncated but still large enough to pass the header read, later reads fromregion->devices[...]can SIGBUS. Add anst_sizecheck for the full region size before mapping.
csrc/amd/rocm-slicing-shim/hip/hip_table.c:38entry->tracedis incremented and read concurrently from multiple threads without synchronization. In C this is a data race (undefined behavior) and can also cause the TRACE_LIMIT gate to behave unpredictably. Use an atomic fetch/add (relaxed is sufficient) for the limit check and increment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.claude/skills/gpustack-operator-xbuild-and-verify/references/troubleshooting.md:275
- This troubleshooting note says
VROCM_LEDGER_PATH“has no default” and that omitting it makes the shim refuse everything. In the current implementation,vrocm_quota_ledger_path()falls back toVROCM_LEDGER_DEFAULT_PATH("/dev/shm/vrocm-ledger"), so the statement is misleading (the real risk is that the default can be unsafe when /dev/shm is shared).
`LIBVROCM_LOG_LEVEL=2`, because the load marker and the counter dump sit above the default of 1,
which carries denials only; and `VROCM_LEDGER_PATH`, which has **no default** — without it the
constructor reports `VROCM_LEDGER_PATH is unset; nothing can be accounted` once and then refuses
everything, which looks nothing like "quietly did nothing" but is easy to miss on a busy stream.
8c26275 to
fb9a649
Compare
fb9a649 to
b556832
Compare
Copilot's suppressed comments — outcomesThese arrive collapsed in the review body with no thread to reply on, so the disposition is
On #1, and why it matters more than the line it names. Two commits earlier I gave
All three are folded into Verified after: seven AMD cases 149 PASS / 0 FAIL, 44 unit cases, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.claude/skills/gpustack-operator-xbuild-and-verify/scripts/preflight.sh:242
- The preflight guard for “every case decides through xb_verdict” is currently
grep -q 'xb_verdict ', which can be satisfied by a comment mentioningxb_verdict(or other non-invocation text). That would let a case script omit the real verdict call while still passing this safety check.
for c in "${HERE}/../cases"/*.sh; do
[ -f "${c}" ] || continue
seen=$((seen + 1))
grep -q 'xb_verdict ' "${c}" || rogue="${rogue} $(basename "${c}")"
done
… quota hip/ lands whole: the resolver, the interception table, the three reported-capacity entry points, the classic allocating family and the stream-ordered/pool family. libvrocm.so now links, and `build.sh check` passes all four linkage assertions on a glibc 2.35 image. Measured on a gfx1101 card, ROCm 7.2.4: under a 4 GiB quota all three capacity entries report 4.000 GiB against a 15.984 GiB card; under 2 GiB both hipMalloc AND hipMallocFromPoolAsync stop at exactly 2.000 GiB. Four things this turned up, each caught by an assertion rather than by reading the code: - TWO ENTRY POINTS WERE SILENTLY NOT EXPORTED. Under -fvisibility=hidden a definition is exported only if some declaration gave it default visibility, which for these names means only if ROCm's header declares it. The plain hipGetDeviceProperties is macro-mapped away before the header can declare it, and hipGetDevicePropertiesR0000 is not declared at all. Both compiled clean, exported nothing and would have interposed nothing. Every wrapper now carries VROCM_EXPORT explicitly. - THE TWO PROPERTY STRUCTS ARE NOT THE SAME SHAPE: hipDeviceProp_tR0600 is 1472 bytes with totalGlobalMem at 288, hipDeviceProp_tR0000 is 792 with it at 256. Writing the 6.0 field into a caller that passed the 4.2 struct is memory corruption in its frame, not a missed quota. The pre-6.0 entries take the pre-6.0 type and both layouts are pinned with _Static_assert. - ld-linux-x86-64.so.2 WAS IN DT_NEEDED, from __tls_get_addr rather than from the dl* pins -- -ldl changes nothing. -ftls-model=initial-exec is the correct model here rather than a trick: the object is loaded through /etc/ld.so.preload, so it is always in the initial link map and never dlopen()ed. - THE lock-split MUTANT HAD UN-HOOKED ITSELF when the allocation callback's signature changed, and a mutation that stops applying looks exactly like a property that holds. The harness reported it; the pattern is re-hooked and the episode is recorded where the patterns live. Also extends common/: the allocation callback's byte count is now in-out, so a pitched allocation is admitted on the caller's width and charged for the runtime's stride WITHOUT releasing the card's lock in between. A second call would have to retake it, and by then another process could have decided against the stale total. One more named unit case covers it. Task 2 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
… writer rocm-monitor prints the VRAM quota in force and what is charged against it, per card, plus the per-process breakdown. It exists because nothing on an AMD host reports a slice: rocm-smi and amd-smi read sysfs and the DRM nodes rather than HIP, so a container capped at 4 GiB still sees the card's full 15.984 GiB from them. - parse the region from the layout contract alone: the struct definitions and their _Static_asserts come from the header, the mmap is this tool's own and read-only. It links NONE of common/'s ledger code, because that code maps lazily -- it would CREATE a region for anything that merely looked at the container -- and its other entries take the card's lock, which would let a monitor wedge behind a driver allocation that hung - open O_RDONLY with no O_CREAT, and prove it: the tool leaves nothing behind after being pointed at a path that does not exist - refuse rather than misparse. A foreign magic, a layout version this reader does not speak, a file too short to hold a header, or slot counts it was not built for all exit 2; no region is 1; a parsed region is 0. A scraper has to tell those apart - deliberately print no compute figure. Compute is enforced by the platform through a CU mask this library never sees and does not own, so a number here would be the library repeating a value it cannot verify -- rocm-cumask-check answers that question by running a kernel instead Also lets `build.sh list` take either spelling: the artifacts are named with hyphens and the sources with underscores, and somebody asking about `rocm-monitor` is asking about the command they can type. Verified in a plain ubuntu:22.04 container with no ROCm and no device: the figures match the writer exactly, DT_NEEDED is libc.so.6 alone, and no vrocm_ledger/quota/log object is linked in. Task 3 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
T4 said the probe reads HW_ID and compares the physical (XCC, SE, CU) slots its waves ran on. That is right on CDNA and wrong on RDNA, where the register is a different one and a CU-level comparison would be counting something the hardware does not expose. Measured on gfx1101: through HW_ID1 the decode WGP_ID[13:10] / SA_ID[16] / SE_ID[20:18] tracks the mask exactly -- unmasked reports 30 WGPs, 0:0-29 reports 15, 0:0-13 reports 7, 0:0-1 reports 1 -- but SIMD_ID only ever reports 0 or 1, so the two CUs of a WGP cannot be told apart from inside a wave. This costs nothing, because the WGP is exactly RDNA's allocation atom: each architecture ends up compared in the unit it actually allocates in, CUs on CDNA and WGPs on RDNA. Record both register layouts in the task, since getting either wrong produces a probe that counts confidently and means nothing. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add tools/rocm-cumask-check: read the topology through the HSA agent-info API, derive a mask per the architecture's own rules, run a kernel under it, and compare the physical units the waves occupied against what the mask asked for - judge by occupancy and never by throughput; on gfx942 the one-bit mask `0:0` measures a plausible 3.7 % of the card while the container reaches 267 of 304 CUs, because seven XCCs received no mask and the makespan hides them - re-exec with HSA_CU_MASK set, since ROCr reads it at init; a mask already in the environment is verified as it stands, which is how each fail-open construction is reproduced - add device/vrocm_hwid.h so the probe and the soak read occupancy through one decode: HW_ID plus XCC_ID on gfx9 counting CUs, HW_ID1 on gfx10+ counting WGPs, because SIMD_ID there only ever reports 0 or 1 - carry both conformance tables as a checked-in fixture, with every valid row and every fail-open construction measured on real silicon - build the probe with hipcc and an explicit -x hip: hipcc compiles a .c file as C and does not even add the HIP include path when it does - name the offload architectures rather than detecting them, so the artifact is not tied to the build host's card and builds where there is none - state in the spec and the README that computing a mask, injecting it, enforcing it and checking it are four jobs, and that this tree ships only the last - document rocm-monitor and rocm-cumask-check in the README Task 4 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add testing/hip_mem_paths: one allocation family per invocation, so a case can name which path crossed; the array families derive their shape from the device's texture ceilings, because a shape the hardware refuses returns hipErrorInvalidValue and must not be read as the quota's hipErrorOutOfMemory - add testing/hip_props_probe: all four reported-capacity entry points, plus the object each intercepted name actually binds to, which is what turns the R0600 finding into an observation rather than a claim - add testing/cumask_soak: the cross-process file barrier, an ILP-saturating kernel and the per-tenant occupancy readout, with --self-check asserting that two tenants under one mask add up to one - add testing/ledger_lifecycle: hold a charge, take SIGKILL, and prove a later process sweeps it back; it drives the ledger directly and links no ROCm, so it runs on a host with no card - enqueue over several streams before synchronising: at one launch per sync the card read 21636 GFLOP/s solo while two tenants summed to 25015, an aggregate above the solo figure - build cumask_soak through -x hip like the mask probe, and drop the HIP link from ledger_lifecycle, which had nothing to call it for Task 5 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add an xbuild-amd-rocm arm that stages the shim tree and calls its own build.sh, inside a ROCm devel image or in place when the target has no container runtime and already carries ROCm - add XB_MODE=pty for a target whose SSH offers only an interactive shell: the answer returns as one base64 line, since the stream also carries the prompt and a thrice-repeated, width-corrupted echo of what was sent - verify every file typed over that transport by digest, because a line discipline drops what overflows its buffer without saying so - report rocm-smi, the amdgpu-backed render nodes, the amdgpu module and NUM_XCC as preflight rows; the last selects the conformance table the compute cases assert - let run-capable WARN when only an in-place ROCm toolchain is present, so an instance the AMD arm can use no longer fails a required gate - declare the four AMD backend sections in SKILL.md, including a per-script allowed-tools entry for each of the seven case scripts Task 6 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add amd-case-4: every row of the conformance table NUM_XCC selects, and every fail-open construction for that architecture as a negative row asserted on its own occupancy figure, since each one fails by handing the container the whole card - assert a valid ROC_GLOBAL_CU_MASK is still accepted, so the negative row above shows something rather than a value never being taken - require a sub-atom request to be refused on multi-XCC rather than rounded up into a mask that covers only some XCCs - add amd-case-5: the single-tenant ceiling first, since concurrent readings cannot tell a correct partition from a broken one, plus occupancy, fair sharing, saturation, repeatability and the two placements - derive the fairness tolerance from the topology: three tenants over eight XCCs quantise to 3:3:2, measured 1.36-1.52 over four rounds with the slow tenant rotating, which a fixed bound would either fail or loosen enough to pass an unfair single-XCC card - assert an ordering rather than a band on the two placements: measured five times, the complementary pair aggregated to 97-105% four times and to 73% once Task 8 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- wrap hipMalloc3D, and the driver-API halves hipMemAllocPitch, hipArrayCreate, hipArray3DCreate and hipArrayDestroy, which are separate exported symbols from their runtime-API twins rather than aliases of them - add hip/hip_vmm.c for the virtual-memory-management family, where hipMemCreate is the only entry that allocates and is keyed on the handle because no address exists yet; the charge follows the card prop.location.id names rather than the calling thread's context - each of the five was measured satisfying a 512 MiB request under a 64 MiB quota in a run where hipMalloc was correctly refused one, and each now refuses with hipErrorOutOfMemory and a counted denial - find them by subtraction rather than by review: list every allocating name libamdhip64 exports, remove the interposed set, measure the rest - give hip_mem_paths the vmm, drvpitch, drvarray and drvarray3d paths - record hipGraphAddMemAllocNode as a measured, unmitigated boundary: a graph node allocates at launch, not at capture, so the accounting is no longer one call in and one call out Task 7 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…y path - case 1 needs no GPU and no container runtime where ROCm is installed: it recompiles into a scratch directory, re-runs the tree's own four linkage assertions and its four mutants, then judges the STAGED artifacts the later cases load - assert the exported set EXACTLY rather than by membership, which is the half that catches an entry that stopped being compiled in; a dropped entry otherwise looks like one no workload happened to call - case 2 reads all three reported-capacity entry points plus hipDeviceTotalMem, with two control arms: interposing hipMemGetInfo alone leaves totalGlobalMem physical, and interposing the bare hipGetDeviceProperties binds the name while the call still lands on R0600 — the second is the R0600 finding as an observation - case 3 walks thirteen allocation families, one per process with its own ledger, under a quota deliberately below one request; every refusal is checked twice, by status and by the entry's own counter - record the caller-origin baseline: the runtime calls no allocating entry of its own, measured across every run rather than assumed - add the symbol manifest, whose denominator is the point: every exported name that could hand out device memory, minus the interposed set, with each remaining group reasoned about by name Task 7 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…ion reach - case 6 part A relays common/'s 36 unit cases and folds their FAILS= line into this case's count rather than relaying it, since the outer verdict grep matches any line - part B holds a charge from a process that links no ROCm, so the card keeps tens of GiB free and only our accounting can explain the second process's refusal; the denial line and rocm-monitor must then agree - keep the refund row inside one live process: a cross-process form is vacuous, because the sweep reclaims a leaked charge from an exited process the moment the next admission would not otherwise fit - part C is the only place per-card keying is observable — one container, two cards, two quotas, one size between them refused on the smaller naming its OWN figure and served on the larger - case 7 part A pairs the reclaim with its control: while the holder is alive the same request must be refused, or the sweep would be evicting live tenants rather than dead ones - part B preloads the same libvrocm.so, asserted by digest, into a ROCm 7.x and a ROCm 6.x container and requires the same quota outcome in both; the workload is rebuilt per image and its DT_NEEDED is what shows the two arms faced different runtimes Task 9 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- record what each of the seven cases established on the RDNA host, 141 PASS rows and no FAIL, with the figures the rows actually printed - state plainly that the two-architecture rule was met for cases 4 and 5 only: the CDNA target is itself a container with no runtime, so the five cases that inject had nothing to inject into, and what the other five assert is architecture-independent by argument rather than by measurement - re-answer the Open Questions against what shipped: the interception table was NOT complete, and the method that found the five open doors is the transferable result; CDNA multi-tenant sharing is XCC-quantised rather than noisy, and the disjoint-pair aggregate is bimodal - add the AMD section to troubleshooting.md: a preload that silently did nothing, the R0600 miss, a bypass found by set difference, an RTLD_NEXT miss under a dlopen-ing framework, the GLIBC_2.34 ceiling and why an executable cannot pin it away, a timing run with no barrier, and a multi-XCC mask that measured as a working slice while occupying most of the card - name hip/hip_vmm.c in the tree layout and the README's artifact table Task 10 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- name the four checks as delivered, including the exported set asserted as a SET rather than as an absence of foreign names - say why the export check cannot be nm -D | grep, which also matches an imported symbol, and where nm -D -u is the right tool - scope the GLIBC_2.4 figure to the shared object: an executable carries a 2.34 startup stub whatever it calls, so case 1 records rocm-monitor's floor rather than asserting it Task 10 of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
- decide amd-case-1..5 on the payload's own FAILS count instead of a grep for the token, which any row could satisfy from a detail column - drop the FAILS=0 token amd-case-4 printed into every passing row - fail amd-case-3 when a family reports no result at all, rather than read it as a refusal and drop both of that family's arms uncounted - fail amd-case-2 when the baseline probe reports no physical figure, which left four rows comparing "" with "", both control arms among them - fail amd-case-3 when not one caller-origin line was recorded - warn rather than pass in preflight when rocm-smi or the render-node scan finds no card - refuse the artifact checks in build.sh and amd-case-1 when readelf, objdump, nm or strings is absent Proven against the card: the same deliberate break in amd-case-4 reported PASS and exit 0 before this change, FAIL and exit 1 after. The suite is 141 PASS / 0 FAIL either way, and the three degenerate states now produce a FAIL row where they used to pass or skip in silence. Found by the end-of-build review of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…a fork - commit no tracking slot for an allocation that produced no key: a zero-size request succeeds with a null pointer, and freeing a null pointer is defined to do nothing, so that slot could never be given back. Measured before: 17001 zero-size allocations, then 64 MiB against a 4096 MiB quota on an unused card, refused with hipErrorOutOfMemory and 618 tracking refusals. It is served now. - elect one thread to open the region and have the rest wait for its answer, rather than a non-atomic check-then-set in which a thread arriving during the open read "undecided" as "unusable" and refused an allocation on an empty card - reset the process-local state on a pid change, so a forked child neither spins on a lock flag it inherited with no owner nor refunds a charge its parent still holds. The region's latch is reset the same way and for the same reason -- a fork landing inside the open leaves the child waiting on an answer no thread is left to give -- while the mapping itself is kept, because MAP_SHARED across a fork is the design. pthread_atfork is not available here: its names would lift the library's glibc floor off GLIBC_2.4. - state the pid-namespace limit of the reclaim sweep as it is; the previous comment claimed every pid in the table is one this namespace can see, which holds only under shareProcessNamespace - three unit tests for the above, each red against the ledger as it was: zero_size_keeps_no_slot, fork_under_lock, fork_refunds_nothing Verified on two gfx1101 cards: the unit suite reports the three new cases FAIL before the change and PASS after, the seven AMD cases are 144 PASS / 0 FAIL, and build.sh unit mutants stays silent. Found by the end-of-build review of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…eview found wrong - refund a charge only once the runtime has accepted the free, in all five release entries. Refunding first credits the container for memory a refused free left exactly where it was, and it could then take those bytes twice. hipFreeAsync keeps refunding at issue rather than at stream completion, which is a separate and documented choice, but it too now depends on the call's status. - `env -i` is not an escape: /etc/ld.so.preload is a file, so the child still loads the library, and with no figure vrocm_quota_validate marks the container unusable and every allocation is refused. The README said the opposite of the direction it fails in. - the host-memory names in the manifest are not interposed at all; only hipHostMalloc and hipHostFree are counted-never-charged - the Gate-8 occupancy measurement reads 3.7 %, not 3.9 %, in SKILL.md and in the troubleshooting page; the spec and the conformance table were right - the shim README no longer lists the verification skill's arm and the case scripts as outstanding, and the spec's suite figures are the measured ones again: 144 PASS rows, 39 unit cases - `build.sh list` with no argument prints its own message instead of dying on an unbound variable under set -u - the five AMD cases that were not executable now are, like the other two Four facts recorded in Risks rather than changed: the card's in-process lock is a bare spin held across the driver call; re-entry on a different card is refused rather than nested; a pid in the region names a process only within the namespace that wrote it; and the region file's 0600 mode is decided by whichever container creates it. Verified on two gfx1101 cards: 144 PASS / 0 FAIL across the seven cases with the refund order changed. Found by the end-of-build review of amd-gpu-slicing-shim. Signed-off-by: thxCode <thxcode0824@gmail.com>
…t stays there
All 21 cases ended with `echo "${out}" | grep -q 'FAILS=0'`, which searches
every line for the token instead of reading the payload's count. A row that
prints `FAILS=0` in its detail column therefore decided the whole case, and
one did: AMD-CASE 4 printed it in every passing row, so the case that exists
to catch a silently discarded CU mask could not itself fail.
- add xb_fails and xb_verdict to scripts/lib.sh and call them from all 21
cases. The count is matched as a WHOLE line, the last one wins, and a
payload that printed none reads as 1 rather than 0.
- rename the two-part cases' PARTA_FAILS token back to FAILS, which the
anchored parse no longer confuses with a relayed inner count
- preflight FAILs when a case under cases/ decides for itself, checked by
the absence of the xb_verdict call rather than by the shape of the old
bug: a blocklist only catches the mistake somebody already made, and
every way of getting this wrong looks the same from outside
- preflight also checks the helper's own arithmetic on two inputs, because
the row above only asserts that 21 cases call one function, and one wrong
function turns all 21 green together
- preflight decides its own exit the same way; it carried the same defect
- correct the three comments and the two SKILL.md rows that described the
old grep, and record the convention as a hard rule
This is the only thing that runs over these scripts -- no CI job touches
this directory and nothing runs shellcheck over it -- so it is a floor, not
a gate: it catches the next hand-rolled verdict, and only for somebody who
runs preflight.
Verified on all four vendors' own hardware: 21 of 21 cases PASS through the
helper. Seven AMD cases on two RDNA3 cards (144 rows), seven THead cases on
a 16-card PPU host (270 rows), three NVIDIA cases on a two-GPU host, and
four Ascend cases on an AARCH64 910B host. The parse is exercised on every
vendor rather than inferred from the shared idiom, and the suite now decides
verdicts on a non-x86 target.
Both guards were proven able to fail. A hand-rolled verdict put back into
nvidia-case-1 turns the first row red and preflight exits 1; removing the
anchor from xb_fails makes it read a poisoned table as 0 instead of 3,
which the second row catches. The first probe written for that second row
did NOT catch it, because with the poisoned line above the count both
readings agree; the input now puts it below.
Signed-off-by: thxCode <thxcode0824@gmail.com>
- check the staging step's exit status in the buildx path, the way the THead and AMD paths already check theirs. Measured: on a target whose default XB_STAGE is under /opt and whose login user is not root, the image built, `mkdir` was refused, and build.sh printed its success line and exited 0 regardless -- after which every case reports "run build.sh first", which is the one thing the operator had just done. - map `arm64` to linux/arm64. That is what Darwin's uname reports, and XB_MODE=local on an Apple Silicon machine is a real route: it is the only arm64 builder most developers have, and the Ascend stages are arm64. Both found by running the suite against NVIDIA, THead and Ascend hardware. Signed-off-by: thxCode <thxcode0824@gmail.com>
… it loads `/etc/ld.so.preload` does not distinguish a workload from the `sed` in a start-up script, so complaining at load meant a container carrying this library and no configuration printed a line per process. Measured: three `sed` invocations, three lines, and nothing an operator could act on. - log the absent ledger path at level 2 when nothing else was configured either, and keep it at level 1 when a memory figure IS set -- that pair is a misconfiguration, and the only one worth a line at load - say it instead on the first allocation a process is refused for want of a configuration, once per process. That is the process that needs the reason, and it is the only one that does; without this the demotion above would have made a real refusal silent. Measured after, in a container with the shim preloaded: with the variables injected the way the device-plugin injects them, nothing is printed at all; with a quota and no path, the misconfiguration is still one line per process; with neither, the `sed`s are silent and the process that asks for memory prints one line and is refused. The latch resets on fork with the other process-local counters, so a child is not refused without a reason of its own. Signed-off-by: thxCode <thxcode0824@gmail.com>
…out the region Two things an integrator needed and had to read the sources for. - a table naming every variable this library reads beside the THead shim's equivalent, with the unit and what absence means for each. Three of the differences are deliberate and now say why: this side has no default ledger path, it has no compute variable at all because the platform enforces compute through HSA_CU_MASK, and it distinguishes a figure that is set-but-unusable from one that was never set. - the usage region documented as a byte layout -- every offset, both struct sizes, and a worked `od` read of a real slice -- because `rocm-monitor` is not the only reader it is meant to have, and a metrics scraper cannot be asked to preload a slicing library into itself. The two consequences of reading it without the lock are stated rather than left to be found. Also records what a correctly injected container prints at the default level, which is nothing. Signed-off-by: thxCode <thxcode0824@gmail.com>
… region copy Both from a Copilot review of the branch. - `vrocm_entry_hit` incremented `entry->traced` with a plain `++` while `calls` and `denials` beside it were already atomic. It is reached only at log level 2 and only bounds how many caller-origin lines an entry prints, but the wrappers run in frameworks with dozens of threads and a data race is undefined behaviour whatever it guards. Relaxed atomics, since nothing is ordered against it; the cap stays approximate either way. - rename `rocm-monitor`'s stack copy of the region from `header` to `snapshot`. It was always a whole `struct vrocm_region`, so the size test before `mmap` already required the full 34912 bytes — but the name said otherwise and the review read the guard as header-sized and reported a truncated file as a SIGBUS waiting to happen. Two error messages said the same wrong thing and now report what is actually required. The SIGBUS itself is not reachable and no check was added for it. Signed-off-by: thxCode <thxcode0824@gmail.com>
…ult cannot do
`VROCM_LEDGER_PATH` now falls back to `/dev/shm/vrocm-ledger`, the shape and
the location the THead shim already uses, so this tree can be run by hand
with one variable instead of two. An empty value falls back too: that is how
a variable arrives when a substitution had nothing to put in it.
The default is only safe for ONE container, and the README, the header and
the spec now say so in the same words. /dev/shm is container-private until
something shares it, and two ordinary configurations do: `hostIPC: true`
makes it the host's, and an `emptyDir{medium: Memory}` mounted at /dev/shm is
shared by a Pod's containers -- the usual answer to a data loader that finds
64 MiB too small. Either way two containers charge two DIFFERENT physical
cards into the slot they both call index 0, because the region is addressed
by each container's own position in ROCR_VISIBLE_DEVICES, and nothing reports
it. The quota is silently wrong rather than visibly absent. The `.sliced`
path must therefore always set the variable; the device-plugin does.
- the load-time complaint now turns on whether a FIGURE was named, not on
whether the path was: with a default the path is never absent, so without
this the per-process noise would simply have come back in another form
- `rocm-monitor` resolves the same way, taking both the name and the default
from the shared header rather than spelling either twice. It still links no
object from `common/`, so the resolution is repeated rather than called.
- two unit rows pin the fallback and the empty-value case
The THead shim carries the identical exposure for the identical reason and is
documented separately.
Measured: with a quota and no path, the allocation is served and accounted
into the default region and `rocm-monitor` finds it with no arguments; with
nothing configured, the non-workload processes stay silent, the process that
asks for memory prints one line and is refused, and NO region file is
created -- admission returns before the region is ever opened.
Seven cases 149 PASS / 0 FAIL, 44 unit cases, mutants and linkage checks
clean.
Signed-off-by: thxCode <thxcode0824@gmail.com>
`VPPU_LEDGER_DEFAULT_PATH` explains why /dev/shm was chosen and not what the
choice costs. The deviceplugin comment beside the allocator has the missing
half -- the region is addressed by a CONTAINER-LOCAL card index, so a shared
location lets two containers' index 0 charge one slot -- but a reader of the
header alone would take the default for a safe fallback.
It is safe only while /dev/shm is container-private, and two ordinary
configurations share it: `hostIPC: true` makes it the host's, so every
container on the node meets in one region; and an `emptyDir{medium: Memory}`
mounted at /dev/shm is shared by a Pod's containers, which is the usual
answer to a data loader that finds the default 64 MiB too small. In either
case two containers charge two different physical cards into the same slot
and nothing says so -- the quota is silently wrong rather than visibly
absent.
Nothing changes here: the allocator already sets `HGGC_LEDGER_PATH` to a
per-container directory under the pod work dir, so the default is reached
only by a tree run by hand. This records the boundary that makes that true,
in the same words the AMD shim's `VROCM_LEDGER_PATH` now carries.
Signed-off-by: thxCode <thxcode0824@gmail.com>
b556832 to
f9c6e39
Compare
Suppressed comment — outcomeOne this round, and it is the best finding on the PR so far, so it gets its own note rather than a
The guard exists because # this case decides through xb_verdict at the endwhile hand-rolling its verdict underneath. A guard against unfailable assertions that was itself Now anchored to a call rather than a mention: grep -qE '^[[:space:]]*xb_verdict ' "${c}"Checked both directions rather than only the one that matters to me: all 21 cases still match, and The lesson generalises past this line, and it is the third time on this PR: a check whose subject is |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated no new comments.
Suppressed comments (5)
csrc/amd/rocm-slicing-shim/hip/hip_query.c:126
- hipDeviceTotalMem unconditionally replaces the runtime-reported device total with the configured quota. If the quota is misconfigured above physical VRAM, this will report an impossible capacity to callers. Clamp the reported value to min(runtime_total, quota).
csrc/amd/rocm-slicing-shim/hip/hip_query.c:166 - hipGetDevicePropertiesR0000 sets totalGlobalMem to the configured quota even when the runtime reports a smaller physical total. Clamp totalGlobalMem to min(runtime_total, quota) to avoid reporting impossible capacity under misconfiguration.
csrc/amd/rocm-slicing-shim/hip/hip_query.c:191 - hipGetDeviceProperties (plain) sets totalGlobalMem to the configured quota even when the runtime reports a smaller physical total. Clamp totalGlobalMem to min(runtime_total, quota) to avoid advertising capacity the device cannot provide.
csrc/amd/rocm-slicing-shim/hip/hip_query.c:146 - hipGetDevicePropertiesR0600 sets totalGlobalMem to the configured quota even when the runtime reports a smaller physical total. This can mislead frameworks into sizing allocations larger than the device can support. Clamp totalGlobalMem to min(runtime_total, quota).
csrc/amd/rocm-slicing-shim/hip/hip_query.c:107 - hipMemGetInfo overrides total_bytes with the configured quota even when that quota exceeds the runtime-reported physical total. That can cause callers to believe more VRAM is available than the device can supply. Clamp the effective quota to the physical total before rewriting total/free.
This issue also appears in the following locations of the same file:
- line 123
- line 143
- line 163
- line 188
Delivers the AMD ROCm logical-slicing shim — the library and its verification, not the
device-manager wiring, which the spec names as a Non-Goal and a later change.
specs/2026-08-06-amd-gpu-slicing-shim.mdcarries the design, the measurements behind it, and theRisks it does not close.
What lands
csrc/amd/rocm-slicing-shim/—libvrocm.so, preloaded into a sliced container through/etc/ld.so.preload, plusrocm-monitor(reads a slice's usage without touching the ledger'swriter) and
rocm-cumask-check(fails a card whose CU mask the platform silently discarded).MAP_SHAREDledger under an
fcntlrecord lock. Check, allocate and charge happen under ONE lock hold, whichis what closes the check-then-allocate race between processes.
hipGetDevicePropertiesR0600among them. ROCm 6+ headers rewrite the call to that name, so awrapper on the plain one interposes a symbol nothing calls; measured, only the R0600 variant is
ever reached.
its only
DT_NEEDEDislibc.so.6, and its glibc floor isGLIBC_2.4so it loads into anUbuntu 20.04 or RHEL 8 workload image.
The interception table was completed by subtraction, not by review. Every allocating name
libamdhip64exports, minus the interposed set, measured. That found five open doors review hadmissed —
hipMalloc3D,hipMemCreate,hipMemAllocPitch,hipArrayCreate,hipArray3DCreate—each of which took 512 MiB out of a 64 MiB quota before it was wrapped. The width of the
denominator is part of the claim, and the first manifest's was a guessed regex that four of the
five fell outside.
.claude/skills/gpustack-operator-xbuild-and-verify/— thexbuild-amd-rocmarm, sevenamd-case-*.shscripts, the AMD preflight rows and the reference pages.Verification
Every case ran on the vendor's own hardware. 21 of 21 pass.
The AMD suite also ran against a CDNA host for cases 4 and 5 — the two whose arithmetic differs by
NUM_XCC— which is a partial result and says so in the spec.The harness had to be fixed before any of that meant anything
AMD-CASE 4printed the literalFAILS=0into every passing row's detail column, and the verdictgrepped the output for that token. The case built to catch a silently discarded CU mask could not
itself fail. Measured: one deliberately broken assertion printed a red row, printed
FAILS=1, andstill exited 0 saying PASS.
All 21 cases now decide through one helper in
lib.shthat reads the count as a whole line, andpreflight.shfails if a case decides for itself or if that helper's own arithmetic is wrong. Bothguards were proven able to fail before they were trusted; so were the three unit tests added for
the ledger defects below.
Known and recorded, not closed
hipGraphAddMemAllocNodeis measured open (512 MiB under a 64 MiB quota). A graph node allocatesat launch rather than at capture, so the accounting is no longer one call in and one call out.
bandwidth.
.slicedis a compute ceiling, never a QoS.hsa_*or/dev/kfdcallers never pass through this library. That is a boundary of theapproach, not a defect, and this is not a security boundary either way.
process only within the namespace that wrote it; the region file's
0600mode is decided bywhichever container creates it.