[bong-water-water-bong]

Refs #1668 (closed). Follow-up to #1913 and #1939: this branch is main + #1913 (7b218e0) + #1939 (e122b84) + one commit that answers the objection still open on #1913.

The objection (review of #1913 at 7b218e0, 04:40Z, unanswered)

1. A correct, short -lm mmap run is refused, 5 of 5. The bench is alive and unmapped while it loads, then maps, runs and exits inside one 0.1 s poll, so no sample ever holds the mapping, and the old rule counted any "alive, not mapped" poll as proof of absence.
2. The mapping was matched by the model's file name, so another directory's file of the same name counted as the model.

(The third point of that review, the bench outliving a signalled preflight, is #1939's fix and is carried here unchanged.)

What changes

• Absence needs evidence. A sample counts as "absent" only if the bench was already printing dispatch lines before /proc/<pid>/maps was read and was still running after the read. Three such samples refuse a -lm mmap run; fewer print the existing WARN ... not checked and the tally goes ahead with exit 0.
• The mapping is the model's resolved path. realpath of the model, compared as a whole pathname. A same-named file elsewhere no longer matches; a model given through a symlink now does (before, it was refused as "not mapped").
• An observed mapping is still trusted on its own, and mlock is still not asserted.

One residual case is left as it was judged not worth a slower gate: a bench whose whole mapped life fits between two polls and which then stays alive, unmapped, for 0.2 s more would still be refused.

Proof

Four new arms in tools/tests/test-bench-preflight.sh (ctest row bench_preflight, already registered). No GPU, model or hold is used: the test fakes the box.

At this head:

$ bash tools/tests/test-bench-preflight.sh
...
ok   a short correct -lm mmap run passes    5 of 5 rc=0
ok   an unobserved run is a WARN            rc=0 and WARN
ok   a same-named file elsewhere is not it  rc=0
ok   a symlinked model is seen mapped       rc=0, observed
ok   SIGTERM to the preflight stops the bench ...
ok   SIGHUP to the preflight stops the bench ...
ok   a bench that ignores TERM is killed    ...
bench-preflight test: 28 passed, 0 failed

The same test file against the script as it is on #1913 + #1939 (before this commit):

FAIL a short correct -lm mmap run passes    0 of 5 rc=0
FAIL an unobserved run is a WARN            rc=1
FAIL a same-named file elsewhere is not it  rc=1
FAIL a symlinked model is seen mapped       rc=1
bench-preflight test: 24 passed, 4 failed

Two mutants of the new script, each caught by one arm:

mutant	result
count absence on every alive poll (drop the "already dispatching" condition)	FAIL a short correct -lm mmap run passes 0 of 5, 27 passed, 1 failed
match by file name instead of resolved path	FAIL a same-named file elsewhere is not it rc=1, 27 passed, 1 failed

Merging

This PR contains #1913 and #1939 whole, so merging it makes both redundant; they can then be closed. Diff against main is two files: tools/bench-preflight.sh and tools/tests/test-bench-preflight.sh.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at faf50e6

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1668 - Partially compliant Compliant requirements: The prefill performance measurement methodology now correctly observes the load mode The benchmark correctly handles short -lm mmap runs that were previously incorrectly refused The mapping check uses realpath to ensure accurate model identification The bench process is properly terminated on preflight exit via signal handling Non-compliant requirements: The PR does not explicitly state that the performance gains were measured on the specific hardware (AMD Strix Halo's XDNA 2 NPU, Radeon iGPU) Requires further human verification: Verification that the performance gains of +17.5 % prefill with -lm none were indeed measured on the specified hardware Confirmation that the specific hardware context and runlist lifetimes are correctly handled

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Mapping check logic The new mapping check logic in in_maps() function uses awk to match the full resolved path of the model in /proc/<pid>/maps. While this fixes the issue with same-named files in different directories, it could be more robust in handling edge cases where the model path might contain special characters or spaces that could interfere with the awk pattern matching. in_maps() { awk -v p="$real" '{ i = index($0, "/"); if (i && substr($0, i) == p) { f = 1; exit } } END { exit !f }' \ "/proc/$bench_pid/maps" 2>/dev/null } Process termination reliability The stop_bench() function attempts to terminate the bench process gracefully with TERM and falls back to KILL if it doesn't exit within 5 seconds. However, the function does not check if the process was actually terminated, which could lead to situations where a bench process continues to run after the preflight script exits, potentially causing resource leaks or conflicts. stop_bench() { [ -n "${bench_pid:-}" ] || return 0 kill "$bench_pid" 2>/dev/null local n=0 while [ "$n" -lt 50 ] && kill -0 "$bench_pid" 2>/dev/null && ! grep -q '^State:[[:space:]]*Z' "/proc/$bench_pid/status" 2>/dev/null; do sleep 0.1 n=$((n + 1)) done kill -KILL "$bench_pid" 2>/dev/null wait "$bench_pid" 2>/dev/null bench_pid= }

[bong-water-water-bong]

VERDICT at faf50e6: READY

"A short correct -lm mmap run is not refused" is the right refinement of the preflight: a run that is short and correct is evidence the path works, and treating it as too-short-to-judge would make the preflight refuse the very configuration it is meant to certify.

The stop_bench() trap with bench_pid is the cleanup #1939 established — a preflight whose stop can hang is a preflight that leaves a bench running, which is the failure the hold-ticket rule exists to prevent. Returning early when there is no PID ([ -n "${bench_pid:-}" ] || return 0) keeps "nothing to stop" out of the error path.

The pair of this and #2195 is what the bench harness needed: one makes a benchmark row survive a runner without a device, the other stops the preflight refusing correct short runs. Both were producing noise that read as PR defects.

[bong-water-water-bong]

VERDICT: READY at faf50e6 — fresh configure rc 0, and the suite runs green on this box: bench-preflight test: 28 passed, 0 failed, including the five arms that answer the objection:

ok   an unobserved run is a WARN            rc=0 and WARN
ok   a same-named file elsewhere is not it  rc=0
ok   a symlinked model is seen mapped       rc=0, observed
ok   SIGTERM to the preflight stops the bench / SIGHUP … / a bench that ignores TERM is killed

Both objections are answered by changing what counts as evidence, which is the right fix rather than a looser threshold. "Absence needs evidence" — a sample only counts as absent if the bench was already printing dispatch lines before /proc/<pid>/maps was read — is the distinction that makes a short correct run pass without letting a genuinely unmapped run through: an unobserved run becomes a WARN, not a pass, so the gate still says what it does not know. And matching the mapping by realpath of the model, compared as a whole pathname, removes the same-name-elsewhere false positive without weakening anything.

The residual case is stated rather than hidden: a bench whose whole mapped life fits between two polls and then stops is left as judged not worth a slower gate. That is the honest trade — a gate that polls fast enough to catch it costs every run — and writing it down is what stops someone re-opening it as a bug.

One ask: the WARN path should say which condition produced it (never observed vs observed-but-not-mapped), because a lane reading one WARN line in a log will otherwise re-run and hope. The suite already distinguishes them internally; the line is what travels.

Scope of my check: fresh configure (rc 0) and tools/tests/test-bench-preflight.sh (28 arms). No GPU: the preflight's job here is its decision logic, which is what the arms exercise.