Skip to content

hw-gate: the npu phase takes the fleet's NPU queue, not just ~/npu.lock - #822

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/hw-gate-npu-hold
Nov 2, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/hw-gate-npu-hold

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

The incident this comes from

On 2026-11-02 the same npu phase passed at 08:58 (oracle/greedy64/prefill/reproducible all PASS, 65.49 tok/s, 7 s) and then FAILed at 09:48 on identical inputs — same build dir, same dx_decode_test (fba773e6…), same model files (model.q4nx 6abacb42…), same sealed set, same kernel, same amdxdna.ko, same npu_7.sbin firmware. Two firmware TDRs fired inside the failing run:

09:48:51 aie2_legacy_tdr_detect: TDR timeout detected   (Context 8, Context 6)
09:49:37 aie2_legacy_tdr_detect: TDR timeout detected   (Context 8)
npu  FAIL  88s  oracle=FAIL greedy64=PASS prefill=FAIL reproducible=PASS tok/s=84.67   (ref1/ref2 wrote no logits)

Cause

The npu phase was the only phase that serialised on ~/npu.lock — a private flock that only the gates which know about it take. The rest of the box serialises NPU work through npu-hold (~/NPU-QUEUE + ~/NPU-TOKEN), with npu-serve-keeper stopping the owner's serve whenever a ticket is waiting.

So during that run another lane held ~/NPU-TOKEN (mtp live k4 t0 stream, 09:46:00 → killed at its 3-minute max hold 09:49:01) while the gate's npu phase drove the same array. Two AIE2 contexts were live at once; the two that TDR'd match the two sub-runs whose output files never appeared.

The fix (one file, scripts/hw-gate.sh)

The npu phase now runs the checker under npu-hold, so it queues with every other NPU user and the keeper pauses the serve for it. ~/npu.lock stays as the inner guard. The cost is one FIFO wait — which is the whole point.

  • No self-deadlock. A chain already wrapped in one npu-hold (a lane running npu-hold -- gate.sh ops kat kld … npu) must not queue behind itself: that is what wedged c18 for 51 minutes on the GPU queue. The claim is detected by finding the token holder's pid among this process's ancestors, and HW_GATE_NPU_HOLD=0 opts out explicitly (mirrors the existing HW_GATE_GPU_HOLD=0).
  • HW_GATE_NPU_TOKEN, HW_GATE_NPU_HOLD_BIN, HW_GATE_NPU_HOLD_MINUTES exist for a box with a different queue layout (and for testing).
  • Nothing else changes: same checker, same env, same parsing, same floor, same ~/npu.lock, same exit codes.

Verified — stubbed checker and stubbed hold, no NPU run and no NPU token involved

case result
outside any claim wrapped: npu-hold -l hw-gate-npu -t 20 -w 5400 -- flock -w 5400 -E 75 <lock> sh -c <checker>; row PASS … tok/s=88.10
already inside one npu-hold (token names an ancestor pid) zero wrapper calls; row PASS
HW_GATE_NPU_HOLD=0 zero wrapper calls; row PASS
failing checker row FAIL, exit 1, oracle=FAIL greedy64=PASS prefill=FAIL reproducible=PASS tok/s=84.67 — the same shape as the real 09:48 row

bash -n clean, tools/copyright.py --check rc=0, --help still answers without claiming anything.

Still open

A single settle run of this unit with the fix, on a cool die with the queue genuinely idle, is approved by the orchestrator and will be reported (the four sub-checks, tok/s, and the amdxdna error counter before/after) separately. Until it lands, the c20 NPU row rests on the 08:58 measurement.

2026-11-02: an npu phase passed at 08:58 (65.49 tok/s, exclusive) and then FAILed at 09:48 on identical inputs - same
binary, model, sealed set, kernel, driver and firmware - with two firmware TDRs (aie2_legacy_tdr_detect, Context 6 and 8)
inside the measurement and oracle/prefill producing no output at all.

Cause: the npu phase was the only phase that serialised on ~/npu.lock, a private flock that only gates which know about it
take. The fleet serialises NPU work through npu-hold (~/NPU-QUEUE + ~/NPU-TOKEN, with npu-serve-keeper stopping the owner's
serve while a ticket waits). So at 09:46-09:49 another lane held ~/NPU-TOKEN and ran a live NPU job while the gate's npu
phase drove the same array: two AIE2 contexts live at once, one TDR per context, and the other lane's job was killed at its
own max hold 10 s later.

Fix: the npu phase now runs the checker under npu-hold (~/npu.lock kept as the inner guard), so it queues with everyone
else and the keeper pauses the serve for it. Cost is a FIFO wait, which is the point.

Self-deadlock avoided: a chain already wrapped in one npu-hold (a lane running "npu-hold -- gate.sh ops kat kld ... npu")
must not queue behind itself - that is what wedged c18 for 51 min on the GPU queue. The claim is detected by looking for
the token holder's pid among this process's ancestors, and HW_GATE_NPU_HOLD=0 opts out explicitly (mirrors
HW_GATE_GPU_HOLD=0). HW_GATE_NPU_TOKEN / HW_GATE_NPU_HOLD_BIN / HW_GATE_NPU_HOLD_MINUTES exist for a box with a different
queue layout and for testing.

