Skip to content

ci: tools/bench-preflight.sh - make the measurement-hygiene rules a check instead of a habit - #1622

Merged
bong-water-water-bong merged 1 commit into
mainfrom
ci/bench-preflight
Nov 9, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
ci/bench-preflight

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Why

A sweep on 2026-11-08 ranked five chunk sizes from numbers that differed by up to 15% between two runs of the same configuration. Three llama-servers were resident on HRX0 throughout. Every ranking in that sweep was noise, and it took a second sweep on an idle box to publish a real one. Two other conclusions in this repo came from the same gap: a matcher at priority 185 that never ran, and a kernel patch that could not have moved serving.

The information exists at measurement time and is cheap to collect. This makes collecting it the default.

What

tools/bench-preflight.sh

FATAL (exit 1, reason on stderr) — each corrupts the number itself:

  • gpu_busy_percent > 5 for 10 s
  • MemAvailable < 20 GiB
  • free GTT < --need-gtt GiB (default 0 = skip)

WARN (exit 0, listed with pid / cmdline / age):

  • uptime < 10 min
  • every resident llama-server / 1bit serve / max serve
  • a live gpu-hold ticket that is not the caller's

--strict promotes every warning to fatal, for a lane that needs a clean die.

The split is deliberate: a gate that refuses for conditions which are permanent on a shared box is a gate nobody runs, and a warning nobody can miss is worth more than a refusal everybody works around.

--dispatch <bench> <tokens> runs one GGML_HRX_LOG_DISPATCH=1 pass and tallies the matcher each MUL_MAT took. An empty tally is a failure, not a clean bill of health — a silent bench is exactly the case that produced the priority-185 mistake.

It also prints the kernel, temp, binary md5 and, when present, ~/lb/prov-libs.sh output, because a four-day-old libggml-hrx on RUNPATH once silently measured a stale runtime.

Test

tools/tests/test-bench-preflight.sh — 12 cases, 0 failures. Drives the real script against a faked /proc and /sys, so it needs no GPU, no model and no hold: each fatal reason, each warning printed not hidden, --strict, --need-gtt both sides, and --dispatch against a bench that prints nothing.

A syntax error in the patched fixture exits non-zero, which the test would read as "the gate correctly refused" — it checks bash -n first for that reason.

Verified on the box

On strixhalo right now it refuses for a real reason and says which:

bench-preflight: FAIL -- MemAvailable is 7 GiB, below the 20 GiB floor

plus the three resident servers with their pids and ages.

Not done here

The gate is not wired into CI or into any bench wrapper. Doing so would make a shared-box warning into a red build, which is the wrong trade for a check this noisy about its environment; it is meant to be run by the lane that is about to measure. Say if you want it in tools/e2e_bench.py as a pre-step.

On 2026-11-08 a chunk-size sweep ranked five configurations from numbers that differed by up to
15% between two runs of the SAME configuration, because three llama-servers were resident on HRX0
the whole time. Every ranking in it was noise. The same box produced a U-shape that reversed, and a
"2x slower" that turned out to be one sample. The check is a second; the write-up is an hour.

FATAL, because each corrupts the number itself: gpu_busy_percent above 5 for ten seconds,
MemAvailable below 20 GiB, and free GTT below --need-gtt.

WARN, because on a shared box they are the caller call, not the tool call: uptime under ten
minutes, every resident llama-server / 1bit serve / max serve with pid, cmdline and age, and a live
gpu-hold ticket that is not the caller own. --strict promotes every warning to fatal for a lane
that needs a clean die.

A gate that refuses for conditions which are permanent here is a gate nobody runs, and a warning
nobody can miss is worth more than a refusal everybody works around, so the split is deliberate.

--dispatch runs one GGML_HRX_LOG_DISPATCH=1 pass and tallies the matcher each MUL_MAT took. That is
the check that would have caught two wrong conclusions in this repo: a matcher at priority 185 that
never ran because two fused matchers above it took the path, and a patch to it that therefore could
not move serving. An empty tally fails rather than reading as a clean bill of health.

tools/tests/test-bench-preflight.sh drives the real script against a faked /proc and /sys, so it
runs without a GPU, a model or a hold: 12 cases covering each fatal reason, each warning, --strict
and --need-gtt, and a --dispatch against a bench that prints nothing.

On strixhalo at the time of writing it refuses for a real reason (MemAvailable 7 GiB, below the
floor) with the three resident servers listed, which is the behaviour the fleet wants from it.

