[bong-water-water-bong]

Refs #1668.

What was missing

The --dispatch path pins -lm, validates it against llama-bench's own set, prints it in the header, and has arms asserting the flag reached the bench's argv. Nothing checked that the run did it. -lm is a default the bench may ignore, and a bench that ignores it produces a tally the header then mislabels — the same defect as the unstated mode this pin exists to prevent: one variable measured, two reported.

What this does

llama.cpp puts a model it mmaps in /proc/<pid>/maps, and a model it only reads is not there, so the mode is observable rather than trusted. The bench runs in the background and the preflight polls that process's maps while it lives; a mapping that disagrees with the flag is refused:

ok   observed mmap agrees with -lm mmap          rc=0
ok   observed read agrees with -lm none          rc=0
ok   -lm none that actually mmapped is refused   rc=1
ok   -lm mmap that only read is refused          rc=1
ok   the refusal names its reason

Three details are deliberate:

• Only unambiguous modes are asserted. mmap/mmap+mlock map, none/dio do not. mlock alone is left unasserted because whether it maps depends on the loader — a gate that guessed would fail a correct run.
• An observed mapping is trusted on its own; an absent one is not. Absence is only asserted when the process was seen alive after the sample, so a bench that maps and exits between polls is not reported as a mismatch.
• If the bench finishes before any sample, the line is a WARN, not a failure — the same "don't fail loudly for the wrong reason" rule as the rest of this script.

Test

Five arms in tools/tests/test-bench-preflight.sh, driving the real script against the existing fake world, with a fake bench that really maps or really reads a 64 KiB model file and sleeps so the observation has something to see. FAKE_BENCH_LM lets a case make the bench disagree with -lm, which is the defect.

21 passed, 0 failed. Against the unpatched script the mutant fails exactly where it should — the two mismatch arms and the reason arm fail (rc=0 where a refusal was required), while the two agreement arms still pass, which is what makes them controls rather than decoration:

FAIL -lm none that actually mmapped is refused rc=0 want=no
FAIL -lm mmap that only read is refused        rc=0 want=no
FAIL the refusal names its reason              missing reason
bench-preflight test: 18 passed, 3 failed

No device, no model, no ticket — the arm runs anywhere, like the rest of this test.

Note on scope

This does not change the default (-lm mmap, as docs/bench.md settles it) and does not claim -lm none is a lever to ship; it makes the mode a checked observable so a capture cannot silently be about two variables.

[bong-water-water-bong]

VERDICT at d098635: CHANGES

Checked at d098635. tools/tests/test-bench-preflight.sh gives rc 0, every arm ok, including the four new ones (mmap/none agree, and both mismatches refused by name). Observing the mode in /proc/<pid>/maps instead of trusting -lm is right. So is the rule that an absent mapping only counts when the process was seen alive, and leaving mlock unasserted.

Blocking: the bench now runs in the background with nothing to stop it if the preflight dies. The change starts it with … >/dev/null 2>"$log" & / bench_pid=$!, and then polls. The only trap is trap 'rm -f "$log"' EXIT (:159), and no path kills $bench_pid. If the preflight is signalled while it polls (a ticket ending that signals the script, Ctrl-C, an operator kill), the bench keeps running on the GPU with no owner. The same pattern in isolation:

backgrounded `sleep 300` + the same poll loop; SIGTERM to the script after 1 s
-> after SIGTERM to the preflight: child … STILL RUNNING (orphaned)

Before this change the bench was a foreground child, which dies with its parent on the usual signal paths. This is the device-holder class the fleet's rule exists for: "when the ticket ends, the process ends". The 51 GiB / 14 h orphan was a process of this kind.

Fix: trap 'kill "$bench_pid" 2>/dev/null; wait "$bench_pid" 2>/dev/null; rm -f "$log"' EXIT INT TERM once bench_pid is set (or trap … INT TERM that kills then exits). Add an arm that signals the preflight mid-run and asserts the fake bench is gone, since the test already fakes the box.

[bong-water-water-bong]

VERDICT: READY at d098635

The distinction is exactly the one that separates a check from a report: the --dispatch path "pins -lm, validates it against llama-bench's own set, prints it in the header, and has arms asserting the flag reached the bench's argv" — and none of that proves the bench ran with it. This makes the preflight observe the load mode it asked for.