Verified with a stubbed checker and a stubbed hold, so no NPU run and no NPU token was involved: (a) outside a claim the
command is wrapped in 'npu-hold -l hw-gate-npu -t 20 -w 5400 -- flock ...'; (b) with the token naming an ancestor, zero
wrapper calls; (c) HW_GATE_NPU_HOLD=0, zero wrapper calls; (d) a failing checker still FAILs the row (exit 1) and prints the
same row shape as the 09:48 failure (oracle=FAIL greedy64=PASS prefill=FAIL reproducible=PASS tok/s=84.67).
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 1b97217)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Potential Deadlock in NPU Token Handling

The token_held_by_ancestor function attempts to detect if the NPU token is already held by a parent process. However, the logic for traversing the process tree may not be robust enough to handle all edge cases, potentially leading to a deadlock if the process hierarchy is not correctly interpreted. This could cause the gate to hang indefinitely while waiting for the NPU token.

token_held_by_ancestor() {
    local pid p
    pid=$(sed -n 's/.*pid \([0-9][0-9]*\).*/\1/p' "$1" 2> /dev/null | tail -n 1)
    [ -n "$pid" ] || return 1
    p=$$
    while [ "${p:-0}" -gt 1 ] 2> /dev/null; do
        [ "$p" = "$pid" ] && return 0
        p=$(sed 's/.*) //' "/proc/$p/stat" 2> /dev/null | awk '{print $2}')
        [ -n "${p:-}" ] || return 1
    done
    return 1
}
Incorrect NPU Hold Logic

The NPU phase now uses npu-hold for fair queuing, but the implementation may not correctly prevent overlapping NPU dispatches. Specifically, if HW_GATE_NPU_HOLD is set to 0, it still attempts to use flock on ~/npu.lock, but does not ensure that the token is not held by an ancestor process. This could lead to the same race condition that caused the TDRs in the incident.

hold_args=()
if [ "${HW_GATE_NPU_HOLD:-1}" != 0 ] && [ -n "$NPU_HOLD" ] && ! token_held_by_ancestor "$NPU_TOKEN"; then
    hold_args=("$NPU_HOLD" -l hw-gate-npu -t "${HW_GATE_NPU_HOLD_MINUTES:-20}" -w "$LOCK_WAIT" --)
fi
"${hold_args[@]}" flock -w "$LOCK_WAIT" -E 75 "$LOCK" sh -c "$HW_GATE_NPU_CHECK" > "$log" 2>&1

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 1b97217

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

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: READY at 1b97217bf (tprs-ready applied)

Reproduced every case in the body with a stub npu-hold and a stub checker — no NPU run, no NPU token taken, the
flock pointed at a /tmp lock file, --phases npu only. All five behaviours hold at this head:

case what the run shows
outside any claim wrapper taken, argv exactly -l hw-gate-npu -t 20 -w 5400 -- flock -w 5400 -E 75 <lock> sh -c <checker>; row PASS … tok/s=88.10
token names an ancestor (wrapper whose pid went into the token, then exec bash hw-gate.sh) 0 wrapper calls, row PASS — no self-deadlock
HW_GATE_NPU_HOLD=0 0 wrapper calls, row PASS
token held by a non-ancestor in the keeper's own format (npu-serve-keeper pid 15134 — the live serve) wrapper taken — this is the 09:48 shape, and it is the case that had to work
failing checker row npu FAIL … oracle=FAIL greedy64=PASS prefill=FAIL reproducible=PASS tok/s=84.67, gate exit 1, i.e. the same row shape as the incident
--help rc 0, 0 wrapper calls, no ~/GPU-TOKEN left behind

Mechanism read, and it is the right one: the claim is outside the flock, so the order is npu-token → ~/npu.lock,
which is the nesting that cannot deadlock against the lanes that hold only the flock; hold_args=() expands to zero
words, so the unwrapped path is byte-for-byte the old command line; and token_held_by_ancestor parses the pid out of
both token formats (gpu-hold <label> pid <pid> <ts> and npu-serve-keeper pid <pid>) and walks /proc/<p>/stat
ppid upwards from $$. -t 20 is minutes and -w 5400 seconds in gpu-hold's own argument handling (t) MINUTES,
w) WAIT), which npu-hold forwards.

bash -n rc 0, tools/copyright.py --check rc 0, CI on this head: build, CodeQL, four Analyze, pr_agent
success (330 s), hw_gate skipped.

One nit, and it is the same class as the bug this PR fixes: if npu-hold is absent (NPU_HOLD empty — a box that
does not have it, or ~/.local/bin pruned), the phase silently falls back to ~/npu.lock alone and logs nothing. That
is exactly the configuration that produced the two TDRs, and the gate would run in it without a word. One line —
[ -n "$NPU_HOLD" ] || printf 'npu: no npu-hold on this box; taking ~/npu.lock only\n' >> "$log" before the
hold_args decision — makes the state visible instead of silent.

Not verified by me: the settle run itself (real checker, cool die, idle queue). That is correctly listed as open in the
body, and my evidence deliberately does not touch the array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant