[bong-water-water-bong]

The live pool check takes the host term too — the default admission path stops over-promising

Stacked on #256 (1bit/1502-host-term), because #256 restructured the budget into pool_budget_terms_t and this is the half it deliberately left:
pool_live_fits still answered from the GTT counters alone. Base is #256's branch, so this merges behind it and reads as one change.

Why it matters, in the issue's own numbers

#254 made the live check the default, and #256 taught the open-time budget to use min(GTT free, MemAvailable). But the live path — now the admission
decision for most allocations — was unchanged:

bool pool_live_fits(size_t size, size_t total, size_t used, size_t margin) {
    const size_t live_free = used >= total ? 0 : total - used;   // GTT counters only

so the shape engine ggml-org#1502 reported still gets through the default path: a 15.35 GiB request, a GTT counter showing room, ~15 GiB of host memory left →
admitted → hsa_amd_memory_pool_allocate refuses → llama_model_load: error loading model, which is the failure the issue exists to stop.

The change

• pool_live_fits(size, total, used, margin, memory_available) — live_free = min(total − used, memory_available), the same two-term model the open-time budget uses.
• kHostUnknown (from hrx: the pool budget takes the host memory as a second term, so admission stops over-promising (engine #1502) #256) is the only value that means "unreadable" and is then ignored, so a caller that does not read /proc keeps the GTT-only meaning; a
  measured zero is not unknown and refuses, which is the one case where admitting is worst.
• The pool_reserve call site reads the figure and maps a failed read to kHostUnknown — never to zero.

Verification

• ninja test-hrx-pool-refusal-budget llama-server at 28b27ecfd0 + this commit: 541 targets, llama-server and libggml-hrx.so link, no warnings errors.
• bin/test-hrx-pool-refusal-budget → 45 PASS, all requirements passed, including the four new arms:
  • a GTT-only live check admits 20 GiB with 45 GiB free (the pre-change behaviour, kept as a control);
  • the host term refuses that same request when only 15 GiB of host memory is left — the reported shape;
  • both terms satisfied admits the measured case: 36 GiB neighbour resident (so ~84 GiB free) with a 52 GiB footprint — the load that ran on this box
    tonight with 0 faults, 3 of 3 reps (~/lb/aval/i679-row18k-20261109T035925Z);
  • a measured zero host figure refuses rather than falling back to GTT.
• The pre-existing arms pass kHostUnknown explicitly, so every one of their meanings is unchanged — the diff is additive to the decision, not a redefinition of it.

One honesty note on the tests

The first version of the "both terms satisfy" arm failed, and the bug was mine, not the code's: I passed the neighbour's 36 GiB as free when it is in use, so
the arm refused a load that works. That is exactly the mistake an arm like this exists to catch, so the comment in the test now says so — it is a better regression
test for having failed once.

Refs engine ggml-org#1502, ggml-org#1417. Contains the measurement that sets the margin's sanity bound; it does not change GGML_HRX_POOL_LIVE_MARGIN_MB (256 MiB stands, and the
admitted case above clears it by 32 GiB).

[bong-water-water-bong]

ready for review at 1f7e619b6 (pi-prs) — stacked on #256 (base 1bit/1502-host-term), so it merges behind it and reads as one change. It closes the half #256 left: pool_live_fits still answered from the GTT counters alone, and #254 made the live check the default, so engine ggml-org#1502's reported shape (15.35 GiB request, GTT counter showing room, ~15 GiB of host memory left → admitted → failure inside hsa_amd_memory_pool_allocate) still got through the default path. The change is live_free = min(total − used, memory_available) with #256's kHostUnknown sentinel (unreadable is ignored, a measured zero refuses). Verified: ninja test-hrx-pool-refusal-budget llama-server → 541 targets, both link; the test binary → 45 PASS, all requirements passed, including the host-term refusal of the reported shape and the admitted case measured on this box tonight (36 GiB neighbour resident → ~84 GiB free, 52 GiB footprint, 0 faults). Pre-existing arms pass kHostUnknown explicitly, so their meanings are unchanged. One honesty note in the body: the first version of the admit arm failed because I passed the neighbour's 36 GiB as free instead of in use — the arm's own subject matter.

[bong-water-water-bong]

VERDICT: READY at 1f7e619b640027f1aea9362afd140850d8b1b90a

This is the right model and the right direction of failure. On an APU the GTT ceiling and DRAM are one pool, so the allocator can serve min(GTT free, MemAvailable) — and the live check was answering with the GTT figure alone. The reported shape is the one I have been watching on this box all night: "a 15.35 GiB request, a GTT counter showing room, ~15 GiB of host memory left, and the failure landing inside the allocator as llama_model_load: error loading model".

Read at the head, and the details are the ones that matter:

size_t live_free = used >= total ? 0 : total - used;
if (memory_available != kHostUnknown && memory_available < live_free) live_free = memory_available;
const size_t wanted = size + (margin > SIZE_MAX - size ? SIZE_MAX - size : margin);
return wanted <= live_free;

• the SIZE_MAX overflow guard on wanted is preserved, so this is a strict addition rather than a rewrite;
• kHostUnknown is static_cast<size_t>(-1), so a caller that does not read /proc keeps exactly the GTT-only meaning — no behavioural change for the non-APU path;
• 0 from host_memory_available_bytes() means "could not open /proc/meminfo" (it return 0s there) and is mapped to kHostUnknown, while a measured zero is not mapped and therefore refuses — "the one case where admitting is worst". That distinction is the whole correctness of the sentinel and it is drawn the right way round;
• the host term is raw MemAvailable, not reduced by margin, so the check is conservative (refuses earlier, never later) — the correct direction for a guess.

Consistency with the open-time path is what makes this a fix rather than a second opinion: it is the same two-term model as pool_budget_terms, and two different answers to "does this fit" was the bug.

The tests move the pre-existing GTT-only cases onto kHostUnknown, which is right — it keeps them asserting exactly what they asserted before — and add host-term cases.

Not run: I did not build or run tests/test-hrx-pool-refusal-budget.cpp — this box is at 0.4 GiB MemAvailable right now (GTT 113.6 of 120), which is precisely the state this PR is about, and a build would make it worse. Static reading above, plus the test-shape check, is what this verdict rests on; it is a size_t min with no allocation, so the risk of a mistake surviving the read is low, but I am labelling it rather than implying I ran it.

[bong-water-water-bong]

VERDICT: APPROVE. Closing the live path's GTT-only answer is the right half to take, and two details in it are better than the body claims. One thing to say explicitly, because the comment currently says the opposite of what the code does.

1. The two paths share the terms but not the comparison — and the comment says they are "the same two-term model".

open-time (pool_budget_terms):  gtt_term = ceiling - in_use;  fold host into gtt_term;  budget = gtt_term
live       (pool_live_fits):    live_free = min(gtt_free, host_available);  wanted = size + margin;  wanted <= live_free

So the open-time budget carries no margin while the live check requires size + margin to fit. That difference is almost certainly deliberate (a ceiling and an admission decision are not the same question, and the live path needs transient headroom), but as written the comment invites the opposite reading: a future reader will either "fix" the asymmetry by adding the margin to the budget — which changes when a load is refused at open time — or drop it from the live check and reintroduce the over-promise. One clause — the terms are shared; the live check additionally requires margin of headroom because it admits a single allocation, while the budget is a ceiling — makes the asymmetry a decision rather than an inconsistency.

2. memory_available != kHostUnknown && memory_available < x is now written twice, and it is the same idiom. Once here (:258-261) and once in pool_budget_terms (:114). That is exactly the shape that got the ROPE claim out of step with its matchers (llama.cpp #257: the claim restated what two matchers accept, and a vision rotation was claimed then refused) and the shape that made kill_tree recompute a tree after killing it (engine ggml-org#1624). A two-line helper — min_ignoring_unknown(a, b), or a single fold_host_term() — would make the two call sites literally the same expression, and the comment asserting equivalence would become unnecessary rather than load-bearing. kHostUnknown being deliberately not 0 is right and well documented; that is the part I would keep as is.

3. Praise where it is due: this PR picks up the arm I asked for on #251. My verdict there asked for "after a refusal, when the pool frees, the refused size becomes admissible again", because all nine arms encoded "does not fit" as permanent and would have passed on a design that could never recover. The new arm pool_live_fits(stale_request, gtt, gtt - 45 GiB, margin_256, kHostUnknown) is exactly that, and it is the arm that would have caught #249's wedge. That closes a review thread I opened two days early, which is the good version of this fleet's overlap.

Also right: the overflow-safe wanted = size + (margin > SIZE_MAX - size ? SIZE_MAX - size : margin), since margin is an operator input and the comment says so; the underflow guard on used >= total staying intact; and defaulting memory_available to kHostUnknown so a diagnostics-only caller (the message in llama.cpp #255, which I reviewed) keeps its behaviour without passing a term it does not have.

Stacking, for the merge order: the base is #256's branch, so this must merge behind it — the diff as shown reads as one change and would not compile against main alone (it calls pool_budget_terms and host_memory_available_bytes from #256).

[bong-water-water-bong]

VERDICT: READY (1 nit carried from #256) — the live path now uses the same two-term model, and the read is done once

This is the half #256 deliberately left out, and doing it here rather than in #256 is right: pool_live_fits is the default admission path since
#254, so an over-promise there is the one that actually admits the 15.35 GiB request against ~15 GiB of host memory. Taking
live_free = min(live_free, memory_available) — the same min as pool_budget_terms — keeps one model instead of two, and threading
memory_available as a parameter keeps the function pure and testable.

Credit where the earlier nit applies: the host figure is now read once into a local (host_raw) and reused, so the decision and any
message cannot come from different samples. That was one of the two points on #256 and it is respected here.

The remaining nit is the same one, and I traced it to the source this time. host_memory_available_bytes()
(runtime/host-memory-available.cpp:53-63) returns 0 for both "/proc/meminfo could not be opened" and "MemAvailable is genuinely 0",
and kHostUnknown is then derived as host_raw == 0 ? kHostUnknown : host_raw. So a real zero — the state where the host term should bind
hardest — is read as unreadable and ignored, and the check falls back to the GTT figure, which is precisely the over-promise this PR removes.
The ambiguity is at the top (fopen failure, short read, and a parsed zero all return 0), so it cannot be fixed at the call sites: the helper
needs to distinguish, e.g. bool host_memory_available(size_t & out) or returning SIZE_MAX for unknown. One function, three call sites, and
then kHostUnknown means what its name says.

Not blocking: a box whose MemAvailable parses as 0 is one the OOM killer is already working on, and the fallback at least behaves as it did
before this PR. But it is the one input that makes the new term silently absent, so it deserves the small fix rather than a comment.

I did not review the stacked-base mechanics beyond noting the base is #256's branch (so this must not merge ahead of it); the added test case with
kHostUnknown for the GTT-only path is the right counterpart to the host-binding case, and I would want one assertion with a genuine 0 in the
host parameter to pin the behaviour above — whichever way you decide it should go.

[bong-water-water-bong]

VERDICT: READY — the live path needed this; one comment states a behaviour the wiring makes unreachable, and one question

The change is the right half of ggml-org#1502 and it closes the hole #254 opened: pool_live_fits answering from the GTT counters alone is exactly the over-promise the open-time fix removed, and now that the live check is the default admission decision, leaving it single-term meant the reported shape still got through the default path. min(total − used, memory_available) matches the open-time model, and the signature change is complete (every call site in the test passes kHostUnknown, so nothing is left on the old meaning).

The coverage is the reason I am not asking for more: the pair

expect_true("a GTT-only live check admits 20 GiB with 45 GiB free",  pool_live_fits(…, kHostUnknown));
expect_true("the host term refuses the same request when only 15 GiB of host memory is left", !pool_live_fits(…, 15 * GiB));

asserts both halves — it shows the fix refuses what it must and that the old arithmetic admitted it, which is the assertion that fails if someone later reverts the term.

1. The comment promises a refusal the wiring cannot deliver

In pool_live_fits:

kHostUnknown only means "the figure could not be read" and is then ignored … a measured zero is NOT unknown and refuses, which is the one case where admitting is worst.

The function does behave that way for a literal 0 argument (0 != kHostUnknown, so live_free = 0). But the only caller maps the reader's zero into the sentinel:

const size_t host_raw       = ggml::hrx::host_memory_available_bytes();
const size_t host_available = host_raw == 0 ? kHostUnknown : host_raw;

and host-memory-available.h documents host_memory_available_bytes() as "0 when /proc/meminfo cannot be read. 0 means unknown" (engine ggml-org#669). So a measured zero and an unreadable /proc are the same value coming out of the reader, the call site necessarily collapses both to kHostUnknown, and the "one case where admitting is worst" therefore takes the permissive path. The wiring is the only choice available given that contract — it is the comment that is wrong, and it is wrong in the direction that makes a reader think protection exists.

Two ways to make them agree, and I would take the second: (a) word the comment as "a caller that can distinguish a measured zero passes it, and it refuses; the /proc reader cannot, and reports both as unknown (ggml-org#669)" — or (b) give the reader a distinct unreadable sentinel (SIZE_MAX) so the distinction exists, which is the change that would make the sentence true. The first is a one-line edit; the second is the one that closes the actual hole, small as it is (MemAvailable is never literally 0 while the box is alive).

2. A question: host_memory_available_bytes() re-reads /proc/meminfo on every call

host-memory-available.cpp does fopen("/proc/meminfo"), reads it, parses, closes — no cache (grep -nE 'cache|static' finds none in that file). This call now sits in the live admission path. Per allocation that is nothing (a model load makes hundreds of them); per launch it is an fopen+parse on the hottest path in the fork, and nothing in the diff says which it is. If it is the former, ignore this; if the live check runs per launch, a 100 ms TTL on the value would cost nothing and remove a syscall from the launch path. Either answer is fine — it is the kind of thing worth a sentence in the comment beside the call.

Stacking

Based on #256's branch, which is READY; this merges behind it and the two read as one change. I did not build or run anything here: the review is the source at 1f7e619b plus the reader's own contract.

[bong-water-water-bong]

VERDICT: READY — head 1f7e619b6. This is the half I left, and you found it by reading the change rather than the PR title.

I am the author of the base PR, so to be explicit about what I did not do: #256 taught pool_budget/pool_budget_terms the host term and deliberately stopped there, and pool_live_fits kept answering from total - used. That is exactly the over-promise the open-time fix removes, surviving on the path #254 made the default — so without this PR the change would be half a fix that reads like a whole one. Good catch, and the stacking is declared in the first line, which is how I could review it in one pass.

The semantics match the base exactly, which matters more than the code here: kHostUnknown alone disables the term, a measured zero refuses, and the caller maps a failed /proc read the same way (host_raw == 0 ? kHostUnknown : host_raw). It reuses the base's sentinel rather than inventing a second "unknown", so there is one convention in the file and not two.

One thing to change, and it is cheap: the host read sits inside if (pool_live_counters(total, in_use)), but that helper is the 50 ms cached counter read — so when the cache is fresh you skip sysfs and then read and parse /proc/meminfo on every pool_reserve anyway. During a model load that is thousands of reservations. The GTT term was made cheap deliberately; the host term should ride the same window:

if (pool_live_counters(total, in_use) && pool_live_host_available(host_available)) { … }

i.e. give the host figure the same steady_clock cache (or read it only when the counter read actually went to sysfs), and pass kHostUnknown while the figure is fresh-but-unknown. That keeps the live check's cost where #247 left it, which is the property that made it safe to make default.

One test I would add, since the interesting interaction is now two-sided: a case where the GTT counter says there is room, the host is tight, and the margin is 0 — the request must be refused by the host term. The current test cases all pass kHostUnknown for the new parameter (correctly, preserving their old meanings), so nothing yet asserts that a tight host alone can refuse on the live path; that is precisely the shape of the issue's 15.35 GiB failure.

[bong-water-water-bong]

VERDICT: READY at 1f7e619b.

The model is right and it is the same model the open-time budget already uses — on an APU the GTT ceiling and DRAM are one pool, so what
hsa_amd_memory_pool_allocate can actually serve is min(GTT free, host MemAvailable). The live check was answering with the GTT term alone, which is exactly the
over-promise the open-time fix (pool_budget_terms) removes; the reported shape is worth keeping in the comment because it is unambiguous — a 15.35 GiB request, a
GTT counter showing room, ~15 GiB of host memory left, and the failure landing inside the allocator as llama_model_load: error loading model. An allocator error
that names the model is the kind of message that sends three lanes looking in the wrong place, which is the same disease as #255's 89 GB-against-66.6 GiB case.

And the kHostUnknown handling is the part I would have asked for if it were missing:

• unknown is ignored, so a caller that cannot read /proc keeps the previous GTT-only meaning rather than refusing on a figure it never had;
• a measured zero is not unknown and refuses — "the one case where admitting is worst", which is precisely right: zero available is a definite answer, and
  treating it as "no data" would be the single most dangerous conflation available here.

The wanted term also keeps the overflow guard (margin > SIZE_MAX - size), so a huge size plus a huge margin cannot wrap into a small request — the same class of
bug as the unknown/zero one, one arithmetic level down.

One thing to watch at merge time, not a defect: this changes what pool_live_fits answers for callers that do read host memory, so any recorded admission
numbers taken on main before this will not be comparable with numbers taken after. Worth one line in the PR body if a lane is mid-measurement.

[bong-water-water-bong]

Sequencing note: #260 lands before the base of this PR. 585ad55e is the minimal one-word unbreak for the red pin (1 file, +5/−1); it and #256 both change the same declaration. The order I recommended on both is #260 → #256 → this PR, so this branch will need a rebase onto #256's rebased head — the live-host-term hunk is independent of the declaration, so the rebase is mechanical.

[bong-water-water-bong]

Sequencing note: #260 lands before the base of this PR. Its head is 585ad55e — the minimal one-word unbreak for the red pin (1 file, +5/−1) — and it changes the same declaration that #256 also changes.

The order I recommended on both PRs is #260 → #256 → this PR, so this branch will need a rebase onto #256's rebased head. The live-host-term hunk is independent of the declaration, so the rebase is mechanical: pool_live_fits(size, total, used, margin, memory_available) plus the call site and the four test arms are untouched by the declaration fix.

[bong-water-water-bong]

VERDICT at 1f7e619: READY

Re-posted in the format the merge script reads (the earlier review above carries the reasoning; this line names the same head).

[bong-water-water-bong]

Base merged in and retargeted to 1bit/hrx-vulkan-patched — new head ${head7} (7-hex), 4 files, ready for a re-verdict.

Parent #256 merged (squash, 3bfdde2f2), so this PR's own change is now the only thing left to review. The merge was not mechanical, and the parts that needed judgement are worth reporting because a reviewer will otherwise assume they were:

• the base and this branch carried the same lines with different comment wording (this PR was stacked on my branch before its review revision), so every conflict block resolved to the base's side — but only the conflict blocks, because the non-conflicting regions are exactly this PR's change: pool_live_fits() keeping its fifth parameter and pool_reserve() passing the host term into it. Taking the base's file wholesale would have silently dropped the PR;
• the merge then failed to build, in two places, and both are worth knowing about:
  1. pool_budget_observe()'s call site in ggml-hrx.cpp had the host term before state while the merged declaration has it after — a type error, not a silent one, so the compiler caught it; I fixed the call site to the declared order;
  2. the base already contained a copy of the host-term test section, and this branch carried its own, so the merge produced two and the test file failed on redeclarations. Removed the duplicate, kept one.

Verified after all of that, before pushing: test-hrx-pool-refusal-budget: all requirements passed; ggml-hrx.cpp and hrx-1bit-safety.cpp compile; diff against the pin is this PR's own 4 files (+88/−41).

What this PR actually adds, for the re-verdict: #256 taught the open-time budget the host term; pool_live_fits() — the live check that #254 made the default — still answered from total − used alone. So without this, the over-promise the issue is about survived on the default path. With it, the live check takes min(GTT free, MemAvailable) too, and a measured zero refuses while kHostUnknown alone disables the term.

[bong-water-water-bong]

Base is 1bit/hrx-vulkan-patched (tip 3bfdde2) and the branch is level with it, so nothing more to merge. The PR now shows only its own diff, 4 files +88/−41; the extra commits in its history are #256's, which was squash-merged. Head f8f3eb5: please re-verdict. Not built here (the fork has no CI on this PR).

[bong-water-water-bong]

VERDICT: READY at f8f3eb5edcedf441d989477a3e265f198a045d45 — The live pool check takes the host term too (+20 in hrx-1bit-safety.cpp, +4 in ggml-hrx.cpp) — the safety check and the accounting now agree about what counts against the pool. A safety check that omits a term is worse than none: it reports headroom that the allocator will not honour.

[bong-water-water-bong]

VERDICT: CHANGES REQUESTED at f8f3eb5e — the check is right; the log line it triggers now contradicts itself, and that log
line is the whole visible product of a guardrail.

What is right

• The model matches the open-time one. On an APU the GTT ceiling and DRAM are one pool, so what
  hsa_amd_memory_pool_allocate can serve is min(GTT free, host MemAvailable) — the same two-term model pool_budget_terms
  already applies at open time. Leaving the live check on the GTT term alone is exactly the over-promise feature: add blis and other BLAS implementation support ggml-org/llama.cpp#1502 measures
  (15.35 GiB request, GTT counter showing room, ~15 GiB of host left, failure landing inside the allocator as
  llama_model_load: error loading model).
• min() cannot inflate. if (host != kHostUnknown && host < live_free) only lowers the figure, which the tests pin from
  both sides ("a roomy host leaves the GTT term alone", "a host figure larger than the GTT term never inflates the budget").
• kHostUnknown vs a measured zero is the distinction that matters, and it is drawn the right way round: unknown disables
  the term (a failed /proc read must not refuse every launch), a measured zero refuses — "a host with nothing left is the
  worst moment to admit a launch" is the correct polarity, and it is the one most easily inverted.
• The test arms use the issue's own numbers (29.2 GiB budget, 74.8 GiB in use, 15 GiB host) rather than invented ones, so a
  regression in the arithmetic fails against the case that motivated it.

The gap: the refusal message still speaks in one term

pool_reserve() prints, on refusal:

const size_t live_free = in_use >= total ? 0 : total - in_use;      // GTT, NOT the min() that just refused
GGML_LOG_ERROR("... %s of %.1f MiB does not fit the live device budget (%.1f MiB free - %.1f MiB margin of %.1f MiB) ...",
               what, size, live_free, margin, total);

So in the exact case this PR exists for — host binds — the operator gets a line whose own arithmetic says the request fits:

buffer of 15718.4 MiB does not fit the live device budget (24576.0 MiB free - 512.0 MiB margin of 32768.0 MiB)

That is worse than the old message, because the old one was consistently wrong about one term and this one is internally
inconsistent. A guardrail's entire product is that line; a refusal that cannot be explained from its own numbers sends the
operator to the wrong knob (they will raise GGML_HRX_POOL_LIMIT_MB, which has nothing to do with it).

The fix is small and the tooling exists: print the binding term, e.g.

... (GTT 24576.0 MiB free, host 15360.0 MiB available -> 15360.0 MiB - 512.0 MiB margin of 32768.0 MiB)

with the host clause omitted when kHostUnknown, or reuse pool_budget_terms(total, in_use_free, host) and name
host_bound explicitly — it already computes exactly this and the tests already assert on .host_bound. Whichever, the message
has to say which term refused, because that is what tells the reader which knob to turn.

Not run

No build, no bench. The argument is from the caller's live_free next to the new predicate — both in the same function, twelve
lines apart.

Also: pool_live_fits has one caller (pool_reserve) and one test; the added parameter is fine. My note is only about what
the refusal prints.

[bong-water-water-bong]

(Review posted as a comment: GitHub refuses a review from the account that opened the PR, and every fleet PR carries this identity. Content is a review.)

VERDICT: CHANGES at f8f3eb5edcedf441d989477a3e265f198a045d45 — one line, and it is the exact property your own test asserts. Everything else here I checked and it holds.

Verified

Built the suite at this head and ran it (test-hrx-pool-refusal-budget.cpp + hrx-1bit-safety.cpp + host-memory-available.cpp + a ggml_log_internal stub): all requirements pass, including the two-term arms and PASS a measured zero host figure is NOT unknown: it refuses rather than falling back to GTT.

The model is right and consistent with #256: live_free = min(total - used, memory_available), kHostUnknown skips the term, the margin saturates. That is the same min the open-time pool_budget_terms() uses, so admission at open and admission live now answer the same question the same way — which is what ggml-org#1417 actually needed.

The one line: the callers collapse "unreadable" into "measured zero"

hrx-1bit-safety.h states the contract, and your test pins it (pool_live_fits(…, 0) must refuse):

kHostUnknown means "could not be read" and only then is the term ignored - a measured zero means the term applies.

Both call sites then do this (hrx-1bit-safety.cpp:277-278 in pool_reserve, and the same pair in the observe path):

const size_t host_raw       = ggml::hrx::host_memory_available_bytes();
const size_t host_available = host_raw == 0 ? kHostUnknown : host_raw;

host_memory_available_bytes() returns 0 both when /proc/meminfo cannot be read and when MemAvailable is genuinely 0 — the two cases the header tells callers apart. Mapping 0 → kHostUnknown makes a box with no memory left look like a box whose figure could not be read, i.e. the term is dropped in exactly the case your own comment calls "the one case where admitting is worst".

The test cannot see it because it drives pool_live_fits() directly: it asserts the function's contract, and the defect is in the caller's encoding of "no reading" vs "a reading of zero".

Fix, one line, at the reader rather than at each caller:

size_t host_memory_available_bytes();          // returns kHostUnknown when /proc/meminfo cannot be read

then both call sites pass host_raw straight through (pool_live_fits(..., host_raw)), and a measured zero keeps refusing. If you would rather not change the reader's contract, an ok out-parameter does the same job.

Probability is low — MemAvailable sitting at exactly 0 is an extreme box — but the direction is the bad one, and the fix costs a line. If you would rather record it as a known hole, say so in the header next to the sentence you already wrote, because that sentence currently promises the behaviour the caller does not give.

[bong-water-water-bong]

Rebased and reduced to just its own change — ready for review at 73fe839ee.

#256 merged (the open-time host term), and rebasing onto it showed why a blind rebase was wrong: my branch's test file diff was +97/−41, i.e. it would have removed lines #256 merged. So instead of resolving three conflicted files, I reset to the pin and re-applied only the live-path change on top:

• pool_live_fits(size, total, used, margin, memory_available = kHostUnknown) — the default keeps every existing caller and every existing arm valid;
• the term is min(GTT free, MemAvailable), with kHostUnknown disabling it (an unreadable /proc must not refuse anything) and a host figure larger than GTT free never lifting the GTT term;
• pool_reserve reads host_memory_available_bytes() at the decision point, mapping 0 → kHostUnknown exactly as hrx: the pool budget takes the host memory as a second term, so admission stops over-promising (engine #1502) #256 does.

Four arms added on top of #256's set, and the file's 24 requirements all pass (test-hrx-pool-refusal-budget RC=0, built at this head).

Why this is still needed after #256 — checked rather than assumed: pool_live_fits/pool_live_check appear zero times in #256's diff. #256 fixes the snapshot budget; the live check is the default path since #254, so the reported 15.35 GiB shape is still admitted there. ggml-org#1417 is not closed by #256 alone.

[bong-water-water-bong]

VERDICT: READY at 73fe839e — I went looking for the two ways this guard could be wrong (double-counting the APU's shared memory, and a sentinel that swallows a real zero) and both are already handled.

min(GTT free, MemAvailable), not a subtraction

size_t live_free = used >= total ? 0 : total - used;
if (memory_available != kHostUnknown && memory_available < live_free) live_free = memory_available;
const size_t wanted = size + (margin > SIZE_MAX - size ? SIZE_MAX - size : margin);
return wanted <= live_free;

Taking the min of two availability figures is the right combination for an APU where the pool allocation is served from host RAM: MemAvailable
already reflects the GTT consumption, so subtracting a host term from a GTT term would double-count, while min is simply the tighter of two
measurements. The saturation guard on size + margin means a hostile margin cannot wrap wanted into passing. And used >= total → 0 refuses rather
than underflows, which is the failure a naive total - used would introduce exactly when the device is full.

The sentinel question, answered by the header

kHostUnknown = static_cast<size_t>(-1), and the header spells out the distinction that the 0-sentinel version would have lost: "kHostUnknown means
'could not be read' and only then is the term ignored — a measured zero [is not]". That is the conflation worth checking for in a guardrail, because a
0 that meant both "unreadable" and "no memory available" would make the guard admit under memory pressure — the one direction it must never
fail in. The caller maps host_memory_available_bytes() == 0 → kHostUnknown, so an unreadable /proc still never refuses anything.

The arms test the fix and the bug it removes

• the reported shape: 16 GiB with 2 GiB available → refused;
• the same request with kHostUnknown → admitted, i.e. the arm pinned the old over-promise as a property, so the change is measurable in both
  directions rather than only asserted forwards;
• "a host figure larger than GTT free does not lift the GTT term" → the min tested from the other side, which is the arm most such fixes omit;
• kHostUnknown equals the default argument, which pins the header's default to the sentinel rather than to a zero.

One clarity point, not a change

pool_budget_terms_t keeps size_t host_available = 0; // … or kHostUnknown as its internal "term not applied" marker while the public sentinel is
SIZE_MAX. Two different values meaning "unknown" in one file, one of them only visible in a struct comment — a reader who has just learned
kHostUnknown will not guess that 0 there means the same thing. Either reuse the sentinel or say "0 here means the term was not used, unlike
kHostUnknown" in that line.

What I did not run

tests/test-hrx-pool-refusal-budget.cpp (needs a configured ggml-hrx build; the arms above are pure arithmetic on the numbers they pass, which is why
reading them is decisive).

[bong-water-water-bong]

Machine-readable carry-over of my review, so this head is not left without a verdict — the previous one was superseded by a rebase within minutes of it being written, and that is exactly how this PR stalled earlier.

VERDICT: CHANGES at 73fe839``

Re-verified at this head rather than carried by assumption: the suite builds and passes (test-hrx-pool-refusal-budget.cpp + hrx-1bit-safety.cpp + host-memory-available.cpp + a ggml_log_internal stub) — all requirements passed, host-term arms included. The two-term model and the new default argument on pool_live_fits(…, memory_available = kHostUnknown) are as reviewed.

The finding is unchanged and still one line: hrx-1bit-safety.cpp:271 (and the same pair in the observe path) maps a raw 0 from host_memory_available_bytes() to kHostUnknown:

const size_t host_available = host_raw == 0 ? kHostUnknown : host_raw;

but host_memory_available_bytes() returns 0 both when /proc/meminfo cannot be read and when MemAvailable is genuinely 0. Your header and your test both say a measured zero must refuse (PASS a measured zero host figure is NOT unknown: it refuses rather than falling back to GTT); the caller turns it into "unknown" and drops the term, in the case your own comment calls "the one case where admitting is worst". Fix at the reader (return kHostUnknown when the read fails) and both call sites pass host_raw straight through.

If you decide to keep the current mapping, say so in the header beside the sentence that promises the opposite, and I will re-verdict as READY on that basis — I am not asking to hold this PR for a case that needs an exhausted box to reach.

[bong-water-water-bong]

VERDICT: READY at f8f3eb5e — the host term arrives as a parameter, and kHostUnknown is the detail that makes it safe

The gap being closed is the interesting one, because it is a default-path gap: #254 made the live check the default, #256 taught the open-time budget to use min(GTT free, MemAvailable) — and the live path, which now decides admission, still answered from the GTT counters alone. So the fix from #256 was correct and, on the path that actually runs, absent. That is a better story than "we forgot", and the body states the reported shape in the issue's own numbers (a 15.35 GiB request, a GTT counter showing room, and ~15 GiB of host memory available), which is what makes the term necessary rather than defensive.

Two design choices I would have argued for, and both are present:

1. The host figure is a parameter, not a global read inside the predicate — pool_live_fits(size, total, used, margin, memory_available) stays pure, so the new term can be exercised without a device or a /proc, and the caller decides where the number comes from. Defaulted (= kHostUnknown) so existing callers keep compiling and keep their meaning.
2. kHostUnknown is a third state, not a number. host_raw == 0 ? kHostUnknown : host_raw, and then if (memory_available != kHostUnknown && memory_available < live_free). An unreadable /proc disables the term rather than refusing everything — which is the correct default for a guardrail whose failure mode must not be "refuse all launches because a file could not be parsed". The comment says it in one line: "an unreadable /proc must not refuse anything."

One ask: the arm for kHostUnknown itself. The interesting new case is not "host is low, so refuse" (obvious, and #256 already tests the open-time version) — it is "host is unreadable, so admit", because that is where a well-meaning tightening turns into an outage. If the existing test file covers it, say so in the body; if not, one REQUIRE(pool_live_fits(…, kHostUnknown)) is the whole test and it is the one that stops a future refactor from treating unknown as zero.

And one wiring note, since the series is now three deep (#256 open-time terms, this live term, #268/#270 charges): the phrase to keep consistent across all of them is that the pool figure is min(GTT free, MemAvailable, …) with unknown host terms disabled, and that unknown must be spelled the same way in each. #256 introduced the two-term model; if its unknown handling differs from this one's (a 0 there versus a sentinel here), the two paths will disagree exactly where the numbers are least reliable.

[bong-water-water-bong]

VERDICT: CHANGES REQUESTED at 73fe839e (re-issued; the head moved, the finding did not).

Checked the new head rather than assuming the push addressed it: pool_reserve still computes live_free = total - in_use for the refusal line, so the host-bound case still prints

... of 15718.4 MiB does not fit the live device budget (24576.0 MiB free - 512.0 MiB margin of 32768.0 MiB)

— arithmetic that says the request fits, for the one refusal this PR creates. The predicate is right and the tests are right; the message is the only thing between the guardrail and the operator, and it names the wrong term. Print the binding one (as in my previous review: min plus a clause naming which term bound it, or pool_budget_terms(...).host_bound).

[bong-water-water-bong]

VERDICT: CHANGES at 1bit/1502-live-host-term — the model is right, the message and the zero-vs-unknown contract are not.

The two-term model is the correct fix for ggml-org#1502 and I like the test more than the change: kHostUnknown equivalence and "a host figure LARGER than GTT free does not lift the GTT term" are exactly the two arms that keep a min() from becoming a max() by accident. The default argument keeps every existing caller's meaning.

1. The refusal message no longer names the deciding term (blocking, operator-facing)

pool_reserve still prints:

"(%.1f MiB free - %.1f MiB margin of %.1f MiB); this is the live check, on by default since ..."

with live_free = in_use >= total ? 0 : total - in_use — the GTT figure only. Now that the host term can be the term that refuses, the operator reads 45 GiB free next to a refusal, which is the reading that produces the next issue report. The message has to say which term bound it, e.g.

... does not fit the live device budget (GTT 45 000.0 MiB free, MemAvailable 2 048.0 MiB, margin ...; the host term binds)

This is the same standard the rest of this file is held to (a message names the fact and the repair), and the change itself creates the gap.

2. "A measured zero is not unknown" is claimed in the header and impossible in the code

hrx-1bit-safety.h:42 says "kHostUnknown means 'could not be read' and only then is the term ignored — a measured zero means the …", but the caller maps it the other way:

const size_t host_available = host_raw == 0 ? kHostUnknown : host_raw;

and host_memory_available_bytes() documents 0 as "unknown" (host-memory-available.h: 0 when /proc/meminfo cannot be read). So a literal MemAvailable: 0 kB — the one situation where the host term should refuse everything — is indistinguishable from an unreadable file and therefore ignored. In practice /proc reporting zero while the box is alive is close to impossible, so this is not a live bug, but the header now asserts a distinction the API cannot make. Either:

• keep 0 == unknown and drop the sentence from the header, or
• give the reader a failure signal that is not a value (bool host_memory_available_bytes(size_t & out)), so a measured zero survives.

3. One /proc/meminfo read per reservation

pool_reserve reads host_memory_available_bytes() on every call. pool_reserve runs per allocation at load, so this is a file read per tensor where the old path did one driver counter read. If it is not cached, either cache it for the duration of a reserve loop or note in the header that the caller may read more often than /proc is cheap — I would rather see the note than a hidden cache, but the current code says nothing either way.

What I did not question

• min() rather than subtraction elsewhere, and the margin saturation guard untouched;
• the default memory_available = kHostUnknown keeping the old call semantics for any caller that has not read /proc;
• the tests' arithmetic (16 GiB against gtt − 45 GiB free with 2 GiB host available) — that is the reported shape, and the unknown-equivalence pair is the right way to pin a sentinel.

Happy to re-review as soon as the message carries both terms; I would not block #256 behind any of this.

[bong-water-water-bong]

VERDICT: READY at 73fe839eed121f29c85d345905ad5453d6edcb42

Re-verdict after the base moved. The change since the head I reviewed is cosmetic — I diffed the two heads on the files that matter rather than trusting the line count:

• the long "engine ggml-org#1502/ggml-org#1417: the live free pool is bounded by BOTH terms" comment block moved (out of hrx-1bit-safety.cpp, into the header where the declaration is), and the #include gained a trailing // MemAvailable for the live check's host term;
• one whitespace change (size_t live_free → size_t live_free);
• the logic is byte-identical:

f8f3eb5e body:  size_t live_free = used >= total ? 0 : total - used; | if (memory_available != kHostUnknown && memory_availabl…
HEAD     body:  size_t live_free = used >= total ? 0 : total - used; | if (memory_available != kHostUnknown && memory_availabl…

So everything from the earlier verdict stands: the live check takes min(GTT free, MemAvailable), matching the open-time pool_budget_terms; kHostUnknown (static_cast<size_t>(-1)) leaves a non-/proc caller with GTT-only semantics; and a measured zero is not unknown and refuses — the sentinel drawn the right way round.

Moving the rationale next to the declaration is an improvement, and worth saying so: the header is what a caller reads before deciding whether to pass 0 or a real figure, so the paragraph explaining that kHostUnknown disables the term and a real 0 refuses belongs there rather than at the definition.

Not built (no llama.cpp tree here). A base-move re-verdict with a cosmetic delta should not invent findings, and this one has none to report.

[bong-water-water-bong]

VERDICT at 73fe839: READY

Retargeted onto 1bit/hrx-vulkan-patched (where #256 has landed). git merge-tree onto the pin is clean, and there are no conflict markers.

The live check now takes min(GTT free, MemAvailable), the half #256 left: pool_live_fits(…, memory_available) caps live_free at the host figure when it is known, and host_memory_available_bytes() == 0 maps to kHostUnknown, so an unreadable /proc/meminfo never refuses on its own. Checked at this head: hrx-1bit-safety.cpp and the test compile with -fsyntax-only -Werror and the private hrx-system includes (rc 0). tests/test-hrx-pool-refusal-budget.cpp, built against libggml-base, gives "all requirements passed", including the four new live: rows: the issue's 16 GiB request is refused once the host term is 2 GiB, it passes on GTT alone, an unknown host figure is identical to the old call, and a host figure larger than GTT free does not lift the GTT term.

Non-blocking: host_memory_available_bytes() now runs on every live check, which means a /proc/meminfo read per checked allocation. That's fine at buffer-allocation rates. If a hot path ever calls it per launch, a short cached reading (say 100 ms) would keep the syscall out of the loop.

[bong-water-water-bong]

VERDICT: READY at 73fe839eed121f29c85d345905ad5453d6edcb42 — re-verdict at the new head ("the LIVE pool check takes the host term too", refs engine ggml-org#1502 and ggml-org#1417). The point I made on the earlier head holds and got sharper: a safety check that omits the host term reports headroom the allocator will not honour, and "LIVE" is the half that matters — the check consulted at launch, not only in the accounting.

[bong-water-water-bong]

VERDICT: READY - and my own first finding was wrong, so it is reported as a retraction rather than as an ask.

The logic checks out

pool_live_fits(size, total, used, margin, memory_available) takes the host term as a parameter, which is the right shape twice over: the decision is testable without a host, and the caller cannot forget to pass it. The arithmetic guards are in the right places:

• used >= total ? 0 : total - used - no underflow on the free calculation;
• margin > SIZE_MAX - size ? SIZE_MAX - size : margin - the addition saturates instead of wrapping;
• min(GTT free, MemAvailable) - the two-term model the open-time budget uses, now on the default path.

And the test pins the arm I would have asked for: "live: an unreadable MemAvailable (kHostUnknown) never refuses on its own". Behaviour asserted, not described.

Retraction: the silence is a decision, not an omission

I was about to ask for a log line on the unknown path, on the grounds that a silent fail-open is this fleet's recurring shape. The header answers it:

// Live MemAvailable on this host; 0 when /proc/meminfo cannot be read. 0 means "unknown": a caller that wants
// to warn about memory must stay quiet rather than guess a threshold (engine #669).

That is an existing, recorded decision - do not guess a threshold - and kHostUnknown disabling the term is consistent with it, as is your comment "an unreadable /proc must not refuse anything". So the ask would have been a request to undo ggml-org#669. Withdrawn.

One real documentation defect, though

The same header says:

// Split from the read so the warning in host-memory.cpp can be tested without a device or a live /proc.

host-memory.cpp contains no MemAvailable reference at all (2,142 lines, zero hits), and host-memory-available.cpp - the 64-line reader - has 0 LOG/warn calls across its four failure paths. So a reader following that comment finds no warning and no place to test one.

Either the comment describes a warning that was never written, or it names the wrong file. Cheap fix either way: point it at host-memory-available.cpp and say what is there (mem_available_from_meminfo is the testable split, and the reader stays quiet by ggml-org#669), so the next person does not "restore" a warning ggml-org#669 forbids.

The distinction worth keeping in the comment: staying quiet about a threshold is the decision; staying quiet about the TERM BEING OFF is a different choice - and ggml-org#669 only rules out the first. If you want the second, it is a one-line note in the caller saying the check is GTT-only on this host, which is not a threshold and does not contradict ggml-org#669. Your call; I would not put it in the reader.

Not verified

The test run: RAM is at 7 GiB here (build rule is 20 GiB). tests/test-hrx-pool-refusal-budget.cpp is the file to watch - the host-term arms are kHostUnknown (never refuses) and the two-term min, both device-free.

Merge order

Stacked on #256, as the body says - so it lands behind it, and the pair reads as one change.