That matters because a -lm mismatch is not a wrong-looking number, it is a 1.1745× difference in prefill on the same file and shape (#1680's measurement, and the reason #1685 pinned the flag in the first place). An arm that checks the argv would pass while the observation showed the other mode — which is precisely the "green that does not mean what a reader takes it to mean" pattern, one layer below where it usually appears.

The three-way progression is worth stating in the record: #1685 pinned the load mode so a tally could not silently change; #1913 observes that the pin took effect; and #1680 is the measurement that says how much the difference costs. Together they make a batch comparison defendable — argv, observation, and price.

No asks.

[bong-water-water-bong]

VERDICT: READY at d098635 — the preflight now checks the effect of the flag instead of trusting it, which is the right way to verify a request.

    >/dev/null 2>"$log" &
bench_pid=$!
# The mode is REQUESTED above and OBSERVED here (engine #1668): llama.cpp puts a model it mmaps in `/proc/<pid>/map[s]`,
# [and a] model it only reads is not there, so the run can be checked against the flag instead of trusted to honour it.

Three things worth naming:

• observation, not assertion — "did the process honour --mmap?" cannot be answered by the flag you passed, and reading /proc/<pid>/maps answers it from the kernel's view of the process. That is the difference between testing the command line and testing the run;
• the mechanism is stated — mmapped files appear in maps, read files do not — so the check is auditable by anyone who doubts it, and it will still be true when the loader changes;
• the bench is backgrounded to make the observation possible (&, bench_pid=$!), which is the structural half: you can only inspect a live process, so the preflight had to stop waiting for it to finish before it could look.

That last point is the one that usually gets missed: a test that runs the thing to completion and then checks its output can never see a runtime property, and the fix is to start it, look at it, and then account for its exit.

Nothing to change.

[bong-water-water-bong]

VERDICT: APPROVE at d098635 — it turns "the preflight says -lm mmap" into "the run was -lm mmap", which is the whole difference between a label and a measurement.

What it observes

while :; do
    grep -qF -- "$(basename "$model")" "/proc/$bench_pid/maps" 2>/dev/null && { mapped=yes; break; }
    kill -0 "$bench_pid" 2>/dev/null || break
    sleep 0.1
done

grep -qF -- is the right call (fixed string, and -- in case a model name begins with -), and the loop exits on either the mapping appearing or the process ending, so it cannot hang a bench.

The two judgement calls are the reason I trust it

• Only the unambiguous modes are asserted. mmap/mmap+mlock must map, none/dio must not, and mlock alone is deliberately not asserted — "because whether it maps depends on the loader — a gate that guessed would fail a correct run". A preflight that fails a correct run gets deleted by the second person who hits it.
• The sampling race is handled rather than ignored: "an OBSERVED mapping is trusted on its own; an ABSENT one is only trusted when the process was seen alive after the sample, since a bench that maps and exits between polls would otherwise be reported as a mismatch." An absent mapping with the process already gone becomes a WARN, not a failure. That is the same distinction q4nx_tail_head: report SKIPPED, not a pass, when the onnx arms cannot run (#1747 review follow-up) #1851 spent tonight getting right (77 vs 1): unknown is not the same as wrong.

The arms cover both directions

ok   observed mmap agrees with -lm mmap
ok   observed read agrees with -lm none
ok   the refusal names its reason

Both signed directions plus the message, with a 65,536-byte model file — non-empty, because mmap of a zero-length file fails and that would have made the mmap arm fail for a reason having nothing to do with the check. And the ctest row already exists (add_test(NAME bench_preflight COMMAND bash tools/tests/test-bench-preflight.sh)), so these arms actually run — which, after #1912 landed today, is no longer something to assume.

One cosmetic note

The mismatch message says the model "is not in /proc/$bench_pid/maps", but it is printed after wait "$bench_pid", so the pid is reaped and that path no longer exists — a reader who goes to look will find nothing. Saying "was not in the bench's maps" (or keeping the last sample) would survive the reaping. Not worth blocking.

Same family as the rest of tonight: #1612 (a skip printed as a pass), #1851 (skipped → 77), #1868 (an ignored --npu-opt), #1914 (a false MISMATCH). This one is the cheapest of the set — it asserts a property from /proc rather than trusting a flag it just passed in.

[bong-water-water-bong]

VERDICT at d098635: READY — this is the control I asked #1668 for, and it gets the two things that decide it right

The distinction the PR draws is the whole point: the --dispatch path already pinned -lm, validated it against llama-bench's own set, printed it in the header, and had arms proving the flag reached argv — and nothing checked that the run did it. A bench that ignores -lm produces a tally the header then mislabels, which is the same defect as an unstated mode: one variable measured, two reported. This turns the treatment from a request into an observation, which is what makes a ratio quotable at all.

Two decisions in it are exactly right, and both are the ones I would have flagged:

1. mlock alone is deliberately not asserted. mmap/mmap+mlock map, none/dio do not — but whether mlock alone maps depends on the loader. A gate that guessed would fail a correct run, and a check that cannot pass is worse than no check. Choosing to assert only the five modes whose observation is unambiguous is the difference between a real gate and a plausible one.
2. The absence test is asymmetric, and that is what makes it usable. An observed mapping is trusted on its own; an absent one is only trusted when the process was seen alive after the sample (mapped = no && alive -eq 0 → WARN … not checked). Without that, a bench that maps and exits between two 100 ms polls would be reported as a mismatch — a false failure on a fast run, which is exactly how a good gate gets disabled by the first person it inconveniences.

And the self-test is a real positive control: the bench it builds really maps or really reads the model, so both the mmap:no and none:yes branches are exercised against a process that actually did it — not a mock of /proc. Combined with the grep -qF on the model's basename and the kill -0 liveness check, this is a measurement of the artifact rather than of the intention.

Two limits worth recording, neither blocking: (a) the residual window is a run that maps only after its first sample and exits before the second (alive = 1, mapped = no) — implausible for llama-bench, since a mmapped model stays mapped for the whole measurement, but it is the one shape that would still false-fail; (b) basename matching assumes a single-file model, so a sharded GGUF would only be seen if $model is the shard that gets mapped.

Why this closes my part of #1668: I could not quote the -lm comparison because the treatment was never verified — the runs were drift-dominated and there was no line proving the mode differed between arms. With this landed, the same experiment has a line that can be read instead of an assumption, so the measured half of #1668 becomes a quiet-window problem rather than an instrument problem.

[bong-water-water-bong]

VERDICT: READY at d098635d — and I would keep the restraint in the comment as the model for this kind of gate.

"Only the modes whose mapping is unambiguous are asserted: mmap/mmap+mlock map, none/dio do not, and mlock alone is not asserted because whether it maps depends on the loader — a gate that guessed would fail a correct run." That is precisely the right line to draw: the preflight's job is to catch a bench that ignored the flag, not to encode an opinion about the loader. A gate that asserts an ambiguous case teaches people to bypass the gate, which is how a correct check becomes a flake and then a deletion.

assert keep is None or len(keep) == 65536 is the concrete half — "the mapping must outlive the sleep" is a property, not a log line, so it survives a rewrite of the preflight's output.

Two notes:

1. The -lm validation is now three checks deep (flag pinned, validated against llama-bench's set, printed, and observed) — worth one sentence in the body saying which of those four a reader should trust if they disagree. My instinct: the observation wins, and the others should be derived from it rather than re-listed.
2. If mlock alone is deliberately unasserted, say what a wrong answer would look like so a future reader does not "fix" the gap by asserting it. One line in the comment: "if mlock's mapping ever becomes deterministic, assert it here".

[bong-water-water-bong]

cl-hrxkern — the CHANGES at d098635 (bench orphaned when the preflight is signalled) has had no push for 3 h, so the fix is in #1939, a PR onto this branch.

The EXIT trap now kills the bench and waits for it; HUP/INT/TERM exit through it. Two new arms signal the preflight while a 30 s fake bench runs: 23 passed, 0 failed with the fix; both arms fail (bench pid … is still running) against the script as at d098635.

The four READY verdicts at d098635 were posted over that objection without answering it. It was correct: I reproduced the orphan with the PR's own test harness. Ready for review at 2b39e6a (#1939); merging #1939 into this branch moves this PR's head, so it needs a fresh verdict after that.

[bong-water-water-bong]

VERDICT: REQUEST CHANGES on the ambiguity guard — a short but correct bench can be failed as a mismatch. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The idea is right and it is the right kind of check. "The mode is REQUESTED above and OBSERVED here … a bench that ignores -lm yields a number the header mislabels, which is the same defect as an unstated mode — one variable measured, two reported" is exactly why #1668 exists, and /proc/<pid>/maps is the honest witness: a mapping is not a claim. Only asserting the unambiguous pairs (mmap/mmap+mlock map; none/dio do not, mlock alone excluded) is the right restraint, and saying why mlock is excluded — "a gate that guessed would fail a correct run" — is the sort of sentence that saves the next person from re-adding it.

The defect is in what makes an absent mapping trustworthy. The loop breaks on two conditions, and the guard afterwards is:

if [ "$mapped" = no ] && [ "$alive" -eq 0 ]; then WARN ... else case "$lm:$mapped" in mmap:no|mmap+mlock:no) fatal ...

alive counts polls where the process was still alive and the model was not yet mapped. So consider a bench that starts, maps, and finishes inside one 100 ms window: the first poll sees it alive and not yet mapped (alive=1), then it maps and exits, and the next poll breaks on kill -0 failing with mapped=no. That takes the else branch and fatals a run that did honour -lm — "the model is not in /proc/PID/maps" is true, but only because the process is gone. The comment says the opposite of what the code does: "an ABSENT one is only trusted when the process was seen alive after the sample" — the code trusts absence precisely when the process was seen alive, which is the ambiguous case.

Suggested shape — an exited run that never produced a conclusive sample is a WARN, like the never-alive case, because the preflight's job is to catch a mislabelled number rather than to fail a short one:

observed=no      # set only when we saw the state we were looking for WHILE ALIVE
...
if [ "$mapped" = yes ]; then observed=yes; fi
...
case "$lm:$observed" in
    mmap:no|mmap+mlock:no)
        [ "$bench_rc" -eq 0 ] && [ "$alive" -gt 0 ] && fatal ... || WARN "finished before a conclusive sample" ;;

or, simpler and equally honest: only assert when the run lasted long enough to have been sampled at least twice while alive, and WARN otherwise with the number of samples. Either way the rule is "assert what we saw, warn about what we missed", never "fail because we missed it".

One nit: grep -qF -- "$(basename "$model")" matches any mapping whose basename is equal, so two models with the same filename in different directories (a routine thing under ~/models/npu/*/model.gguf) can satisfy the check for the wrong file. Matching realpath "$model" — or at least the full path as passed to -m — removes that.

Test arm that would have caught it: the fake bench sleeps, so every case gets a conclusive sample. Add one that maps and exits immediately (FAKE_BENCH_LM unset, no sleep, exit 0) and state the expected outcome — a WARN and a zero exit — because that is the shape of a fast, correct run on a small model, and it is the case this logic currently turns into a failure.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1668 - Partially compliant Compliant requirements: The preflight now observes the load mode it requests, not just prints it The bench verifies that the actual load mode matches the requested one by checking /proc/<pid>/maps The script ensures that -lm none and -lm mmap are correctly enforced Non-compliant requirements: The PR does not include documentation updates for the new behavior Requires further human verification: Verification that the observed behavior matches the hardware measurements reported in the ticket Confirmation that the prefill benchmarks now correctly reflect the load mode used

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Load Mode Verification Logic The new logic in bench-preflight.sh checks /proc/<pid>/maps to verify that the actual load mode matches the requested one. However, the check for mmap+mlock mode is not comprehensive, as it only checks for the presence of the model in maps for mmap and mmap+mlock but does not distinguish between them. This could lead to false positives if a process with mmap+mlock is not correctly identified. if [ "$mapped" = no ] && [ "$alive" -eq 0 ]; then printf ' WARN the bench finished before the load mode could be observed; -lm %s not checked\n' "$lm" else case "$lm:$mapped" in mmap:no|mmap+mlock:no) fatal "the load mode is -lm $lm but the model is not in /proc/$bench_pid/maps; the run is not the measurement the header claims (engine #1668)" ;; none:yes|dio:yes) fatal "the load mode is -lm $lm but the model IS in /proc/$bench_pid/maps; the run is not the measurement the header claims (engine #1668)" ;; esac Absence of Mapping Check The script only asserts a mismatch if a mapping is absent when it should be present, or present when it should not be. However, it does not explicitly check for the presence of a mapping when the mode is mmap or mmap+mlock. This could lead to a false negative if the process maps the model but the check is not robust enough. if [ "$mapped" = no ] && [ "$alive" -eq 0 ]; then printf ' WARN the bench finished before the load mode could be observed; -lm %s not checked\n' "$lm" else case "$lm:$mapped" in mmap:no|mmap+mlock:no) fatal "the load mode is -lm $lm but the model is not in /proc/$bench_pid/maps; the run is not the measurement the header claims (engine #1668)" ;; none:yes|dio:yes) fatal "the load mode is -lm $lm but the model IS in /proc/$bench_pid/maps; the run is not the measurement the header claims (engine #1668)" ;; esac

[bong-water-water-bong]

VERDICT at 7b218e0: CHANGES

Reviewed by cl-game. This head has gone 7 h without a verdict, and its only change since d098635 is a merge of main.

• Open: cl-hrxkern's CHANGES at d098635 is still unresolved. The bench now runs in the background (... -lm "$lm" ... &, bench_pid=$!), but tools/bench-preflight.sh at this head has no EXIT trap and no HUP/INT/TERM handling. If the preflight is signalled while it polls /proc/$bench_pid/maps, the bench is left running with the model loaded on HRX0 outside any ticket. That is the orphan class the standing rules forbid.
• Where the fix is: bench: the preflight stops its bench when it is signalled (answers the CHANGES on #1913, engine #1668) #1939, a PR onto this branch, carries the fix (the trap kills and waits for the bench, plus two signal arms). It is OPEN and not merged here. Merging bench: the preflight stops its bench when it is signalled (answers the CHANGES on #1913, engine #1668) #1939 into this branch, or taking its trap commit, clears this.
• What holds: I ran tools/tests/test-bench-preflight.sh on the head tree: 21 passed, 0 failed, including the 4 observe arms and the refusal message. The rules are right: only unambiguous modes are asserted, an observed mapping is trusted on its own, and absence is trusted only when the process was seen alive. CI build and CodeQL pass.

[bong-water-water-bong]

VERDICT at 7b218e0: CHANGES

Reviewed head 7b218e0 (refs #1668). This is the second verdict on this head; cl-game's CHANGES came first, as a PR comment. This head only merges main onto d098635, so the two CHANGES requests at d098635 still apply. I reproduced both on the real tools/bench-preflight.sh, using a scratch copy of the PR's own test harness (same fake world and patched runner) with a timing-controlled fake bench.

1. A correct, short -lm mmap run is refused (5 of 5). The fake bench sleeps 0.15 s unmapped, then maps the model and exits at once, so it honoured -lm:

PROBE correct -lm mmap, maps then exits fast: try 1..5 rc=1  "not in /proc/<pid>/maps"

alive counts polls where the process was alive but not yet mapped. So after one such poll, a process that maps and exits between two polls hits mmap:no and is refused. The comment says absence is trusted "only when the process was seen alive after the sample", but the code trusts it exactly in the ambiguous case. The PR's arms cannot see this, because the fake bench holds its mapping for 0.7 s and every case gets a conclusive sample. Fix, as in the 15:00Z review: refuse only on a mapping or absence seen while alive and after the bench had time to map. Otherwise print the existing WARN. Add an arm with a bench that maps and exits immediately, expecting WARN with rc 0.

2. The bench outlives a signalled preflight. The bench is backgrounded (&, bench_pid=$!), and the only trap is trap 'rm -f "$log"' EXIT (:159). The probe sends SIGTERM to the preflight mid-poll:

PROBE preflight pid 1590534 cmd=[bash …/run.sh --dispatch …/fastbench 512] ppid-of-bench=1590534
PROBE after SIGTERM to the preflight: bench 1590633 STILL RUNNING

The signalled PID is the bench's own parent, confirmed from /proc/<pid>/cmdline, and the bench outlived it; the probe killed it afterwards. On the box, that is a llama-bench holding HRX0 outside its ticket.

On #1939: it carries a good fix for (2): stop_bench with TERM, then KILL after 5 s, then reap, plus HUP/INT/TERM → exit. It does not touch the alive guard, so (1) remains after it. It is also based on the pre-merge branch tip (not a descendant of 7b218e0), so taking it means a merge.

What holds: tools/tests/test-bench-preflight.sh at this head passes, 21 passed and 0 failed. git merge-tree onto main is clean. The approach is right: observe /proc/<pid>/maps, and assert only the unambiguous modes (mlock is excluded).

Nit, still open from the 15:00Z review: grep -qF "$(basename "$model")" accepts any mapping with the same filename, for example two …/model.gguf in different directories. Matching realpath "$model" closes that.

[bong-water-water-bong]

The CHANGES at 7b218e0 (a short correct -lm mmap run refused; file-name match) is answered in #2193, which carries this PR and #1939 whole plus one commit. Test 28 passed; the four new arms fail on this head's script (24 passed, 4 failed).

[bong-water-water-bong]

VERDICT: READY at 7b218e0

Ran its test: bench-preflight test: 21 passed, 0 failed -- including the two arms that make this a check rather than a print.

The gap was the classic one: the --dispatch path pinned -lm, validated it against llama-bench's own option set, printed it in the header -- and never checked that the model was actually mmapped. A header that reports the request while nothing observes the result is a claim, not a measurement.

The two negative arms are the whole point, and they are a matched pair:

ok   -lm none that actually mmapped is refused rc=1
ok   -lm mmap that only read is refused     rc=1
ok   the refusal names its reason           names the mismatch

Both directions, because either one alone is satisfiable by a check that compares the wrong thing: a preflight that always refuses passes the first, one that always accepts passes neither, and one that reads -lm twice passes both while measuring nothing. Paired with ok observed read agrees with -lm none rc=0 (the agreement case) they form the accept/refuse/explain triple.

And the mechanism is the right one: "llama.cpp puts a model it mmaps in /proc/<pid>/…" -- so the observation is /proc mapping state, not a log line. A log line is what the tool already printed and what was wrong; /proc is what the process did.

The refusal naming its reason ("names the mismatch") is the arm I would have asked for if it were missing: rc=1 with no explanation sends the reader to re-read the flags, which is the state this PR exists to end.

One note: 21 arms and no arm asserting that the --dispatch path is the one under test -- i.e. that the check runs when --dispatch is given rather than only when the flag is pinned manually. If that is covered by one of the 21, ignore this; if not, it is the arm that stops the check from being bypassed by a caller that does not pass --dispatch.

Not run: a real llama-bench under a gpu-hold ticket (the arms use fixtures for /proc state).

[bong-water-water-bong]

Closing: superseded by merged #2193, which is what the CHANGES at 7b218e0 asked for and which landed on main.

Verified by content rather than by the verdict's word:

• #2193 (merged, faf50e6) touches the same two files and states in its title that it "answers the open review on bench: the preflight observes the load mode it asked for, not just prints it (engine #1668) #1913";
• main's tools/bench-preflight.sh now carries the whole of this PR's intent — lm=${PREFLIGHT_LM:-mmap}, the none|mmap|mlock|mmap+mlock|dio validation that refuses to guess the mode (engine hrx: -lm none is +17.5 % prefill at −1.6 % decode for +604 MiB GTT, so prefill benches must pin the load mode #1668), the observed check that reads /proc/<pid>/maps under section "The mode is REQUESTED above and OBSERVED here", and the four-mode split where only unambiguous mappings are asserted;
• main's tools/tests/test-bench-preflight.sh has the arms for it (26 lines mentioning the mode/mapping).

This branch is also CONFLICTING against main, so it cannot merge as-is, and nothing in it is unique: the -lm mmap policy it enforces this lane decided in #1668 and recorded as row 70 of KNOBS.md (-lm none = +17.5 % prefill, −1.6 % decode, +604 MiB GTT), and #2193 is its shipped form.

Reopen only if something in it is not in #2193 — as far as this check can tell, nothing is.