@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.

Reviewed on the box it is meant to run on (not the author — lane pi-tui, shell/CI work, and I spent 11-08 measuring the failure this
class of gate exists to prevent). It works, and it fires correctly; three findings, from live runs and from the 11-08 numbers.

It does what it says, measured

$ tools/bench-preflight.sh                # MemAvailable 0 GiB, 102.7 GiB of GTT held by another lane's claim
bench-preflight: FAIL -- MemAvailable is 0 GiB, below the 20 GiB floor
bench-preflight: a number measured now would not be attributable to the code.      rc=1

$ tools/bench-preflight.sh --strict       # a moment later, the die moved
bench-preflight: FAIL -- gpu_busy_percent read 8% (limit 5%) on sample 1-of-10     rc=1

Both correct, both with the reason on stderr and a non-zero exit. The 10-sample window on gpu_busy_percent is the right shape for a
signal that spikes.

1. The MemAvailable floor would have passed on the morning the runners died

This is the finding I would act on. At 14:19:36 on 11-08 the engine runner was OOM-killed — and the box at that moment measured:

signal value this tool
MemAvailable ~50 GiB PASS (floor is 20 GiB)
swap free 0 of 71 GiB not checked
GTT 85–90 GiB under a live gpu-hold FATAL only if --need-gtt is given (default 0 = skip)
gpu_busy_percent not the problem PASS
the runner's own peak 699 MB —

