Repository navigation
os/v3/puppy: ship the NPU TDR timeout and a release row that crosses the position count the board wedged at - #1020
Conversation
470bfed to
9b6eeed
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY — but merge it after #1016: this branch contains #1016.
Merge order first, because it is the kind of thing that costs an hour later. base says main, but both #1016
commits are ancestors of this head:
origin/main..pr1020:
9b6eeed os/v3/puppy: ship the NPU's TDR timeout, and a release row that crosses …
9c0b734 …a self-check for the inbound rules… <- #1016
9f33671 …the firewall drops every mDNS query… <- #1016
git merge-base --is-ancestor <pr1016 head> <pr1020 head> -> yes
git diff --stat pr1016 pr1020 -> 4 files, +117 (its own content)
So: #1016 merges first, then #1020 (or #1020 gets rebased onto it). If #1020 merges first it silently lands #1016 as
well, and #1016 then goes CONFLICTING for no reason — the same trap as engine #983/#995 and fork #184/#178.
The parameter name is real, not assumed. This box:
$ modinfo -p amdxdna | grep tdr
tdr_timeout_ms:TDR (Timeout Detection and Recovery) timeout in milliseconds (0 = disable) (uint)
$ ls /sys/module/amdxdna/parameters/
aie2_max_col force_cmdlist force_iova tdr_timeout_ms
and the header's claim about where the box's value comes from checks out — /etc/modprobe.d/amdxdna.conf on this box
opens with # Linux 7.3 amdxdna renamed timeout_in_sec to tdr_timeout_ms and carries
options amdxdna tdr_timeout_ms=15000, i.e. the box reads 15000 only because that file exists, which is exactly why a
stick without one gets the 2000 ms default. Worth noting the collision: on this box amdxdma.conf is a pre-existing
2026-09-05 file with the same name the PR installs — harmless (same value), but a good reminder that the image's copy is
not the only file that can set it.
The row asserts the right thing. vm-check's new U5 npu tdr reads the running module
(cat /sys/module/amdxdna/parameters/tdr_timeout_ms -> tdr-loaded=15000) rather than trusting the file, which is the
assertion that matters; the ladder reuses the existing tdr_count() (counts aie2_legacy_tdr_detect in dmesg) and
compares it across each rung, so a TDR is a FAIL and an unreadable dmesg is reported as "unknown" instead of "0".
container-build.sh refuses to pack the image without the option, and refuses if another packed file shadows it.
bash -n clean on all three scripts, shellcheck adds nothing: release-check.sh reports the same 5 findings as on
main (all pre-existing), container-build.sh and vm-check.sh are 0.
Three notes, none a gate:
- The shadow scan keys on the file NAME (
grep -E 'modprobe\.d/[^/]*amdxdna[^/]*$'), so a packed file that is not
named*amdxdna*but carriesoptions amdxdna tdr_timeout_ms=2000would slip past it. Nothing in the c22 image does
that (there was no amdxdna config at all, which is the bug), andU5 npu tdrreading the running value is the real
backstop — so this is a "would be nicer to scan contents", not a hole in the guarantee. - The unload between rungs is not verified. The comment says a fresh backend per step is what makes the ladder
meaningful ("a warm context would let one step hide the next"), but thePOST /api/v1/unload {}result is discarded.
Same note I made on #999's evict arm. The row would still catch the wedge with a warm context (the positions
accumulate either way), so the claim is stronger than the check — oneGET /api/v1/models-is-empty assertion per rung
would make it true. - The ladder stops at the first bad rung (
break), so a run that fails at 600 reports nothing about 1000. That is
deliberate (don't hammer a wedged device) and I would keep it; just noting it so the FAIL text is not read as "the
later rungs were fine".
PR Reviewer Guide 🔍(Review updated until commit 60e3185)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit 883e0bb |
|
Follow-up at Measured on the cycle-23 image why the load cannot happen in this harness: Also worth a line in the hand-off for whoever re-signs modules: the two certificates share |
|
Persistent review updated to latest commit 81692e3 |
|
/review |
|
Persistent review updated to latest commit 81692e3 |
…es the position count the board wedged at The c22 board wedged deterministically on a long chat (4 Nov): 400 max_tokens (429 positions) and 480 (509) answered, 600 - the first step past 512 - gave `npu runtime: invalid call (code 614): state 8` plus one amdxdna TDR and a power-cycle, and both board chat failures that day sat ~7 s into a thinking-on reply. The image ships no kmod config for the NPU at all (its /etc/modprobe.d/ has alsa/apm/ath9k/puppy/radeon and 1bit-quiet), so amdxdna loads with the driver's default hang-detector timeout and tears down a device that was going to answer. * `embed/etc/modprobe.d/amdxdna.conf`: `options amdxdna tdr_timeout_ms=15000`, the value the driver lane measured on strixhalo (the param is 7.3+'s rename of timeout_in_sec, and `modinfo -p` on the shipped module lists it, so the option is not silently ignored). * `container-build.sh` installs it and refuses an image whose PACKED sfs lacks it, or where any other packed `modprobe.d` file names amdxdna without it: kmod merges the directory and a later `options` line wins, so a stale copy shadows the installed one and nothing fails (the strixhalo lesson). * `vm-check.sh` gains `U5 npu tdr`: it loads the module (it loads without NPU hardware) and asserts the value the KERNEL took, `/sys/module/amdxdna/parameters/tdr_timeout_ms` = 15000 - the same reading the board row wants. * `os/mini/release-check.sh` gains `chat-npu-ladder`: fresh backend (unload first), temperature 0, /no_think, a long-essay prompt at 400 / 600 / 1000 max_tokens, and any new `aie2_legacy_tdr_detect` line fails the row. That is the ladder the board failed; the existing rows (one short question, a 512-token turn, one streaming turn) never crossed it. Not claimed: #1017 shows the same ladder answering 661 positions clean on the fleet boot with the same binary, kernels, module and firmware, so "512 is a design limit" is unsupported. This ships the mitigation the cycle ordered plus the row that will say on the board whether it holds - and #1005's after-TDR client recovery is a separate, unmerged engine change (listed as open in the hand-off). closes #1019
…t the loaded value `modprobe amdxdna` in this harness is refused `Key was rejected by service`, measured on the cycle-23 image: the sb-vars.sh Secure Boot store enrolls ONE certificate in db - the one that signs the KERNEL, /var/lib/shim-signed/mok/MOK-Kernel.pem, subjectKeyIdentifier 27:68:3E:38:… - while the modules are signed by a DIFFERENT certificate with the same subject CN (`modinfo -F sig_key` = 35:5C:A4:8A:…). The board loads this module, so its MOK keyring trusts that second key and the harness does not. So the row asserts what the image actually ships and the kernel will apply - `modprobe -c` resolving `amdxdna ... tdr_timeout_ms=15000` (plus the packed file, printed) - and the RUNTIME reading is the board row: /sys/module/amdxdna/parameters/tdr_timeout_ms must read 15000 there. That is what the cycle's item 1 asks for anyway; the alternative was a row that stays red in every VM for a harness reason and gets ignored. Measured on the c23 image: `PASS U5 npu tdr ... kmod-view=1 conf-file=1 | runtime=not-loaded`, 0 FAIL in vm-check.
81692e3 to
8e4a069
Compare
|
Rebased onto current The |
|
Persistent review updated to latest commit 8e4a069 |
|
/review |
|
Persistent review updated to latest commit 8e4a069 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8e4a069a3 — re-verdicted after the author's push, which changes two things and both are the right call.
1. U5 npu tdr now asserts kmod's resolved view instead of the loaded value, and the reason is worth keeping. In the
VM harness modprobe amdxdna is refused Key was rejected by service (the enrolled db cert signs the kernel,
MOK-Kernel.pem, SKI 27:68:3E:38…; the modules carry a different cert with the same CN, sig_key 35:5C:A4:8A…), so the
old row's tdr-loaded=15000 could never pass there while the board trusts the module and loads it. The replacement is
tdr-config-15000 from modprobe -c, which I checked on this box:
$ modprobe -c | grep -i amdxdna
options amdxdna tdr_timeout_ms=15000 (the row's grep counts 1 here)
That is a genuinely better assertion than "the file exists", because modprobe -c is the resolved view — a shadowing
options line elsewhere makes the count 0, which the file-based check could not see. The runtime reading is still printed
(runtime=…) and still required on the board, and the row says so rather than pretending the VM covers it. The SKI/CN
mismatch is your measurement; I could not check it here (root-only, and this box loads the module fine).
2. The new build refusals hang off #1001, which has merged — verified, so they are satisfiable today:
git show origin/main:os/v3/puppy/embed/usr/local/bin/1bit-logs | grep -c '^mounted() ' -> 1 (the refusal's pattern)
git show origin/main:os/v3/puppy/embed/etc/init.d/06onboard-logs | grep -c 'stay in RAM' -> 0 (the refused line is gone)
So container-build.sh's two new refusals (the 06onboard-logs give-up line, and a 1bit-logs with no mount-point
test) fail exactly on the two silent-log-loss shapes and pass on the merged fix. U3 boot logs is also stronger now
(data-is-mount + logs-daemon-running + the existing marker), and U3 logs wait is a real late-mount test: daemon
started on a non-mount, tmpfs mounted over it afterwards, assert pre=0 and post>=1 without anything restarting the
daemon — which is the 4 Nov failure reproduced as a row.
The stacking constraint is gone: #1016 merged at 18:30Z, so this branch no longer carries it (its own diff is now just
its four files) and it merges standalone. MERGEABLE/BEHIND; pr_agent fail is the cancelled-duplicate pattern.
bash -n clean on all three scripts; shellcheck adds nothing over main (release-check.sh keeps its 5 pre-existing
findings, the other two are 0).
|
Persistent review updated to latest commit 60e3185 |
…cle-23 board is green os-v3.md said the appliance ships `7.3.0-rc4-1bit-perfopt`. Measured in the frozen images rather than taken from a note: the c20 image carries `7.3.0-rc4-1bit-perfopt+`, and the c21, c22 and c23 images carry `7.4.0-rc1-1bit` (string present in each), so the page now says which cycle moved it. The same paragraph's newest board was cycle 20; cycle 23 (4 Nov) is in, with its row counts and the two facts the c23 verdict records - the single red is a bug in the check's own u7-mdns row, not the image. clients.md: three citations re-pointed at the lines that now carry them after #1020 moved vm-check.sh and container-build.sh by 13 and 1 lines.
c22 board, 4 Nov 2026 — deterministic,
TOS3-C23-NOTES+TC22-NPU-TDR:What the image was missing (both invisible in the source tree)
alsa-base.conf,amdxdna-lessamdxdna-nothing,apm.conf,ath9k.conf,puppy.conf,radeon-dpm.conf,1bit-quiet.conf— so the module loads with the driver'sdefault
tdr_timeout_msand tears the device down mid-answer. (The box reads/sys/module/amdxdna/parameters/tdr_timeout_ms= 15000 only because strixhalo happens to have/etc/modprobe.d/amdxdna-tdr.conf; a stick does not.)1bit-release-check's NPU rows ask one short question (chat-npu), a512-token turn plus a short follow-up (
chat-npu-followup) and a reload+streaming turn (chat-npu-stream). Noneof them asks the ladder the board failed at, so a wedged image could carry a green release check — which is what
the c22 board did.
The change
os/v3/puppy/embed/etc/modprobe.d/amdxdna.conf(new)options amdxdna tdr_timeout_ms=15000, with the board numbers and the param's 7.3 rename in the headeros/v3/puppy/container-build.shmodprobe.dfile that names amdxdna without it (kmod merges the directory, a lateroptionsline wins — a stale copy shadows the fix and nothing fails)os/v3/puppy/vm-check.shU5 npu tdr:modprobe amdxdna(it loads with no NPU hardware), then assert/sys/module/amdxdna/parameters/tdr_timeout_ms= 15000 — the value the kernel took, not the fileos/mini/release-check.shchat-npu-ladder: fresh backend per rung (unload first), temperature 0,/no_think, long-essay prompt at 400 / 600 / 1000 max_tokens; any newaie2_legacy_tdr_detectline fails the row, and it reports completion and prompt tokens per rungThe new row is added to the hardware-only skip list, so it prints
SKIP chat-npu-ladderon a machine with no NPU(the release check goes 21 → 22 rows).
Verification done here, and what only the board can do
bash -n/sh -nclean;tools/copyright.py --checkrc 0;shellcheck -S warningon the touched files showsthe same 8 pre-existing warnings as
origin/main's copy ofrelease-check.shand none new (checked by diffingboth runs' warning sets).
the c23 cut:
vm-checkU5 npu tdrPASS on the built.img, and the image's own1bit-release-checkprintingSKIP chat-npu-ladder(wired, 22 checks). The runtime half —tdr_timeout_ms= 15000 on the board's loadedmodule, and the ladder answering 1000 — is the board row and is listed as such in the hand-off.
boot with the same binary, kernels, module and firmware, so "512 is a design limit of the dx route" is not
supported. This ships the mitigation the cycle ordered and the row that decides it on hardware.
closes #1019
Root cause, corrected (pi-hrx-fabric, 4 Nov 14:0x, with the c22 stick's own bytes)
The appliance shipped no modprobe override, so
tdr_timeout_mswas the module default 2000 ms. The detectorneeds two intervals with no
aie2_tdr_signal(~4 s at 2000 ms, ~30 s at 15000 ms), and the board's failing chat took~4 s longer for the same 629 positions than the identical stack takes on the box (12.4 s vs 8.1 s). On the box at
15000 ms the same stack answers 429 / 509 / 629 / 1029 positions with 0 TDRs, and the board's capture
(
DPU PC 0xffffffff,Timed out sub command ID: 0) is shape-identical to a false positive provoked on a healthyarray. So the honest framing is the one in the hand-off: NPU chat works; a host-side pause of >=4 s trips a 2000 ms
watchdog and costs the client its backend — at 15000 ms that becomes a >=30 s pause instead. This PR is parity with
the interval every NPU number we quote comes from, plus the row that says so on hardware. The file header carries the
same correction.