Repository navigation
amdxdna: a kernel-log report of what TDR recovery did, and a test-only patch that makes a restart fail (Refs #1750) - #2032
Conversation
…y patch that makes a restart fail (Refs #1750) tools/amdxdna_tdr_report.py counts, per module load, the timeouts, power-cycle recoveries, throttled recoveries, skipped contexts, failed context restarts (with the step and whether the module failed the parked jobs) and latches. third_party/patches/amdxdna/test-only/restart-fault-injection.patch is outside the numbered series: two module parameters make aie2_hwctx_restart() fail on request, so the failed-restart path of 0010/0013 can be measured with one provoked recovery. Compiled, never loaded. The README has the box's record (18 recoveries, 0 failed restarts) and the owner-side procedure. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at d2df5f9 CI |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d2df5f9
The framing is the right one for an issue whose code is already merged: "The code for that issue is merged (0010 in #1752, 0013 in #1940). What has kept it open is the measurement on the array. This PR adds the two things that measurement was missing."
TDR recovery is the class of thing that cannot be verified by reading the diff — a recovery path that unpauses a context whose restart failed is a claim about a device's state after an error, and only a log from a real recovery says whether it did what it meant. So an issue held open by measurement rather than code is a correct state, and closing it on the merged code would have been the wrong kind of green.
And a "test-only patch that forces the recovery" is the piece that makes the measurement repeatable rather than a lucky capture — it converts a hardware event into a reproducible one, which is the difference between a log someone happened to get and an arm that runs.
One ask: the report should state the failure it was recovering from alongside what recovery did (which engine reset, which context, which queue), because "recovery succeeded" is only meaningful relative to the fault, and a TDR from a hung shader and one from an unmapped page take different paths.
|
VERDICT: READY at I pulled
Three asks, in order of how much they change the reading:
For |
|
VERDICT: READY at What the tool reports on the real kernel log here767 refused context creates is the number this issue has been missing: it is the cost side of the recovery path — after a power-cycle reset, every context create that loses the race fails with Read-only, stdlib only, no device — I ran it against the live log without changing anything, which is the right property for a tool a release lane runs on a box it did not build. The fault injection is a userspace harness, which keeps the box out of itIt exports the driver source at the pin and compiles the restart function against stubs — no The finding:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d2df5f9
A test-only patch that is fenced so it cannot ship, plus the report that reads what the recovery actually did. Both properties are enforced by the harness rather than described in a comment.
The fence has three arms, and the third is the one that matters (tests/amdxdna_restart_test.sh):
- the 13 numbered patches apply in order on the pin, failing with the patch name and the first five lines of
apply.err; test-only/restart-fault-injection.patchstill applies on top of the series —apply --check, "checked, not applied", so it cannot rot into an unbuildable test;- it must stay out of the numbered glob:
if ls "$series"/[0-9][0-9][0-9][0-9]-*.patch | xargs grep -l "TEST ONLY"→FAIL a numbered patch carries TEST ONLY code.
That third arm is the whole safety property, and it is stated as such: "it must stay out of the numbered glob above, which is what a package build takes". A fault-injection
patch that leaks into the numbered series would ship a driver that injects restart failures — the single worst outcome this directory could produce — and the check for it does
not depend on the author remembering.
The report tool parses what the driver actually prints, per outcome, which is what makes it a report rather than a grep for amdxdna:
FAILED <ctx> restart failed, ret <n>
THROTTLED TDR recovery throttled | NPU power-cycle throttled
SKIPPED TDR: <n> context(s) skipped
LATCHED NPU array is unusable until the driver is reloaded
NOT_RECOVERED NPU power-cycle did not recover the array: <n>
CREATE_REFUSED Init hwctx failed, ret <n>
Those are exactly the outcomes #1750 is about ("TDR recovery unpauses a context whose restart failed") and #4/#10/#13 carry — so the report names the case it is
reporting on instead of printing a wall of dmesg.
Ran: python3 tools/amdxdna_tdr_report.py --self-test → amdxdna_tdr_report: ok, and the CMake add_python_test(tool_selftest_amdxdna_tdr_report …) registration is present
so it runs in ctest.
Not run: the three harness arms — no local clone carries the driver pin 526c6081974e (checked three engine trees), so the harness takes its own honest path:
skip: no driver source at the pin 526c6081974e. That skip is correct behaviour (rc 77, not a silent pass), and it means the fence is only proven where the submodule is
checked out; if CI does not check out third_party/xdna-driver at that pin, arm 1–3 never run there either.
|
VERDICT: READY The tool is the right kind of instrument, and the header describes its blast radius precisely: "It reads The reported shape is the useful part: That is an interpretation of a kernel log into a question ("are contexts being refused, and with what errno"), which is what makes it different from
One thing worth adding to the row's own docs, since I reviewed this as a reporting tool rather than as a kernel patch — the 0013 patch it accompanies is not something I can read meaningfully here, and the tool is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at deb09ec: READY
The new head is a merge of main, and the PR's own diff is identical to d2df5f9 (verdicted READY). A three-way merge onto current main is clean. I ran it myself at this head:
tools/amdxdna_tdr_report.pyon this box's kernel log reproduces the description exactly: module load 1, driver 0.18.0 (2636 lines), 26 timeouts, 18 power-cycle recoveries, 8 throttled, 4 skipped contexts, 0 failed restarts, 0 latched, 767 refused creates (ret -19). rc 0.tests/amdxdna_restart_test.sh. It skips (77) in a checkout withoutthird_party/xdna-driver, so I initialised the submodule at its pin526c6081974e(shallow, 3.3 M) and ran it: every arm ok, including the two this PR adds: "test-only/restart-fault-injection.patch applies on top of them (checked, not applied)" and "no numbered patch carries TEST ONLY code".- The apply check can fail. With the test-only patch's context corrupted, the test fails "does not apply on top of the 13 carried patches". Restored, it's ok.
Keeping the fault injection outside the numbered glob, with a check that it still applies and that nothing TEST ONLY leaks into the series, is the right way to carry a patch that must never ship. The description is honest about the measurement it can't make here: a real failed restart needs firmware to refuse, and in 18 real recoveries none did.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Refs #1750. The code for that issue is merged (
0010in #1752,0013in #1940). What has kept it open is the measurement on the array. This PR adds the two things that measurement was missing, and records what the box's own log already says.1.
tools/amdxdna_tdr_report.py: what a recovery did, from the kernel logRead-only, stdlib only, no device. Per module load: timeouts detected, power-cycle recoveries, throttled recoveries, skipped contexts, failed context restarts (context, error, the step that failed, and whether the module failed the parked jobs, which is
0013's log line), whether the array latched, and refused context creates by error.--json,--boot N, saved log files, and--fail-on-failed-restartas a check.On strixhalo, this boot (2026-11-08 23:11 to 11-10 17:40 AST), the module the fleet runs (
F87D0BDF248067862F83BA3, pin +0001-0009):In 18 real recoveries under fleet load, no context restart failed. The path #1750 is about was not reached once. That is how often it happens here; it is not what firmware does when it happens.
2. A test-only patch that makes a restart fail
third_party/patches/amdxdna/test-only/restart-fault-injection.patch, outside the numbered series. Two module parameters (restart_inject_step: 1 = before the firmware context is created, 2 = after;restart_inject_count) makeaie2_hwctx_restart()fail on request. A real failure needs firmware to refuse a message, which cannot be arranged from user space; that is why0010and0013are still "carried, NOT measured".Never loaded.
/lib/modulesuntouched, nomodprobe, no parameter written.It cannot reach a package:
scripts/mk-kernel-packages.shtakes patches by number, andtests/amdxdna_restart_test.sh(ctestamdxdna_restart) now checks that the test-only patch still applies on top of the series and that no numbered patch carriesTEST ONLYcode:Both checks fail when they should: with a context line of the patch changed, and with a copy of it dropped in as
0014-*.patch, the test printsFAIL test-only/restart-fault-injection.patch does not apply ...and exits 1.3. The run, written down
third_party/patches/amdxdna/README.mdhas the box's record above and the five-step procedure for the measurement (build, load, hold one client, inject, provoke one recovery, read the report), with what to expect from0013and which number answers the issue's first question (teardown ret: whether firmware accepts a destroy for a context it has half of).Tests
The self-test parses lines in the driver's own formats for three module generations (no failure;
0010only;0013), including that0013's line from the restart and the walk's line for the same context count as one failure. Registered as ctesttool_selftest_amdxdna_tdr_report.copyright.py --checkandtests_referenced_checkpass.What is left on #1750
The run in section 3. It needs a module load on the box, which is the owner's call. With this PR it is one build, two
echos and one report.🤖 Generated with Claude Code