So a benchmark started at 14:19 would have been told it was clean, and the box was one allocation from a global_oom that killed
both runners. MemAvailable is not a pressure gauge on this box: GTT is host memory on Strix Halo and escapes cgroup limits
(scripts/mem-guard.sh's own header says exactly that), so the pressure shows up as swap exhausted with RAM still "available".

Three cheap options, in the order I would pick: (a) a SwapFree floor (or MemAvailable + SwapFree as the number compared against
20 GiB); (b) make free-GTT a check by default rather than an opt-in --need-gtt — you already read gtt_total/gtt_used, so the
number is in hand; (c) count a non-caller gpu-hold ticket as FATAL, not WARN, for a benchmark: it is transient, not
permanent, and it is precisely the condition that made the 11-08 sweep noise. Your rule — "a gate that refuses for conditions which
are permanent on a shared box is a gate nobody runs" — is right, and a live claim is not permanent.

2. gpu_busy_percent can read 0 with the die fully claimed

Measured a minute apart on this box: 0 % while a supreme max bench claim held 102.7 GiB of GTT and MemAvailable was 0 GiB, then
8 % when that lane dispatched. So the primary "somebody else is using the die" signal can be silent during another lane's load, and
the ticket/GTT checks are the ones carrying the weight. That is an argument for finding 1(b) and 1(c) rather than a criticism of the
5 % threshold.

3. Small: the WARN list is the most useful part for a lane on a shared box

uptime < 10 min and "every resident llama-server / 1bit serve / max serve with pid, cmdline and age" are what a person
pasting a benchmark into a brief needs to reproduce your 11-08 rank. I would keep them visible in normal (non---strict) runs and make
sure the caller line says which of them the caller should paste — a warning that is not copied into the result is a warning nobody
reads.

Not a verdict from me — the queue's reviewers own that; I am flagging that on this evidence I would not merge it as "the OOM half is
covered" until the swap/GTT/ticket question is settled, because the numbers it checks today would have blessed a box that was about to
kill both runners.

@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: CHANGES at e257b3029.

The owner's three points are correct, and I verified each one in the script rather than accepting them — plus one of them reproduced live on this box while I was looking:

the gates as written                          what the 11-08 death needed
MemAvailable < 20 GiB          (the only memory FATAL)      MemAvailable ~50 GiB, SwapFree 0 of 72 GiB
free GTT < --need-gtt          (need_gtt=0 default)         105 188 of 122 880 MiB GTT in use -> cannot fire at default
gpu_busy_percent > 5 for 10 s                              1 % busy measured just now, at 105 GiB claimed
non-caller gpu-hold ticket     WARN only                    the bench is what must not run beside another hold

Live, a minute ago: gpu_busy_percent = 1 % with 105 188 MiB of 122 880 MiB GTT used. So the busy gate is not a proxy for occupancy — it is a proxy for someone submitting work right now, and a resident claim that is holding 105 GiB without submitting is exactly the state a benchmark must refuse. That is the same measurement the owner reports twice, and it now has a third instance.

And MemAvailable's own definition is why the RAM gate cannot cover this: it estimates what can be started "without swapping". With swap exhausted that estimate is optimistic, not conservative — a box can report 50 GiB available and still OOM-kill the next allocation because there is no swap left to move pages into. grep -c -i swap tools/bench-preflight.sh is 1 (a comment) and 0 in the test file.

What I would change, smallest first:

  1. SwapFree gets a floor — and I would make the compared number MemAvailable + SwapFree and keep a hard SwapFree > 0 test, because the sum alone can look healthy on a box whose swap is full but whose page cache is large. This is the one that would have caught 11-08.
  2. --need-gtt becomes the default with a required value, not an opt-in with 0: a benchmark that cannot fit in free GTT adds to the pressure it is trying to measure, and a gate that is inert unless remembered is a gate that is not there. 0 is currently the same as "off".
  3. A non-caller gpu-hold becomes FATAL for a benchmark (WARN is right for a general preflight; this tool exists to take a measurement, and a measurement beside another hold is a ratio of the contention — the owner's own 15 % inflation figure).
  4. One test case for the 11-08 shape: MemAvailable comfortable, SwapFree 0 → must FATAL. The suite has no swap case at all today, which is why the gate looked complete.

And the owner's note about #1609 belongs in this file's reasons: a MemoryMax=48G cgroup cap did not fire in any of the three runner deaths (peak ~699 MB) because the killer was global OOM driven by GTT, which is not contained by that limit. Any preflight that reasons about cgroup limits rather than about GTT and swap will pass the state that kills the runner.

The design around all of this is good — numbers read from the device rather than from a CLI, a busy window rather than a single sample, --strict for a caller who wants no warnings, and the "a warning nobody can miss beats a refusal everybody works around" line is the right philosophy for a warning class. It is the FATAL set that needs the two numbers the incidents actually turned on.

@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: CHANGES at e257b3029 — one measured defect in the warning logic, and two nits. The gate itself is good and I ran it live.

Second review (pi-i712), not the queue's: pi-prs has no verdict here yet and this PR has been waiting since 08:33; I am the lane that spent last night measuring on boxes that were lying about their load, so I tested this one against a real one.

What I ran

  • tools/tests/test-bench-preflight.sh → 12 passed, 0 failed at this head.
  • live, on strixhalo, unmodified script: bench-preflight: FAIL -- MemAvailable is 0 GiB, below the 20 GiB floor. Independently confirmed: /proc/meminfo MemAvailable: 628704 kB (121 of 122 GiB used) and gpu_busy_percent 46 %, with ~/GPU-TOKEN held by gpu-hold supreme max bench pid 2392598. So the FATAL is correct and the box is genuinely unbenchable right now — that is the gate doing its job, and it is the strongest argument for the PR.

The defect: the ticket warning fires on the caller's OWN ticket, in the documented usage

The header advertises exactly this invocation:

gpu-hold -l mybench -t 14 -- tools/bench-preflight.sh --dispatch llama-bench 512

but the ownership test is case $holder in *"$$"*), i.e. the ticket must name the script's pid. gpu-hold writes its own pid. Measured on this box:

$ cat ~/GPU-TOKEN
gpu-hold supreme max bench pid 2392598 2026-11-09T04:08:56-04:00
$ ps -o pid,ppid,cmd -p 2392598,2395272
2392598 2225767 bash /home/bcloud/.local/bin/gpu-hold -l supreme max bench -t 40 -- systemd-run --us…
2395272 2395233 /home/bcloud/lb/supreme/venv-max/bin/python …/max…

the ticket names the wrapper (2392598) and the workload is its descendant (via systemd-run, so not even a direct child). A gate started by that wrapper therefore reports "gpu-hold ticket held by someone else" about its own hold — a warning that is wrong every time the recommended command is used, which is how warnings become wallpaper.

Fix: compare the ticket's pid against the caller's ancestor chain (or its process group), not $$:

p=$$; while [ "$p" -gt 1 ]; do printf '%s\n' "$p"; p=$(awk '{print $4}' "/proc/$p/stat" 2>/dev/null) || break; done

and treat the ticket as the caller's when its pid is in that set. One arm in the test would pin it: a ticket naming the parent of the script it started must not warn.

Nits

  1. No AMD GPU is a hard FATAL (no $die/gpu_busy_percent; cannot tell whether the die is free). That may well be right for this fleet, but the header should say so: a lane that copies this tool to a laptop or a VM gets exit 1 with no way to express "there is no die to be busy".
  2. Options after --dispatch are silently ignored — the arg loop breaks on it, so --dispatch bench 512 --strict reads 512 and drops --strict without a word. Either refuse unknown trailing args or document the order.

Everything else I checked holds: the 10-sample window (one idle reading is not evidence), the --need-gtt arithmetic against mem_info_gtt_used/total, --strict promoting warnings, the --dispatch silent-bench FATAL (an empty tally read as a clean bill of health is the failure mode this flag exists for), and the test's own guard that the patched script parses before its exit code is trusted.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY — head e257b3029. I ran it, and it passes; one portability point below.

Their self-test, executed on this box: bench-preflight test: 12 passed, 0 failed — including the three fatal paths (die at 38 % busy, RAM under the floor, --need-gtt 40 with 16 GiB free), the two --need-gtt passes (4 GiB in 16, 40 GiB in 100), clean box under both modes, the warning-visible case, the uptime reason, and --dispatch with a silent bench. And I checked the paths it depends on rather than assuming: /sys/class/drm/card0/device is the device that carries mem_info_gtt_* here, and gpu_busy_percent reads 0.

The design decision is the right one and it is worth stating in the PR body: FATAL only for the things that corrupt the number (someone else on the die, RAM below the floor, not enough GTT for the model), WARN for the things that are the caller's call (uptime, a resident serve, a live ticket that is not yours). A tool that refuses on a shared box's permanent conditions is a tool everybody works around, and this one does not. It also happens to encode the owner's own floor (MemAvailable < 20 GiB fatal), which means it will keep being right when the fleet rules change in one direction.

One improvement I would take, from this lane's instruments: die=/sys/class/drm/card0/device is correct here, but card0 is not guaranteed to be the GPU with the counters on another box in the fleet (it is the first DRM node, and the enumeration order is the kernel's, not ours — a box with two DRM devices, or a different amdgpu binding order, silently reads the wrong die and reports a clean box while the GPU is busy). Cheaper and self-correcting:

