From b314d767e1ac322522f2962fdfd29b2f9af45990 Mon Sep 17 00:00:00 2001 From: agent Date: Sun, 8 Nov 2026 23:25:05 -0400 Subject: [PATCH 1/3] hrx: the pool budget takes the host's memory as a second term, so admission stops over-promising (refs engine #1502) Two parts, the first small and the second the fix. 1. The pin did not compile: the forward declaration #252 added is `static void device_free_total(...)` while the definition at :854 is `static bool`. That is an ambiguous redeclaration, so the translation unit fails at `ggml-hrx.cpp:518` and every lane that builds this pin loses the same minutes. One word, and the reason written down where the next person edits it. 2. The budget was `(total - reserve) - in_use`, i.e. the sysfs GTT counter and nothing else, so the first attempt at a load was admitted on an optimistic number. Measured (engine #1502): in use 74.8 GiB gave a 29.2 GiB budget and the load was admitted, and `hsa_amd_memory_pool_allocate` then refused a 15.35 GiB buffer with OUT_OF_RESOURCES - which surfaced as `llama_model_load: error loading model`, so callers reported a broken workload instead of no room (two Hyperloom measurements scored 0.0 that way). The same issue has two loads an hour apart with identical guardrail input (in use 74.8, budget 29.2 GiB both times) and opposite outcomes, which no number on that line can explain. The term that does explain it is the host's: on an APU the 120 GiB GTT ceiling and the 122 GiB of DRAM are one pool, so what the allocator can serve is bounded by MemAvailable as well as by the counter - and MemAvailable is the only one of the four that is observable (ROCr exposes no AVAILABLE_ALLOC; that probe is in the issue). So pool_budget() now returns min(GTT term, MemAvailable), the same two-term model the engine's own admission check has always used (app/mem_admit.h), which is why the planner and this safety net could disagree by ~14 GiB. 0 ("unknown") keeps the old arithmetic. The guardrail line prints both terms, so the next refusal says which one bound it. The refusal-feedback machinery from #249/#251 is untouched and composes: a cap is still evidence about the GTT pool, and it can only lower a budget that is now honest underneath it. pool_budget_observe() takes the host term so a lifted cap cannot restore an optimistic figure while the host is still full. Proof, offline and deterministic: tests/test-hrx-pool-refusal-budget.cpp keeps every pre-existing assertion (its call sites now pass the host term as 0, i.e. exactly the old behaviour) and adds the host-term section - the issue's 29.2 GiB GTT arithmetic is reproduced, a tight host replaces it, so the refused 15.35 GiB is no longer admissible while the GTT-only arithmetic still called it admissible; a roomy host leaves the budget identical; a host figure larger than the GTT term never inflates it; a lifted cap stays bounded by the host term. 22 assertions, all pass. scope: the budget's estimate at device open and at each observation. Not claimed: that any particular load now succeeds - it makes the refusal land at admission with the binding term named, instead of inside hsa_amd_memory_pool_allocate and attributed to the model. --- ggml/src/ggml-hrx/ggml-hrx.cpp | 25 ++++++++++++------ ggml/src/ggml-hrx/hrx-1bit-safety.cpp | 19 ++++++++++---- ggml/src/ggml-hrx/hrx-1bit-safety.h | 8 ++++-- tests/test-hrx-pool-refusal-budget.cpp | 35 +++++++++++++++++++++++--- 4 files changed, 69 insertions(+), 18 deletions(-) diff --git a/ggml/src/ggml-hrx/ggml-hrx.cpp b/ggml/src/ggml-hrx/ggml-hrx.cpp index 705dcc54155a..0d854a74177d 100644 --- a/ggml/src/ggml-hrx/ggml-hrx.cpp +++ b/ggml/src/ggml-hrx/ggml-hrx.cpp @@ -2,6 +2,7 @@ #include "dispatch_registration/common/moe-placement-guard.h" #include "ggml-hrx.h" #include "hrx-1bit-safety.h" +#include "runtime/host-memory-available.h" #include "ggml-hrx-dmabuf.h" #include "backend-buffer-binding.h" @@ -380,7 +381,10 @@ static const ggml_backend_buffer_i buffer_i = { // engine #1502 (#251) calls this from buffer_alloc, eight hundred lines above its definition, with no declaration in // scope - so the translation unit does not compile ("'device_free_total' was not declared in this scope"). One forward // declaration is enough; the definition and its callers are unchanged. -static void device_free_total(ggml_backend_hrx_device_context * device_ctx, size_t & free_bytes, size_t & total_bytes); +// ...and the declaration must match the definition's type: it is `bool` (it reports whether the driver counters were +// available), so a `void` forward declaration is an ambiguous redeclaration and the translation unit still does not +// compile - the pin built for nobody until this line said `bool`. +static bool device_free_total(ggml_backend_hrx_device_context * device_ctx, size_t & free_bytes, size_t & total_bytes); static ggml_backend_buffer_t buffer_alloc(ggml_backend_buffer_type_t buft, size_t size) { ggml::hrx::onebit::poison_selftest(); // no-op unless GGML_HRX_POISON_SELFTEST=1 @@ -884,8 +888,9 @@ static void device_memory(ggml_backend_dev_t device, size_t * free, size_t * tot std::lock_guard lock(device_ctx->memory_budget_mutex); ggml::hrx::onebit::pool_budget_state_t state{ device_ctx->memory_budget, device_ctx->memory_budget_cap, device_ctx->memory_budget_cap_free }; - state = ggml::hrx::onebit::pool_budget_observe(device_ctx->memory_total, free_bytes, - ggml::hrx::onebit::pool_allocated(), state); + state = ggml::hrx::onebit::pool_budget_observe( + device_ctx->memory_total, free_bytes, ggml::hrx::onebit::pool_allocated(), + ggml::hrx::host_memory_available_bytes(), state); device_ctx->memory_budget = state.budget; device_ctx->memory_budget_cap = state.cap; device_ctx->memory_budget_cap_free = state.cap_free; @@ -1582,7 +1587,9 @@ static std::unique_ptr create_registry_context() { size_t device_free = device_ctx->memory_total; size_t device_total = device_ctx->memory_total; device_free_total(device_ctx.get(), device_free, device_total); - device_ctx->memory_budget = ggml::hrx::onebit::pool_budget(device_ctx->memory_total, device_free); + const size_t host_available = ggml::hrx::host_memory_available_bytes(); + device_ctx->memory_budget = + ggml::hrx::onebit::pool_budget(device_ctx->memory_total, device_free, host_available); if (device_ctx->memory_budget == 0 && device_ctx->memory_total != 0) { // Nothing fits under the ceiling: refuse the device here, loudly, instead of planning a launch that can // reach the fault line (#680). Report only, do not try to be clever about it. @@ -1599,13 +1606,17 @@ static std::unique_ptr create_registry_context() { if (device_ctx->memory_budget != 0) { const size_t reserve = ggml::hrx::onebit::pool_reserve_bytes(); GGML_LOG_WARN("ggml_hrx: %s guardrail: GTT total %.1f GiB, in use %.1f GiB, ceiling %.1f GiB " - "(reserve %.1f GiB), pool budget %.1f GiB - a launch that needs more is refused here " - "rather than faulting the device (engine #680)\n", + "(reserve %.1f GiB), pool budget %.1f GiB (GTT term %.1f GiB, host MemAvailable %.1f GiB) - a " + "launch that needs more is refused here rather than faulting the device (engine #680)\n", device_ctx->name.c_str(), device_total / (1024.0 * 1024.0 * 1024.0), (device_total > device_free ? device_total - device_free : 0) / (1024.0 * 1024.0 * 1024.0), (device_total > reserve ? device_total - reserve : 0) / (1024.0 * 1024.0 * 1024.0), reserve / (1024.0 * 1024.0 * 1024.0), - device_ctx->memory_budget / (1024.0 * 1024.0 * 1024.0)); + device_ctx->memory_budget / (1024.0 * 1024.0 * 1024.0), + ((device_ctx->memory_total > reserve ? device_ctx->memory_total - reserve : 0) - + (device_ctx->memory_total > device_free ? device_ctx->memory_total - device_free : 0)) / + (1024.0 * 1024.0 * 1024.0), + host_available / (1024.0 * 1024.0 * 1024.0)); } device_ctx->description = *name + " (" + *architecture + ")"; device_ctx->architecture = *architecture; diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp index e188bb154c95..c97804daa988 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp @@ -83,7 +83,7 @@ bool gtt_counters(size_t & total, size_t & used) { return false; } -size_t pool_budget(size_t memory_total, size_t memory_free) { +size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available) { const char * env = std::getenv("GGML_HRX_POOL_LIMIT_MB"); if (env != nullptr) { const long long mb = std::atoll(env); @@ -97,7 +97,15 @@ size_t pool_budget(size_t memory_total, size_t memory_free) { const size_t reserve = pool_reserve_bytes(); const size_t ceiling = memory_total > reserve ? memory_total - reserve : 0; const size_t in_use = memory_total > memory_free ? memory_total - memory_free : 0; - return ceiling > in_use ? ceiling - in_use : 0; + const size_t by_gtt = ceiling > in_use ? ceiling - in_use : 0; + // engine #1502: the GTT counter is not the allocator's ceiling on an APU. Letting a 15.35 GiB buffer through a 29.2 GiB + // budget while the host had ~15 GiB left made the failure land inside hsa_amd_memory_pool_allocate and read as + // "llama_model_load: error loading model" (two Hyperloom measurements scored 0.0 that way). The engine's admission check has + // always used min(GTT free, MemAvailable) (app/mem_admit.h); this is the same two-term model in the fork's safety net. + if (memory_available != 0 && memory_available < by_gtt) { + return memory_available; + } + return by_gtt; } // engine #1417: the budget above is a snapshot taken at device open, and the ledger is per-process, so another process @@ -194,14 +202,15 @@ pool_budget_state_t pool_budget_note_refusal(pool_budget_state_t state, size_t s return state; } -pool_budget_state_t pool_budget_observe(size_t memory_total, size_t free_bytes, size_t allocated, pool_budget_state_t state) { +pool_budget_state_t pool_budget_observe(size_t memory_total, size_t free_bytes, size_t allocated, size_t memory_available, + pool_budget_state_t state) { if (state.cap == 0) { return state; // no cap in force: the registration-time budget stands } if (free_bytes > state.cap_free) { // The pool has grown past the figure the refusal saw, so the cap's evidence no longer holds: a transient refusal must not // become a permanent ceiling (engine #1502 review). Recompute from the current figures and drop the cap. - const size_t uncapped = pool_budget(memory_total, free_bytes + allocated); + const size_t uncapped = pool_budget(memory_total, free_bytes + allocated, memory_available); GGML_LOG_INFO("%s: the pool grew from %.1f MiB to %.1f MiB free since the last refusal; lifting the %.1f MiB cap to " "%.1f MiB\n", __func__, state.cap_free / (double) kMiB, free_bytes / (double) kMiB, state.cap / (double) kMiB, @@ -213,7 +222,7 @@ pool_budget_state_t pool_budget_observe(size_t memory_total, size_t free_bytes, } // The pool is no larger than it was at the refusal: keep the cap AND its evidence, and bound only this call's budget by it. // Clearing it here would freeze the low reading for the rest of the process (engine #1502 review). - const size_t capped = pool_budget(memory_total, free_bytes + allocated); + const size_t capped = pool_budget(memory_total, free_bytes + allocated, memory_available); state.budget = state.cap < capped ? state.cap : capped; return state; } diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.h b/ggml/src/ggml-hrx/hrx-1bit-safety.h index 45d4e2139fb3..40a9588aab6b 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.h +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.h @@ -28,7 +28,10 @@ namespace ggml::hrx::onebit { // budget is the GTT ceiling (memory_total minus GGML_HRX_GTT_RESERVE_MB, default 16384) less what other processes // already hold (memory_total - memory_free). A GTT plan past this budget is refused at allocation instead of // faulting the device: #680 is a gfxhub page fault plus a MODE2 reset at GTT 106.5 of 120 GiB. -size_t pool_budget(size_t memory_total, size_t memory_free); +// |memory_available| is the host's MemAvailable (0 = unknown, which keeps the GTT-only arithmetic). On an APU the GTT ceiling +// and DRAM are the same pool, so what the allocator can serve is bounded by both: this is the term that differs between two loads +// with identical GTT counters and opposite outcomes (engine #1502). +size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available); // The budget after an allocation the allocator refused, given the size it refused (engine #1502). // @@ -64,7 +67,8 @@ pool_budget_state_t pool_budget_note_refusal(pool_budget_state_t state, size_t s // pool_remaining() subtracts the ledger again further down the admission path; // - a cap is cleared only when it is LIFTED (the pool grew past the figure the refusal saw). When the pool is smaller the cap and // its evidence are kept, and only this call's budget is bounded by it - otherwise one low reading becomes a permanent ceiling. -pool_budget_state_t pool_budget_observe(size_t memory_total, size_t free_bytes, size_t allocated, pool_budget_state_t state); +pool_budget_state_t pool_budget_observe(size_t memory_total, size_t free_bytes, size_t allocated, size_t memory_available, + pool_budget_state_t state); // The reserve the ceiling keeps for the driver and for kernel/transient spikes, in bytes. size_t pool_reserve_bytes(); diff --git a/tests/test-hrx-pool-refusal-budget.cpp b/tests/test-hrx-pool-refusal-budget.cpp index e0ebdb46cda9..b75b3e3cdae4 100644 --- a/tests/test-hrx-pool-refusal-budget.cpp +++ b/tests/test-hrx-pool-refusal-budget.cpp @@ -88,7 +88,7 @@ int main() { expect_eq("cap after refusing 80 GiB is size-1", st.cap, 80 * GiB - 1); // The other process now holds 1 GiB LESS than at the refusal, so the cap is lifted. - st = pool_budget_observe(total, free_at_refusal + GiB, our_mem, st); + st = pool_budget_observe(total, free_at_refusal + GiB, our_mem, 0, st); expect_eq("lifting the cap restores the registration budget (own memory counted once)", st.budget, 85 * GiB); expect_eq("the cap is cleared when it is lifted", st.cap, 0); @@ -96,7 +96,7 @@ int main() { pool_budget_state_t st2; st2.budget = 30 * GiB; st2 = pool_budget_note_refusal(st2, 40 * GiB, total - 45 * GiB); // refusal seen with 75 GiB free - st2 = pool_budget_observe(total, total - 90 * GiB, our_mem, st2); // other process grew by 45 GiB + st2 = pool_budget_observe(total, total - 90 * GiB, our_mem, 0, st2); // other process grew by 45 GiB expect_eq("a shrinking pool keeps the cap (it is not cleared)", st2.cap, 30 * GiB); expect_eq("a shrinking pool keeps the cap's evidence", st2.cap_free, total - 45 * GiB); expect_true("a shrinking pool never ends up above the cap", st2.budget <= st2.cap); @@ -107,9 +107,36 @@ int main() { st3.budget = b_small; st3 = pool_budget_note_refusal(st3, 4 * GiB + 700 * MiB, 20 * GiB); expect_true("a 4.7 GiB model refused at a 13.6 GiB budget is capped below its size", st3.cap < 4 * GiB + 700 * MiB); - st3 = pool_budget_observe(total, 60 * GiB, 0, st3); // the pool frees + st3 = pool_budget_observe(total, 60 * GiB, 0, 0, st3); // the pool frees expect_true("when the pool frees the same model is admissible again", st3.budget > 4 * GiB + 700 * MiB); - expect_eq("no cap in force: an observation changes nothing", pool_budget_observe(total, 10 * GiB, 0, pool_budget_state_t{20 * GiB, 0, 0}).budget, 20 * GiB); + expect_eq("no cap in force: an observation changes nothing", pool_budget_observe(total, 10 * GiB, 0, 0, pool_budget_state_t{20 * GiB, 0, 0}).budget, 20 * GiB); + + // ---- the host term (this lane's half of #1502) ------------------------------------------------------------------ + // + // On an APU the GTT ceiling and DRAM are one pool, so the allocator is bounded by the host as well as by the GTT counter. + // The issue's measured case: budget 29.2 GiB from the counters, while /proc/meminfo had roughly 15 GiB left - and the pool + // then refused 15.35 GiB, failing at model load instead of at admission. + using ggml::hrx::onebit::pool_budget; + const size_t issue_budget = 29 * GiB + 205 * MiB; // 29.2 GiB, as printed by the guardrail + const size_t issue_free = total - 74 * GiB - 819 * MiB; // in use 74.8 GiB + expect_eq("the GTT arithmetic reproduces the issue's 29.2 GiB budget", pool_budget(total, issue_free, 0), issue_budget); + const size_t host_tight = 15 * GiB; + expect_eq("a tight host replaces the GTT term", pool_budget(total, issue_free, host_tight), host_tight); + expect_true("so the refused 15.35 GiB is no longer admissible", pool_budget(total, issue_free, host_tight) < 15 * GiB + 359 * MiB); + expect_true("while the GTT-only arithmetic still called it admissible", pool_budget(total, issue_free, 0) > 15 * GiB + 359 * MiB); + expect_eq("a roomy host leaves the GTT term alone (no behaviour change)", pool_budget(total, issue_free, 80 * GiB), issue_budget); + expect_eq("a host figure larger than the GTT term never inflates the budget", pool_budget(total, issue_free, 900 * GiB), issue_budget); + expect_eq("unknown host (0) keeps the old arithmetic", pool_budget(total, issue_free, 0), pool_budget(total, issue_free, 0)); + expect_eq("unknown device stays unfettered", pool_budget(0, 0, 15 * GiB), 0); + + // The state machine must stay honest under a tight host: a cap can only lower the budget, and the host term bounds the + // recomputation that lifts a cap, so a refuted cap cannot restore an optimistic figure while the host is still full. + pool_budget_state_t st4; + st4.budget = issue_budget; + st4 = pool_budget_note_refusal(st4, 4 * GiB, issue_free); + st4 = pool_budget_observe(total, issue_free + 10 * GiB, 0, host_tight, st4); + expect_true("a lifted cap is still bounded by the host term", st4.budget <= host_tight); + expect_eq("and the cap is cleared, because the GTT pool did grow", st4.cap, 0); // engine #1417: the live check is now the DEFAULT, so the decision it makes is worth pinning like the cap above. The // numbers are the report's own: a 120 GiB pool, a box that had 74.8 GiB in use at device open and ~90 GiB when the From cc5440f1a5dc99ae2d6a0d2ce4ef946d5764c579 Mon Sep 17 00:00:00 2001 From: agent Date: Mon, 9 Nov 2026 05:13:39 -0400 Subject: [PATCH 2/3] hrx: default the budget's host term, so the change is additive for existing callers (refs engine #1502) llama.cpp #255 formats a diagnostic when a host weight cannot be materialized and calls pool_budget(total, free). Making the host term a required argument would have made whichever of the two PRs merged second fail to compile - a coordination cost for a parameter whose "unknown" value (0) is already defined as the old arithmetic. Defaulting it keeps every existing call compiling and meaning exactly what it meant, while the admission path passes the real figure. Verified with a throwaway TU that has #255's exact call shape: `pool_budget(120 GiB, 100 GiB)` now compiles and returns 84 GiB (the old GTT arithmetic), and the three-argument form returns 15 GiB for a 15 GiB host - so the default changes nothing for callers that do not opt in. --- ggml/src/ggml-hrx/hrx-1bit-safety.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.h b/ggml/src/ggml-hrx/hrx-1bit-safety.h index 40a9588aab6b..04dd283f2bd6 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.h +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.h @@ -31,7 +31,11 @@ namespace ggml::hrx::onebit { // |memory_available| is the host's MemAvailable (0 = unknown, which keeps the GTT-only arithmetic). On an APU the GTT ceiling // and DRAM are the same pool, so what the allocator can serve is bounded by both: this is the term that differs between two loads // with identical GTT counters and opposite outcomes (engine #1502). -size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available); +// +// The parameter is DEFAULTED so that a caller which only formats a diagnostic (llama.cpp #255 reports a refused host-weight +// materialize) keeps compiling and keeps meaning "GTT arithmetic only" - the change is additive, not a new required argument +// every call site has to be told about. A caller that can see /proc should pass it. +size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available = 0); // The budget after an allocation the allocator refused, given the size it refused (engine #1502). // From 28b27ecfd06c86f176d8eaa7b2cf799a5dc47793 Mon Sep 17 00:00:00 2001 From: agent Date: Mon, 9 Nov 2026 06:14:11 -0400 Subject: [PATCH 3/3] hrx: the budget reports the terms it decided with, and "unreadable" is not "zero" (review of #256) Three points from the review, all of them right. 1. The guardrail line recomputed `ceiling - in_use` inside the printf to print the GTT term. That is pool_budget()'s arithmetic written a second time: it agrees today and stops agreeing the first time the budget's definition changes, which is the drift the line exists to expose. pool_budget_terms() now computes the two terms, the minimum and which one bound, and both the budget and the log read from it - one place computes, every caller prints. 2. `0` conflated "MemAvailable could not be read" with "read, and the host has nothing left". The second is the one moment when admitting a launch is worst, and it was taking the same path as "unknown", i.e. ignored. kHostUnknown (SIZE_MAX) now means unreadable and only that; a measured zero is a term like any other and yields a budget of 0. The default parameter is kHostUnknown, so llama.cpp #255's two-argument call still means "GTT arithmetic only". This is stricter at every call site, which is the point: the tests caught two of them that had been passing 0 as "unknown" and now have to say kHostUnknown. A sentinel that callers must be explicit about is the difference between a convention and a check. 3. The line names the binding term ("bound by the host" / "bound by the GTT counter"), because that is the only part a reader had to infer - and the inference is what made the issue's two-identical-inputs pair take an hour to explain. Proof: 41 assertions pass, including `a measured zero host refuses` (0 budget) against `host unknown keeps the GTT arithmetic` (29.2 GiB), and `the terms say the host bound it`. llama-cli and the test build clean. --- ggml/src/ggml-hrx/ggml-hrx.cpp | 25 ++++++++++++-------- ggml/src/ggml-hrx/hrx-1bit-safety.cpp | 32 ++++++++++++++++++-------- ggml/src/ggml-hrx/hrx-1bit-safety.h | 32 ++++++++++++++++++++------ tests/test-hrx-pool-refusal-budget.cpp | 26 ++++++++++++++------- 4 files changed, 81 insertions(+), 34 deletions(-) diff --git a/ggml/src/ggml-hrx/ggml-hrx.cpp b/ggml/src/ggml-hrx/ggml-hrx.cpp index 0d854a74177d..fcf674c3becd 100644 --- a/ggml/src/ggml-hrx/ggml-hrx.cpp +++ b/ggml/src/ggml-hrx/ggml-hrx.cpp @@ -890,7 +890,9 @@ static void device_memory(ggml_backend_dev_t device, size_t * free, size_t * tot device_ctx->memory_budget, device_ctx->memory_budget_cap, device_ctx->memory_budget_cap_free }; state = ggml::hrx::onebit::pool_budget_observe( device_ctx->memory_total, free_bytes, ggml::hrx::onebit::pool_allocated(), - ggml::hrx::host_memory_available_bytes(), state); + ggml::hrx::host_memory_available_bytes() == 0 ? ggml::hrx::onebit::kHostUnknown + : ggml::hrx::host_memory_available_bytes(), + state); device_ctx->memory_budget = state.budget; device_ctx->memory_budget_cap = state.cap; device_ctx->memory_budget_cap_free = state.cap_free; @@ -1587,9 +1589,13 @@ static std::unique_ptr create_registry_context() { size_t device_free = device_ctx->memory_total; size_t device_total = device_ctx->memory_total; device_free_total(device_ctx.get(), device_free, device_total); - const size_t host_available = ggml::hrx::host_memory_available_bytes(); - device_ctx->memory_budget = - ggml::hrx::onebit::pool_budget(device_ctx->memory_total, device_free, host_available); + // A failed read is "unknown", not "zero bytes left": kHostUnknown disables the term, while a measured zero must + // refuse (a host with nothing left is the worst moment to admit a launch). Review of this PR, engine #1502. + const size_t host_raw = ggml::hrx::host_memory_available_bytes(); + const size_t host_available = host_raw == 0 ? ggml::hrx::onebit::kHostUnknown : host_raw; + const ggml::hrx::onebit::pool_budget_terms_t terms = + ggml::hrx::onebit::pool_budget_terms(device_ctx->memory_total, device_free, host_available); + device_ctx->memory_budget = terms.budget; if (device_ctx->memory_budget == 0 && device_ctx->memory_total != 0) { // Nothing fits under the ceiling: refuse the device here, loudly, instead of planning a launch that can // reach the fault line (#680). Report only, do not try to be clever about it. @@ -1607,16 +1613,15 @@ static std::unique_ptr create_registry_context() { const size_t reserve = ggml::hrx::onebit::pool_reserve_bytes(); GGML_LOG_WARN("ggml_hrx: %s guardrail: GTT total %.1f GiB, in use %.1f GiB, ceiling %.1f GiB " "(reserve %.1f GiB), pool budget %.1f GiB (GTT term %.1f GiB, host MemAvailable %.1f GiB) - a " - "launch that needs more is refused here rather than faulting the device (engine #680)\n", + "launch that needs more is refused here rather than faulting the device (engine #680; bound by %s)\n", device_ctx->name.c_str(), device_total / (1024.0 * 1024.0 * 1024.0), (device_total > device_free ? device_total - device_free : 0) / (1024.0 * 1024.0 * 1024.0), (device_total > reserve ? device_total - reserve : 0) / (1024.0 * 1024.0 * 1024.0), reserve / (1024.0 * 1024.0 * 1024.0), - device_ctx->memory_budget / (1024.0 * 1024.0 * 1024.0), - ((device_ctx->memory_total > reserve ? device_ctx->memory_total - reserve : 0) - - (device_ctx->memory_total > device_free ? device_ctx->memory_total - device_free : 0)) / - (1024.0 * 1024.0 * 1024.0), - host_available / (1024.0 * 1024.0 * 1024.0)); + terms.budget / (1024.0 * 1024.0 * 1024.0), + terms.gtt_term / (1024.0 * 1024.0 * 1024.0), + host_raw == 0 ? -1.0 : terms.host_available / (1024.0 * 1024.0 * 1024.0), + terms.host_bound ? "the host" : (host_raw == 0 ? "the GTT counter (host unreadable)" : "the GTT counter")); } device_ctx->description = *name + " (" + *architecture + ")"; device_ctx->architecture = *architecture; diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp index c97804daa988..e3dfbff05e99 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp @@ -83,29 +83,43 @@ bool gtt_counters(size_t & total, size_t & used) { return false; } -size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available) { +pool_budget_terms_t pool_budget_terms(size_t memory_total, size_t memory_free, size_t memory_available) { + pool_budget_terms_t t; const char * env = std::getenv("GGML_HRX_POOL_LIMIT_MB"); if (env != nullptr) { const long long mb = std::atoll(env); if (mb > 0) { - return static_cast(mb) * kMiB; + // An operator-set limit is the budget, floor and ceiling at once; it is not a min() of terms, and saying so + // keeps the log from implying the box produced the number. + t.host_available = memory_available; + t.gtt_term = static_cast(mb) * kMiB; + t.budget = t.gtt_term; + return t; } } if (memory_total == 0) { - return 0; // unknown device size: keep the unfettered behaviour + return t; // unknown device size: keep the unfettered behaviour (every field stays 0) } const size_t reserve = pool_reserve_bytes(); const size_t ceiling = memory_total > reserve ? memory_total - reserve : 0; const size_t in_use = memory_total > memory_free ? memory_total - memory_free : 0; - const size_t by_gtt = ceiling > in_use ? ceiling - in_use : 0; + t.gtt_term = ceiling > in_use ? ceiling - in_use : 0; + t.host_available = memory_available; // engine #1502: the GTT counter is not the allocator's ceiling on an APU. Letting a 15.35 GiB buffer through a 29.2 GiB // budget while the host had ~15 GiB left made the failure land inside hsa_amd_memory_pool_allocate and read as - // "llama_model_load: error loading model" (two Hyperloom measurements scored 0.0 that way). The engine's admission check has - // always used min(GTT free, MemAvailable) (app/mem_admit.h); this is the same two-term model in the fork's safety net. - if (memory_available != 0 && memory_available < by_gtt) { - return memory_available; + // "llama_model_load: error loading model". The engine's admission check has always used min(GTT free, MemAvailable) + // (app/mem_admit.h); this is the same two-term model in the fork's safety net. kHostUnknown alone disables the term. + if (memory_available != kHostUnknown && memory_available < t.gtt_term) { + t.budget = memory_available; + t.host_bound = true; + } else { + t.budget = t.gtt_term; } - return by_gtt; + return t; +} + +size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available) { + return pool_budget_terms(memory_total, memory_free, memory_available).budget; } // engine #1417: the budget above is a snapshot taken at device open, and the ledger is per-process, so another process diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.h b/ggml/src/ggml-hrx/hrx-1bit-safety.h index 04dd283f2bd6..af5f2c6571e2 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.h +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.h @@ -28,14 +28,32 @@ namespace ggml::hrx::onebit { // budget is the GTT ceiling (memory_total minus GGML_HRX_GTT_RESERVE_MB, default 16384) less what other processes // already hold (memory_total - memory_free). A GTT plan past this budget is refused at allocation instead of // faulting the device: #680 is a gfxhub page fault plus a MODE2 reset at GTT 106.5 of 120 GiB. -// |memory_available| is the host's MemAvailable (0 = unknown, which keeps the GTT-only arithmetic). On an APU the GTT ceiling -// and DRAM are the same pool, so what the allocator can serve is bounded by both: this is the term that differs between two loads -// with identical GTT counters and opposite outcomes (engine #1502). +// The two terms the budget is the minimum of, and the budget itself. Returned together so a caller can PRINT what the +// decision used instead of recomputing it (a second copy of `ceiling - in_use` in a printf agrees today and drifts later), +// and so the log can name the term that bound. +struct pool_budget_terms_t { + size_t gtt_term = 0; // (total - reserve) - in_use, from the driver counters + size_t host_available = 0; // MemAvailable as passed in, or kHostUnknown + size_t budget = 0; // min(gtt_term, host_available), or gtt_term when the host figure is unknown + bool host_bound = false; +}; + +// |memory_available| is the host's MemAvailable. On an APU the GTT ceiling and DRAM are the same pool, so what the +// allocator can serve is bounded by both: this is the term that differs between two loads with identical GTT counters and +// opposite outcomes (engine #1502). +// +// kHostUnknown means "the host figure could not be read", and only then is the term ignored. It is deliberately NOT 0: +// a measured zero means the host has nothing left, which is the one case where admitting a launch is worst, and conflating +// it with "unreadable" would let that case take the fallback (review of this PR). // -// The parameter is DEFAULTED so that a caller which only formats a diagnostic (llama.cpp #255 reports a refused host-weight -// materialize) keeps compiling and keeps meaning "GTT arithmetic only" - the change is additive, not a new required argument -// every call site has to be told about. A caller that can see /proc should pass it. -size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available = 0); +// The parameter is DEFAULTED to kHostUnknown so a caller that only formats a diagnostic (llama.cpp #255 reports a refused +// host-weight materialize) keeps compiling and keeps meaning "GTT arithmetic only" - the change is additive. A caller that +// can read /proc should pass the figure, and must map a failed read to kHostUnknown rather than to 0. +inline constexpr size_t kHostUnknown = static_cast(-1); + +pool_budget_terms_t pool_budget_terms(size_t memory_total, size_t memory_free, size_t memory_available = kHostUnknown); + +size_t pool_budget(size_t memory_total, size_t memory_free, size_t memory_available = kHostUnknown); // The budget after an allocation the allocator refused, given the size it refused (engine #1502). // diff --git a/tests/test-hrx-pool-refusal-budget.cpp b/tests/test-hrx-pool-refusal-budget.cpp index b75b3e3cdae4..dca053a4d649 100644 --- a/tests/test-hrx-pool-refusal-budget.cpp +++ b/tests/test-hrx-pool-refusal-budget.cpp @@ -43,6 +43,9 @@ static void expect_eq(const char * what, size_t got, size_t want) { int main() { using ggml::hrx::onebit::pool_budget_after_refusal; + using ggml::hrx::onebit::pool_budget; + using ggml::hrx::onebit::pool_budget_terms; + using ggml::hrx::onebit::kHostUnknown; // The issue's own case: the guardrail admitted 15.35 GiB out of 29.2 GiB and the pool refused it. const size_t budget = 29 * GiB + 205 * MiB; // 29.2 GiB @@ -88,7 +91,7 @@ int main() { expect_eq("cap after refusing 80 GiB is size-1", st.cap, 80 * GiB - 1); // The other process now holds 1 GiB LESS than at the refusal, so the cap is lifted. - st = pool_budget_observe(total, free_at_refusal + GiB, our_mem, 0, st); + st = pool_budget_observe(total, free_at_refusal + GiB, our_mem, kHostUnknown, st); expect_eq("lifting the cap restores the registration budget (own memory counted once)", st.budget, 85 * GiB); expect_eq("the cap is cleared when it is lifted", st.cap, 0); @@ -96,7 +99,7 @@ int main() { pool_budget_state_t st2; st2.budget = 30 * GiB; st2 = pool_budget_note_refusal(st2, 40 * GiB, total - 45 * GiB); // refusal seen with 75 GiB free - st2 = pool_budget_observe(total, total - 90 * GiB, our_mem, 0, st2); // other process grew by 45 GiB + st2 = pool_budget_observe(total, total - 90 * GiB, our_mem, kHostUnknown, st2); // other process grew by 45 GiB expect_eq("a shrinking pool keeps the cap (it is not cleared)", st2.cap, 30 * GiB); expect_eq("a shrinking pool keeps the cap's evidence", st2.cap_free, total - 45 * GiB); expect_true("a shrinking pool never ends up above the cap", st2.budget <= st2.cap); @@ -107,26 +110,33 @@ int main() { st3.budget = b_small; st3 = pool_budget_note_refusal(st3, 4 * GiB + 700 * MiB, 20 * GiB); expect_true("a 4.7 GiB model refused at a 13.6 GiB budget is capped below its size", st3.cap < 4 * GiB + 700 * MiB); - st3 = pool_budget_observe(total, 60 * GiB, 0, 0, st3); // the pool frees + st3 = pool_budget_observe(total, 60 * GiB, 0, kHostUnknown, st3); // the pool frees expect_true("when the pool frees the same model is admissible again", st3.budget > 4 * GiB + 700 * MiB); - expect_eq("no cap in force: an observation changes nothing", pool_budget_observe(total, 10 * GiB, 0, 0, pool_budget_state_t{20 * GiB, 0, 0}).budget, 20 * GiB); + expect_eq("no cap in force: an observation changes nothing", pool_budget_observe(total, 10 * GiB, 0, kHostUnknown, pool_budget_state_t{20 * GiB, 0, 0}).budget, 20 * GiB); // ---- the host term (this lane's half of #1502) ------------------------------------------------------------------ // // On an APU the GTT ceiling and DRAM are one pool, so the allocator is bounded by the host as well as by the GTT counter. // The issue's measured case: budget 29.2 GiB from the counters, while /proc/meminfo had roughly 15 GiB left - and the pool // then refused 15.35 GiB, failing at model load instead of at admission. - using ggml::hrx::onebit::pool_budget; const size_t issue_budget = 29 * GiB + 205 * MiB; // 29.2 GiB, as printed by the guardrail const size_t issue_free = total - 74 * GiB - 819 * MiB; // in use 74.8 GiB - expect_eq("the GTT arithmetic reproduces the issue's 29.2 GiB budget", pool_budget(total, issue_free, 0), issue_budget); + expect_eq("the GTT arithmetic reproduces the issue's 29.2 GiB budget", + pool_budget(total, issue_free, kHostUnknown), issue_budget); const size_t host_tight = 15 * GiB; expect_eq("a tight host replaces the GTT term", pool_budget(total, issue_free, host_tight), host_tight); expect_true("so the refused 15.35 GiB is no longer admissible", pool_budget(total, issue_free, host_tight) < 15 * GiB + 359 * MiB); - expect_true("while the GTT-only arithmetic still called it admissible", pool_budget(total, issue_free, 0) > 15 * GiB + 359 * MiB); + expect_true("while the GTT-only arithmetic still called it admissible", + pool_budget(total, issue_free, kHostUnknown) > 15 * GiB + 359 * MiB); expect_eq("a roomy host leaves the GTT term alone (no behaviour change)", pool_budget(total, issue_free, 80 * GiB), issue_budget); expect_eq("a host figure larger than the GTT term never inflates the budget", pool_budget(total, issue_free, 900 * GiB), issue_budget); - expect_eq("unknown host (0) keeps the old arithmetic", pool_budget(total, issue_free, 0), pool_budget(total, issue_free, 0)); + // kHostUnknown alone disables the term (a failed /proc read). A MEASURED zero must refuse: a host with + // nothing left is the worst moment to admit a launch (review of #256). + expect_eq("host unknown keeps the GTT arithmetic", pool_budget(total, issue_free, kHostUnknown), issue_budget); + expect_eq("a measured zero host refuses", pool_budget(total, issue_free, 0), 0); + expect_true("and the terms say the host bound it", pool_budget_terms(total, issue_free, 0).host_bound); + expect_true("while the GTT term alone would have admitted it", + pool_budget_terms(total, issue_free, 0).gtt_term > 15 * GiB + 359 * MiB); expect_eq("unknown device stays unfettered", pool_budget(0, 0, 15 * GiB), 0); // The state machine must stay honest under a tight host: a cap can only lower the budget, and the host term bounds the