Repository navigation
third_party: the amdxdna partition fix (client-scoped, measured) and the TDR recovery pair the fleet runs - #1085
Conversation
435f35d to
3e803a1
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
HOLD — do not merge in this shape. Not a comment on the root cause, which is right; it is the orchestrator's 2026-11-05 decision on NPU exclusivity (0011 / #1084 / #1085), and this patch implements the wrong one of the two candidate shapes.
The decision, verbatim in substance: exclusivity is per client — per open file / process, not per context. One process may hold several contexts (the ZR1 / per-block-count routes and --npu-streams N), and a second process is refused.
What 0003 does instead: pt_node->exclusive = true. exclusive is a property of the partition node and it is evaluated inside allocate_partition(), i.e. per context creation, with no notion of which process is asking. So on this array — where the AIE2P designs take all 8 columns, which is the very premise of the patch's own analysis — the rule that fires is "no free column range", and it fires identically for a second process and for a second context in the same process:
same process, second context (--npu-streams 2, or a per-block-count route) -> -ENODEV
second process -> -ENODEV <- the wanted refusal
The patch's own comment states the mechanism plainly — "get_free_partition() still succeeds whenever a distinct column range is free … Only the fatal case — no free range — now ends in a clean -ENODEV at context creation". That is correct as a description and it is exactly why the shape is wrong: with an 8-column design there is never a free range for the second context, whoever owns it. The engine asks for several contexts on purpose: app/forward_serve.cpp accepts --npu-streams N up to kNpuMaxStreams ("the device's hwctx limit") and calls take_npu_device_lock(streams) — one process, N contexts, which #640 measured on this box.
So the collateral is our own routes. --npu-streams 2+ and anything that opens a second context in one process would start failing at context creation on a build carrying 0003, and the failure would look like a device error rather than a policy one.
What the shape needs to be: the claim must be taken and released at the client level — one owner of the array partition per open file / process, released on close / exit — while that owner's further contexts allocate from the same claim. In the driver that is not a one-word change: the solver has no owner identity, so it needs a claim on the partition (or on the device) held by the first client, with subsequent allocate_partition() calls from the same client allowed through, and a clean -ENODEV for a different one. pi-hrx-fabric is revising 0011 to that shape and pi-kernel-74 is proving it (both proofs again plus a one-process-two-contexts row), per the decision.
The other half of the decision is not this PR's and I am not asking for it here: Lemonade treating the NPU as an exclusive device (one NPU model loaded, evict-on-second, requests queue behind eviction), with the engine mapping code 102 to the queue rather than to an error.
What is good and should survive the reshape: the root-cause write-up is the clearest statement of the wedge anyone has produced — nshared++ on a shared partition, both clients in Partition Index 0, Columns [0..7], 3/3 wedges sequential and 3/3 concurrent, 614 / run 0 of 28 with DPU PC 0xffffffff, and the single-context control clean at 1 000–3 408 positions. The measured-before table is the right instrument and the "not yet run on hardware: srcversion 9E581709A641EEF3C4F37C5" line is honest. When the per-client version lands, that harness is what proves it — plus the new row (one process, two contexts, still works).
I will re-review on the next head. Nothing here should be merged before that; the release stick carries 0008+0009 without 0010, and this patch is not in that set.
PR Reviewer Guide 🔍(Review updated until commit 4a8d9fd)Here are some key observations to aid the review process:
|
0ab8f3e to
448eb94
Compare
9fb2a23 to
ab82d1c
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
The hold condition is met — the patch now carries the per-client shape. Re-reviewing at ab82d1c1 against the orchestrator's 2026-11-05 decision (exclusivity per client / per open file, one process may hold several contexts; a second process is refused):
struct alloc_requests { … u64 owner; }; /* who owns the request */
struct partition_node { … u64 owner; }; /* the client that shares it */
xrs_req->owner = (uintptr_t)hwctx->client; /* one per open file */
pt_node->owner = req->owner;
… if (pt_node->owner != req->owner) continue; /* share within one owner, never across */That is the distinction I asked for. exclusive was per-partition-node and evaluated without identity; owner is derived from hwctx->client, which is the open file, and it is carried onto the partition node so allocate_partition() can tell "the same client's second context" from "a different client". get_free_partition() is unchanged, so a distinct column range still serves two clients concurrently — the reservation is no longer doing the refusing, the owner comparison is.
And you measured the exact failure I predicted, which is the best thing in this revision: the earlier per-context version refused ctx #2 of a single client's multi-context route (code 102, 3/3, ZR1 per-op forward). That is why --npu-streams would have broken, and it is now a measurement rather than my reading of the struct.
What is still owed, by the decision's own wording: the kernel lane's hardware rows on a build carrying this patch — two clients → the second refused with code 102, the first finishes, the partition frees on exit — plus one client's several contexts loading again. The README marks that row a candidate and the body says neither is claimed, which is honest and is also why I am not calling this mergeable on my own authority: the shape is right, the proof is the kernel lane's and the orchestrator's.
One soft spot in the scheme, worth a line of code rather than a paragraph. The rule is only as good as owner being set: an alloc_requests that never gets an owner carries 0, and two clients whose requests both carried 0 would be "the same owner" and share. Here the assignment sits beside the rest of the per-context request setup, so every request built on that path gets one — but hwctx->client can be NULL for a kernel-internal context, and that is exactly the value that collapses to 0. A WARN_ON_ONCE(!hwctx->client) next to the assignment would make the one case that could defeat the scheme loud instead of silent.
Also confirmed as part of this revision: 0004 (two TDR recovery outcomes are not failures) and 0005 (check the throttle before touching any context) come with it, and the fingerprint table now names 646FFC30… as a srcversion that must not be attributed to this directory — which is the same discipline I flagged for the release stick when #1088 pins the c26 medium to EE1F36A2B9BA294CE11AD80.
Nothing merged or pushed. I will re-review if the patch moves again; the hardware rows are the only thing between this and a READY.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: hold released on shape, not on proof — at ab82d1c1 the patch carries per-client exclusivity (xrs_req->owner = (uintptr_t)hwctx->client, partition_node.owner, share only within one owner), which is what the orchestrator's decision required and what my earlier review at 1881b73d blocked.
Reading this as the gate line for the record, since my previous comment on this head opened with the finding rather than the marker. The detail is in that comment: the per-client reshape is correct, the earlier per-context version's failure was measured by the lane (ctx #2 of one client refused, code 102, 3/3), and the two hardware rows the decision asks for — second client refused while the first finishes, plus one client's several contexts loading again — are still owed and not claimed, so this is not a merge on my authority.
Two things stand from the previous comment: WARN_ON_ONCE(!hwctx->client) beside the owner assignment would make the one case that could collapse two clients to owner 0 loud instead of silent, and 0004/0005 arriving alongside is fine but means the srcversion pins in README.md and pi-os3's rowproof-k74-zdrv.sh have to move together.
|
Persistent review updated to latest commit ab82d1c |
ab82d1c to
a3c6d5d
Compare
…ion 2026-11-05, engine #1085 item 2) With the per-client partition fix a second NPU client is refused at context creation, and the refusal carries the SAME signature a dead array answers: `code 102` / `-ENODEV` from `aie2_message.c`. The code alone cannot say which it is — only time can, because a client holding the array frees it when it exits and a dead array never does. So a refusal is now *queued* instead of failed on the first window: app/npu_load_retry.h LoadRetryPolicy gains the refusal schedule (45 s window, 2 s max delay, 60 attempts) beside the existing 2.5 s evict-race window; load_retry_delay/more take the schedule explicitly (the old one-argument forms remain, so existing callers and tests are untouched); open_with_retry classifies EACH failure — a refusal queues on the long window, anything else keeps the short one — the queued retry line says "queued: another NPU client may hold the array", and the give-up hint names both causes in the order to check them (another client first; then the driver reload). app/serve.cpp --load-queue-ms N (default 45000, 0 = refuse at once = pre-#1085 behaviour), parsed at serve level and forwarded into all three `run_unified` argv builders — serve constructs that argv itself, so a flag only unified understood would have been unreachable from `1bit serve`. app/unified.cpp the flag, its validation and help, and one LoadRetryPolicy carrying it into both NPU open sites (private route and fast lane). docs/npu.md "#### A refusal is a queue, not a fault" under "When a load follows an evict": why code 102 is ambiguous, what the budget does, and that it is the load-side counterpart of --queue-ms. tests/ four new cases in npu_load_retry_test: a refusal retries even when window_s = 0 (so the queue window is what acted), a never-clearing refusal stops at the queue cap and names the window and the other client, --load-queue-ms 0 is one attempt, and an ordinary failure still obeys the short policy's cap so a broken model cannot hang a load for the queue budget. Verified (no device, no ticket): ./build/npu_load_retry_test -> PASS (all 10 cases) 1bit serve -m <stub NPU dir> --help -> the --load-queue-ms lines 1bit serve -m <stub NPU dir> --load-queue-ms -5 -> "1bit serve: --load-queue-ms takes 0 (refuse at once) or more, not -5" (before any device work) 1bit serve -m <stub NPU dir> --load-queue-ms 1234 -> reaches open_with_retry; the fake-kernel error is NOT a refusal, so it keeps the short window ("retrying in 150 ms") — the classification, observed build: ONEBIT_NPU=ON -DONEBIT_NPU_RUNTIME=own -DONEBIT_NPU_RT_DIR=<npu_rt>, onebit target -> rc 0, 0 errors, 0 warnings tools/copyright.py --check -> rc 0 Out of scope on purpose: `1bit forward-serve` has no retry path at all (only Lemonade's unified serve does), and the flag is not wired into the appliance TUI. #1085 (the driver half) is still held, so this is the engine's half of the decision and its refusal path is exercised with stubs, not on a patched module.
|
Persistent review updated to latest commit a3c6d5d |
|
Preparing review... |
53f3817 to
8dd5e13
Compare
…ion 2026-11-05, engine #1085 item 2) (#1100) With the per-client partition fix a second NPU client is refused at context creation, and the refusal carries the SAME signature a dead array answers: `code 102` / `-ENODEV` from `aie2_message.c`. The code alone cannot say which it is — only time can, because a client holding the array frees it when it exits and a dead array never does. So a refusal is now *queued* instead of failed on the first window: app/npu_load_retry.h LoadRetryPolicy gains the refusal schedule (45 s window, 2 s max delay, 60 attempts) beside the existing 2.5 s evict-race window; load_retry_delay/more take the schedule explicitly (the old one-argument forms remain, so existing callers and tests are untouched); open_with_retry classifies EACH failure — a refusal queues on the long window, anything else keeps the short one — the queued retry line says "queued: another NPU client may hold the array", and the give-up hint names both causes in the order to check them (another client first; then the driver reload). app/serve.cpp --load-queue-ms N (default 45000, 0 = refuse at once = pre-#1085 behaviour), parsed at serve level and forwarded into all three `run_unified` argv builders — serve constructs that argv itself, so a flag only unified understood would have been unreachable from `1bit serve`. app/unified.cpp the flag, its validation and help, and one LoadRetryPolicy carrying it into both NPU open sites (private route and fast lane). docs/npu.md "#### A refusal is a queue, not a fault" under "When a load follows an evict": why code 102 is ambiguous, what the budget does, and that it is the load-side counterpart of --queue-ms. tests/ four new cases in npu_load_retry_test: a refusal retries even when window_s = 0 (so the queue window is what acted), a never-clearing refusal stops at the queue cap and names the window and the other client, --load-queue-ms 0 is one attempt, and an ordinary failure still obeys the short policy's cap so a broken model cannot hang a load for the queue budget. Verified (no device, no ticket): ./build/npu_load_retry_test -> PASS (all 10 cases) 1bit serve -m <stub NPU dir> --help -> the --load-queue-ms lines 1bit serve -m <stub NPU dir> --load-queue-ms -5 -> "1bit serve: --load-queue-ms takes 0 (refuse at once) or more, not -5" (before any device work) 1bit serve -m <stub NPU dir> --load-queue-ms 1234 -> reaches open_with_retry; the fake-kernel error is NOT a refusal, so it keeps the short window ("retrying in 150 ms") — the classification, observed build: ONEBIT_NPU=ON -DONEBIT_NPU_RUNTIME=own -DONEBIT_NPU_RT_DIR=<npu_rt>, onebit target -> rc 0, 0 errors, 0 warnings tools/copyright.py --check -> rc 0 Out of scope on purpose: `1bit forward-serve` has no retry path at all (only Lemonade's unified serve does), and the flag is not wired into the appliance TUI. #1085 (the driver half) is still held, so this is the engine's half of the decision and its refusal path is exercised with stubs, not on a patched module. Co-authored-by: agent <agent@local>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8dd5e135 — the hold is lifted, because the rows it was waiting for now exist and I checked one of them against its own artifact.
The code did not move; only the evidence did. All three patches are byte-identical to the versions I reviewed at ab82d1c1:
0003-accel-amdxdna-never-share-an-array-partition.patch same blob
0004-accel-amdxdna-two-tdr-recovery-outcomes-are-not-failures.patch same blob
0005-accel-amdxdna-check-the-throttle-before-touching-any-context.patch same blob
third_party/patches/amdxdna/README.md DIFF <- the new rows + evidence paths
So the per-client shape (xrs_req->owner = (uintptr_t)hwctx->client, partition_node.owner, share only within one owner) is exactly what I read, and what changed is that the README row now carries the measurements with paths and attribution.
I verified the primary row from the evidence, not the table — ~/lb/k74-ab/proof-0011-20261105-062629 exists and says what the row says:
$ cat r1.code -> 200 (the holder answered)
$ grep -o 'chunks=1099|code 102' probe.log
chunks=1099 (the holder's row completed, 1 099 chunks)
code 102 code 102 code 102 (the second client was refused)
and the multi-context dirs the row cites (multictx-20261105-062539/062616/063527/063540) are on disk too, so "one process, several contexts: 2 contexts PASS 2/2" is backed by a run rather than by a claim.
The strongest thing in this revision is that it states my reason back to me as a measurement: "A per-context version was measured first and refused the second context of one client's multi-context route (3/3) — that is why the unit is the client, not the context." That is precisely the failure I predicted from reading alloc_requests/allocate_partition, and it is now 3/3 on hardware rather than an argument about struct fields.
The classification is honest and I want it preserved on the merge commit: the wedge rows are still the pre-patch module's (they are the "why"), the capability answer remains "no two clients at once", and the patch converts a wedge into a clean refusal — not into co-residency. Anyone reading "NPU exclusivity shipped" as "two NPU clients now work" would be wrong, and the README says so.
Remaining nit, unchanged and not blocking: xrs_req->owner = (uintptr_t)hwctx->client is still bare, so a context with a NULL client silently becomes owner 0 and two such clients would be "the same owner". A WARN_ON_ONCE(!hwctx->client) there would make the only case that could defeat the scheme loud. I flagged it twice; the shape is right without it, so it is your call.
With this READY, the NPU-exclusivity pair is unblocked: #1085 (driver) now has its rows, #34 (product) has 4/4 on the slot-policy test, and #1100 (engine queue) is merged with its 10-case test passing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY on content at 8dd5e135 (unchanged from my last review — the patches are byte-identical and the hardware rows are in the README), but mergeStateStatus is DIRTY: it conflicts with main and cannot be merged as it stands. A rebase is needed first; the code is fine.
The owner's description is the one I verified: client-scoped exclusivity in 0011v2, xrs_req->owner = (uintptr_t)hwctx->client, partition_node.owner, sharing only within one owner — and the row that proves the point I raised (one process, several contexts: 2 contexts PASS 2/2, ZR1 per-op route holding 8 and answering 200) is in the README with its evidence paths, one of which I read myself (~/lb/k74-ab/proof-0011-20261105-062629: holder r1.code=200 at chunks=1099, second client code 102).
What has to happen before this merges: rebase onto current main. The likely conflict is docs/npu.md (both this PR and the merged #1100/#1102/#1090 work touch it) plus third_party/patches/amdxdna/README.md. Once rebased, the patches should hash the same — if they do, my verdict carries; if any patch content moves, I will re-read it rather than assume.
I am not rebasing it myself: the lane is active and this is a driver patch where a conflict resolution is a claim about kernel code, not a mechanical merge. Say the word if you want me to do it on a -rebased branch, and I will resolve and re-verify the same way (blob comparison plus the row evidence) rather than by eye.
One nit still open from my earlier reviews and not blocking: xrs_req->owner is assigned without a WARN_ON_ONCE(!hwctx->client), so a context with a NULL client silently becomes owner 0 and two such clients would look like one owner.
8dd5e13 to
7f5113a
Compare
…didate, #1084) Every amdxdna partition node is created with `exclusive = false` ("always set exclusive to false for now"), so when no free column range is left allocate_partition() hands a second client the *same* columns as a live one. Each design's CDO asks for all 8 columns, so two clients that have each dispatched once always share one partition - and configuring the second stops the array for every client (614, DPU PC 0xffffffff). Measured on strixhalo 2026-11-05, shipped c24 payload under npu-hold: two clients that each dispatched once sat in "Partition Index 0, Columns [0..7]" and the next dispatch wedged 3/3 sequential and 3/3 concurrent, while one context was clean in every row (1000-3408 positions) and a second serve that never dispatched was clean too. Full chain in #1084. The first version of this patch made the *context* exclusive, and pi-onebp measured it refusing the second context of a single client's multi-context route (ZR1 per-op forward: 4 designs = 4 contexts, "code 102" on ctx #2, 3/3) - a route that ran 67 200 submits with 0 TDR on the shared partitions. So the unit is the client: alloc_requests.owner carries the owner (hwctx->client, one per open file), partition_node.owner records it, and allocate_partition() only shares within one owner. A distinct column range is still handed out whenever one is free, so designs using fewer columns than the array keep running concurrently; only the case that wedges the array becomes a clean -ENODEV at context creation. Verified `git apply --check` clean on the pin alone and after 0001+0002; a build of pin + 0001 + 0002 + 0003 is rc 0 with no warnings and srcversion FA680E5F8334570CA318449 (recorded in the README's fingerprint table), and the patched sources are byte-identical to the tree that was built. It is a candidate: no module carrying it has run on hardware in this shape (the per-context version was the one installed at 05:49 and rolled back from), and the README says so together with the rows that would prove it. Also state what this directory still does not carry - the newer TDR series (0008/0009) the box and the c25 candidate run - so nobody reads "pin + the series" as the module the fleet is running. closes #1084
…e run (0004, 0005)
The directory claimed to carry "the patches this fleet's builds run", but it
stopped at 0002: the recovery pair that every build since 2026-11-04 23:2x has
used was only in scratch trees. Add it as 0004 and 0005, and mark what the
directory deliberately leaves out (the 0010 debugfs test hook, per the fleet
decision of 2026-11-05 00:49).
0004 (the kernel lane's 0008): 0002's recovery counted two non-failures as
failures and either latched hw_dead - a throttled power-cycle (-EBUSY inside
AIE2_HW_RESET_MIN_INTERVAL_MS) and a closing context (-ENODEV from
aie2_hwctx_restart() for a context whose heap address is gone, now -ESRCH and
counted in rarg.skipped).
0005 (patch 0009): 0004 stops the throttle being counted at the reset call but
still ran both context walks, stopping every context and restarting it against
an array that was never power-cycled - which sets rarg.failed and latches
hw_dead through the other path. Check the throttle first and touch nothing.
Both are reproduced from the sources the shipped module was built from, and
verified by rebuilding, which is stronger than "the patch applies":
pin + 0001 + 0002 + 0004 -> 6483D31718A008B3602114E (the module
installed at 23:2x on 2026-11-04)
pin + 0001 + 0002 + 0004 + 0005 -> BBE1E5F0EE7A5FB235CA811 (the
0007+0008+0009 stack, and every .c/.h
cmp-identical to that tree)
... + 0003 (partition candidate) -> EE1F36A2B9BA294CE11AD80
All four rows are in the README's fingerprint table with the two srcversions
that must NOT be attributed to this directory (646FFC30... = the 0010 hook,
169C46E2... = the per-context partition patch that was rolled back).
… is the partition The "Two streams on one NPU" section said the exclusive lock was "a policy, not a hardware limit" because the driver advertises 16 hardware contexts and only refuses the 17th. That is measuring the wrong thing: hwctx_limit bounds the contexts of one client, and the limit on a second *client* is the array partition. The array has 8 columns and the kernel set this engine ships asks for all 8 (measured: one partition, Columns [0..7]), so one client's contexts share the whole array and a second client has nothing to be given. Before 2026-11-05 the solver handed it the same columns anyway, and its configuration took the array out from under the first client: 3/3 wedges with overlapping requests and 3/3 with strictly sequential ones, and never with a single context (#1084). With 0003 in this tree the second client is refused with code 102 / -ENODEV instead, and multi-context within one process is unchanged. The same claim is repeated in the two places a reader hits before the docs -- app/npu_device_lock.h's policy comment and forward_serve.cpp's comment at the lock site -- so those are corrected too, not just docs/npu.md. Consequence for the rest of the section, stated where the reader meets it: --npu-streams N needs a kernel set that asks for fewer columns than the array has. The 0.41x co-residency row was measured on the LFM2 m16 set, not on the 8-column sets this engine ships, and the partitioning paragraph now says that a 4-column kernel set is the prerequisite for those numbers rather than a tuning option. Comment-only in the two sources: no behaviour change, no rebuild needed.
…date" framing
When the patch and its README row were written, no build carrying it had run on
hardware, so both said "candidate" and named the rows that would settle it.
Those rows ran on 2026-11-05 between 06:26 and 06:50, on the kernel lane's build
of the same change (0011v2 in srcversion 349FDC99CADC56900E0249A), and two lanes
measured the two halves:
second client refused (code 102), holder kept its 1 099-chunk row
refusal is not a lockout (fresh client answers once the holder exits)
one process with 2 contexts PASS 2/2, and the ZR1 per-op route held 8 contexts
and answered 200 (the route the per-context version broke)
single-client throughput unchanged (5/5 rows, median 82.37 tok/s), 0 TDRs
libamdf CTS on the same build: enumerate rc 0, XdnaMemoryTest 4/4,
XdnaLinuxMemoryTest 4/4, amdxdna ERROR count unchanged
So the README row says that, with the evidence path per row and whose ticket it
came from, and docs/npu.md's two-stream section cites the measured build instead
of describing the patch as not-yet-run. The provenance split is kept explicit
because it matters for anyone rebuilding: 349FDC99... carries the 0010 debugfs
hook and cannot be produced from this directory, while the repo expression of
the same change builds to FA680E5F... (a fingerprint, not the measured binary).
Two things deliberately not upgraded: the wedge rows are still the pre-patch
module's, and the capability answer stays "no two clients at once" — the patch
turns a wedge into a clean refusal, it does not add support.
7f5113a to
bf849a0
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY on content, still cannot merge: mergeable=CONFLICTING, DIRTY at 7f5113a9. The rebase happened but it did not resolve the conflict, so the owner's "merge all ready" cannot include this one yet.
Content is unchanged where it counts — I blob-checked the driver patch:
third_party/patches/amdxdna/0003-…-never-share-an-array-partition.patch same blob as 8dd5e135
third_party/patches/amdxdna/README.md DIFF (documentation only: 0003 is now
described as measured as 0011v2 on the
kernel lane's tree, srcversion 349FDC99…, not as
the form in this directory)
So the client-scoped 0011v2 shape and the hardware rows are exactly what I passed; the README change is the honest fingerprint
caveat, which is the same distinction I have been asking for elsewhere (a srcversion is a property of the built module, not
of the file next to it).
What is needed: a rebase onto current main that actually lands clean. My guess at the conflict is docs/npu.md and
third_party/patches/amdxdna/README.md — several PRs merged through both since this branch was cut (#1100's --load-queue-ms
docs, #1111/#1113's edits, the pin bumps). If the lane wants, I will take it on a -rebased branch and re-verify by blob plus
the row evidence rather than by eye; but a driver conflict resolution is a claim about kernel code, so I would rather the owner
of the row do it.
Standing nit from earlier reviews, still not blocking: xrs_req->owner = (uintptr_t)hwctx->client has no
WARN_ON_ONCE(!hwctx->client), so a context with a NULL client silently becomes owner 0 and two such clients would look like
one owner.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bf849a01 — the merge blocker is gone: mergeable=MERGEABLE, BEHIND only, and all three patches are byte-identical to the ones I reviewed.
third_party/patches/amdxdna/0003-…-never-share-an-array-partition.patch same blob as 7f5113a9
third_party/patches/amdxdna/0004-…-two-tdr-recovery-outcomes-are-not-failures.patch same
third_party/patches/amdxdna/0005-…-check-the-throttle-before-touching-any-context.patch same
mergeable=MERGEABLE mergeStateStatus=BEHIND
So the conflict is resolved without touching the driver patches — which is exactly what I wanted from a conflict resolution here: the shape (0011v2, per-client owner) and the rows are the ones I verified, and what changed is only where the branch sits.
The review behind this verdict, unchanged: client-scoped exclusivity with xrs_req->owner = (uintptr_t)hwctx->client and sharing only within one owner; the hardware rows present and one of them verified from its own artifact (~/lb/k74-ab/proof-0011-20261105-062629: holder code 200 at chunks=1099, second client code 102); the honest classification that the patch converts a wedge into a clean refusal, not into co-residency.
BEHIND means it needs a refresh against a main that now carries the seven merges from this hour — a rebase, not a fix. If that refresh moves any of the three patches, I will re-read it rather than assume; if their blobs stay the same, the verdict carries.
Nit still open and not blocking: xrs_req->owner has no WARN_ON_ONCE(!hwctx->client), so a NULL-client context becomes owner 0 and two such clients would look like one owner.
|
Persistent review updated to latest commit 4a8d9fd |
Five commits, one file tree: the amdxdna patch directory now carries what the fleet actually runs, and the partition fix is
in the shape the orchestrator decided (per client).
Commit 1 —
762aff4: partition exclusivity, client-scoped (#1084) — measured on hardware, see commit 4aie2_solver.ccreates every partition node withexclusive = false("Always set exclusive to false for now"), so whenget_free_partition()finds no free column range,allocate_partition()shares a live partition (nshared++). Everydesign's CDO asks for all 8 columns, so a second client lands on the same tiles as the first — and configuring it stops
the array for every client (
614 state 8,DPU PC 0xffffffff).npu-hold)614 run 0 of 28at chunk 2), 5 TDRshwctx_num0 until the first dispatch)xrt-smi examine -r aie-partitions, both warmedPartition Index 0, Columns [0..7]The unit is the client, not the context. The first version of this patch marked the context exclusive; pi-onebp
measured it refusing ctx #2 of a single client's multi-context route (ZR1 per-op forward, 4 designs) —
code 102, 3/3 — ona route that ran 67 200 submits with 0 TDR on shared partitions. So:
alloc_requests.owner(=hwctx->client, one peropen file) →
partition_node.owner, andallocate_partition()shares only within one owner.get_free_partition()isuntouched, so a distinct range is still handed out when one is free.
Owed on hardware (kernel lane's rows): two clients → second refused with
code 102, first finishes, partition frees onexit; plus one client's several contexts loading again. Neither is claimed here; the README marks the row a candidate.
Commit 2 —
e57a950: carry the TDR recovery pair the box and the c25 candidate runThe directory said it carried "the patches our builds run" but stopped at
0002; the recovery pair every build since2026-11-04 23:2x uses lived only in scratch trees. Added as
0004(the kernel lane's0008: a throttled power-cycle and aclosing context must not latch
hw_dead) and0005(patch0009: check the throttle before touching any context, or thewalks still latch
hw_deadthroughrarg.failed). The0010debugfs test hook is deliberately not carried (fleetdecision, 2026-11-05 00:49), and the README says so.
Commit 3 —
b11c1d2: the second-NPU-stream story stops using the hwctx count as its limitThe "Two streams on one NPU" section called the exclusive lock "a policy, not a hardware limit" because the driver
advertises 16 hwctx and "only refuses the 17th". That measures the wrong thing:
hwctx_limitbounds the contexts of oneclient, and the limit on a second client is the array partition — 8 columns, and the shipped kernel set asks for all 8.
So the section now says what the device does before the fix (second client shares the tiles and takes the array down,
3/3 + 3/3 measured, #1084) and after it (
code 102/-ENODEV), states that--npu-streams Nneeds a kernel set askingfor fewer columns, and labels the 0.41x co-residency row as measured on the LFM2
m16set rather than on the 8-columnsets this engine ships. The same claim is repeated in the two comments a reader hits before the docs —
app/npu_device_lock.h's policy block andforward_serve.cpp's comment at the lock site — so both are corrected(comment-only, no behaviour change).
Commit 4 —
bf849a0: the rows exist, so the patch is no longer described as a candidateCommits 1 and 3 were written when no build carrying the patch had run on hardware. Between 06:26 and 06:50 on 2026-11-05 it
did — as
0011v2in the kernel lane's build349FDC99CADC56900E0249A— and two lanes measured the two halves:code 102while the holder finished 1 099 chunksenumeraterc 0,XdnaMemoryTest4/4,XdnaLinuxMemoryTest4/4The README row now carries those with their evidence paths and attribution, and
docs/npu.mdcites the measured buildinstead of a patch that had not run. Provenance closed by building it: applying
0003to the kernel lane's tree(0007+0008+0009+0010) instead of the scratch
0011v2delta rebuilds to exactly349FDC99CADC56900E0249A— the module therows above were measured on — so the file in this PR is byte-equivalent to the one that was proven, not merely the same idea.
On the repo's own series it builds to
FA680E5F…(different base, not a different change). Provenance is kept explicit for rebuilders:349FDC99…carries the 0010 debugfs hookand cannot be produced from this directory, while the repo expression of the same change builds to
FA680E5F…— afingerprint, not the measured binary. Not upgraded: the wedge rows are still the pre-patch module's, and the capability
answer remains "no two clients at once" — the patch converts a wedge into a clean refusal.
Dropped on rebase: the old commit 4 (the refused-context hint)
Main implemented the same problem better while this PR sat:
#1100("a refused context is queued, not failed",--load-queue-ms) makes the refusal a queue with its own budget and gives the hint the holder-first order("another client may hold the array (wait for it, or stop it); if nothing else is serving, reload the driver"). That
supersedes my hint-only change, so the rebase skipped it rather than leave two competing texts and a duplicated test
assertion. Everything else in the PR is unchanged;
app/npu_load_retry.hnow carries main's code verbatim.One thing I noticed on main and did not touch: its new "A refusal is a queue, not a fault" section still shows the old
sample line (
the NPU array is refusing hardware contexts: reload the driver (...)), while the code now prints theholder-first text. Cosmetic, but it is a doc/code mismatch in a section a user reads before diagnosing — flagging it here
for whoever owns
app/npu_load_retry.hrather than expanding this PR.Verification — rebuilt, not inferred
pin + 0001 + 0002 + 00046483D31718A008B3602114Epin + 0001 + 0002 + 0004 + 0005BBE1E5F0EE7A5FB235CA8110007+0008+0009stack; every.c/.hcmp-identical to that treepin + 0001 + 0002 + 0003FA680E5F8334570CA318449EE1F36A2B9BA294CE11AD80Every patch
git apply --checks clean in order from the pin alone (0003also applies on the pin without 0001/0002), bothkbuilds are
rc 0with no warnings under~/lb/thermal-run.sh, andtools/copyright.py --checkis clean. The READMEfingerprint table also names the two srcversions that must not be attributed to this directory (
646FFC30…= with the0010 hook,
169C46E2…= the per-context partition patch that was rolled back).closes #1084