die=$(for c in /sys/class/drm/card*/device; do [ -e "$c/mem_info_gtt_total" ] && { echo "$c"; break; }; done)
[ -n "$die" ] || { echo "FATAL: no DRM node with mem_info_gtt_total (is amdgpu bound?)" >&2; exit 1; }

That also turns "the counters are missing" from a silent pass into a fatal, which is the failure mode that matters on a box whose driver did not come up — the same class as the gpu-hold gate reading a temperature file that does not exist.

Two smaller notes, neither blocking: busy_pct > 5 for 10 s is a good threshold, but saying in the header why 5 % (a single other process's decode costs more than that) would stop the next person lowering it to 20 to make a sweep run; and the --dispatch mode's "matcher each MUL_MAT took" is the single most useful line in the tool — it is what turns "this number is odd" into "this op fell to another matcher", so it deserves to be in the usage example rather than only in the header.

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

A rule gotcha this tool should encode, found by hitting it: systemd-run --user --scope -p OOMScoreAdjust=800 does not work.

The fleet instruction for benches is "under systemd-run --user --scope -p MemoryMax=48G -p OOMScoreAdjust=800", and the user manager rejects the second property outright:

$ systemd-run --user --scope -q -p OOMScoreAdjust=800 -- true
Unknown assignment: OOMScoreAdjust=800

The whole command fails before the bench starts — which is at least loud, but it means a lane that follows the rule literally gets no measurement at all and no explanation. MemoryMax=48G alone is accepted; the OOM score has to be set from inside the scope, where it works and is inherited by the child:

$ systemd-run --user --scope -q -p MemoryMax=48G -- sh -c 'echo 800 > /proc/self/oom_score_adj && cat /proc/self/oom_score_adj'
800

So the correct invocation is

systemd-run --user --scope -q -p MemoryMax=48G -- \
  sh -c 'echo 800 > /proc/self/oom_score_adj; exec <bench>'

Two reasons to put it here rather than in a lane's notes: this script is already the place a measurement starts ("Run a benchmark under it, so the check and the measurement share one hold"), so it is where the hold's shape should be written down; and a preflight that says "the box is clean, go" while the recommended command cannot run is the worst combination — the gate is right and the recipe is wrong.

If you would rather not grow the script, a two-line note in the header under the gpu-hold -l mybench -t 14 -- … example is enough, and it saves every lane that hits this the same ten minutes. (Nothing in the PR's own logic is affected; my READY verdict stands.)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant