From fc9fdcf2adcdd349b58f31c86b2eceb580cd8fe5 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Tue, 10 Nov 2026 10:53:14 -0400 Subject: [PATCH] hrx: read the host term before classifying it, so a clang build opens the device again (engine #1942) Both guardrail call sites wrote pool_host_term(host_raw, host_memory_available_bytes(host_raw)) which passes host_raw by value while a sibling argument fills it. Argument evaluation order is unspecified; clang evaluates left to right, so the by-value copy is taken (0) before the reader runs and the term arrives as (0, readable) - a measured zero - on a host with tens of GiB available. With the pinned toolchain (amdclang++) the open-time guardrail therefore computed a 0 budget and refused the device ("leaves nothing under the ceiling", no device registered), and the live pool check would have refused every allocation. g++ evaluates right to left, so the line was correct there. pool_host_term_read() does the read as its own full expression and is used by both call sites; the reader is a parameter so the test drives the three cases (readable, measured zero, unreadable) without a live /proc. A failed reader's out value is not reported as a reading. Co-Authored-By: Claude Fable 5.2 --- ggml/src/ggml-hrx/ggml-hrx.cpp | 2 +- ggml/src/ggml-hrx/hrx-1bit-safety.cpp | 12 +++++++- ggml/src/ggml-hrx/hrx-1bit-safety.h | 12 ++++++++ tests/test-hrx-pool-refusal-budget.cpp | 38 ++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 2 deletions(-) diff --git a/ggml/src/ggml-hrx/ggml-hrx.cpp b/ggml/src/ggml-hrx/ggml-hrx.cpp index 5aa6a9dfe9fb..d70ceece75b8 100644 --- a/ggml/src/ggml-hrx/ggml-hrx.cpp +++ b/ggml/src/ggml-hrx/ggml-hrx.cpp @@ -1647,7 +1647,7 @@ static std::unique_ptr create_registry_context() { // refuse - a host with nothing left is the worst moment to admit a launch (engine #1502). size_t host_raw = 0; const size_t host_available = - ggml::hrx::onebit::pool_host_term(host_raw, ggml::hrx::host_memory_available_bytes(host_raw)); + ggml::hrx::onebit::pool_host_term_read(ggml::hrx::host_memory_available_bytes, 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; diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp index da9b5152b9fe..3e280184b557 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.cpp +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.cpp @@ -88,6 +88,16 @@ size_t pool_host_term(size_t raw, bool readable) { return readable ? raw : kHostUnknown; } +size_t pool_host_term_read(bool (*reader)(size_t &), size_t & raw) { + raw = 0; + // A full expression of its own: the read is complete before `raw` is looked at (see the header). + const bool readable = reader != nullptr && reader(raw); + if (!readable) { + raw = 0; // a reader that failed may have written to its out parameter; that is not a reading + } + return pool_host_term(raw, readable); +} + bool pool_budget_is_fixed_limit() { const char * env = std::getenv("GGML_HRX_POOL_LIMIT_MB"); return env != nullptr && std::atoll(env) > 0; @@ -272,7 +282,7 @@ bool pool_reserve(size_t budget, size_t size, const char * what) { if (pool_live_counters(total, in_use)) { const size_t margin = pool_live_margin_bytes(); size_t host_raw = 0; - const size_t host_available = pool_host_term(host_raw, host_memory_available_bytes(host_raw)); + const size_t host_available = pool_host_term_read(host_memory_available_bytes, host_raw); if (!pool_live_fits(size, total, in_use, margin, host_available)) { const size_t live_free = in_use >= total ? 0 : total - in_use; GGML_LOG_ERROR("%s: HRX pool limit: %s of %.1f MiB does not fit the live device budget " diff --git a/ggml/src/ggml-hrx/hrx-1bit-safety.h b/ggml/src/ggml-hrx/hrx-1bit-safety.h index 7dcb2988ae25..3c709d5485d3 100644 --- a/ggml/src/ggml-hrx/hrx-1bit-safety.h +++ b/ggml/src/ggml-hrx/hrx-1bit-safety.h @@ -56,6 +56,18 @@ bool pool_budget_is_fixed_limit(); // `raw == 0 ? kHostUnknown : raw` collapsed those two cases - see tests/test-hrx-pool-refusal-budget.cpp. size_t pool_host_term(size_t raw, bool readable); +// The same term, READ AND CLASSIFIED IN ONE SEQUENCED STEP (engine #1942). Both call sites used to write +// pool_host_term(raw, host_memory_available_bytes(raw)) +// which hands `raw` to the callee BY VALUE while a sibling argument fills it. C++ leaves the order in which a call's +// arguments are evaluated unspecified, and clang evaluates them left to right: the by-value `raw` is copied (still 0) +// before the reader runs, so the term arrives as (0, readable = true) - a "measured zero" - on a host with tens of +// GiB available. The open-time guardrail then refused the device outright ("leaves nothing under the ceiling", no +// device registered, `--device HRX0` invalid) and the live check refused every allocation. g++ evaluates right to +// left, so the same line was correct there and the fault only showed on the pinned toolchain's own compiler. +// |reader| is host_memory_available_bytes in production; a parameter so the test can drive the three cases without +// a live /proc. |raw| receives the figure the reader took (0 when it took none), for the caller's log line. +size_t pool_host_term_read(bool (*reader)(size_t &), size_t & raw); + // The budget after an allocation the allocator refused, given the size it refused (engine #1502). // // The guardrail can only estimate what the pool will serve - ROCr exposes no "available" query, so pool_budget() comes from the diff --git a/tests/test-hrx-pool-refusal-budget.cpp b/tests/test-hrx-pool-refusal-budget.cpp index 0c26c4dade17..81269da7f99d 100644 --- a/tests/test-hrx-pool-refusal-budget.cpp +++ b/tests/test-hrx-pool-refusal-budget.cpp @@ -32,6 +32,20 @@ static void expect_true(const char * what, bool cond) { failures++; } +// Readers for pool_host_term_read(): what /proc/meminfo can say, without a live /proc. +static bool fake_reader_64gib(size_t & out) { + out = static_cast(64) * 1024 * 1024 * 1024; + return true; +} +static bool fake_reader_zero(size_t & out) { + out = 0; + return true; +} +static bool fake_reader_unreadable(size_t & out) { + out = 777; // a reader that failed may have scribbled on its out parameter; the figure must not be trusted + return false; +} + static void expect_eq(const char * what, size_t got, size_t want) { if (got == want) { printf(" PASS %-58s %zu\n", what, got); @@ -194,6 +208,30 @@ int main() { expect_true("an unreadable /proc still admits on the GTT term alone", pool_live_fits(1 * MiB, gtt, gtt - 45 * GiB, margin_256, pool_host_term(0, false))); + // ---- the host term is READ BEFORE it is classified (engine #1942) --------------------------------------------- + // Both call sites wrote pool_host_term(raw, reader(raw)): `raw` goes in by value while a sibling argument fills + // it, and the order of argument evaluation is unspecified. clang (the pinned toolchain's compiler) copies `raw` + // first, so a host with 64 GiB available arrived as (0, readable) - a measured zero - and the guardrail refused + // the device with "leaves nothing under the ceiling" while the live check refused every allocation. + // pool_host_term_read() sequences the read; these arms fail on the old expression under clang and pass under g++, + // which is exactly why a g++ run of this suite never saw it. + using ggml::hrx::onebit::pool_host_term_read; + size_t raw = 12345; + expect_eq("a readable host figure reaches the term (not a measured zero)", + pool_host_term_read(fake_reader_64gib, raw), 64 * GiB); + expect_eq("and the caller's log figure is the one that was read", raw, 64 * GiB); + expect_true("so a roomy host does not zero the open-time budget", + pool_budget(total, issue_free, pool_host_term_read(fake_reader_64gib, raw)) == issue_budget); + expect_true("nor refuse a 1 MiB allocation through the live check", + pool_live_fits(1 * MiB, gtt, gtt - 45 * GiB, margin_256, pool_host_term_read(fake_reader_64gib, raw))); + raw = 12345; + expect_eq("a MEASURED zero still refuses", pool_host_term_read(fake_reader_zero, raw), 0); + expect_eq("with a zero log figure", raw, 0); + raw = 12345; + expect_eq("an unreadable /proc is unknown", pool_host_term_read(fake_reader_unreadable, raw), kHostUnknown); + expect_eq("and a stale caller value is not reported as a reading", raw, 0); + expect_eq("no reader at all is unknown too", pool_host_term_read(nullptr, raw), kHostUnknown); + printf(failures == 0 ? "test-hrx-pool-refusal-budget: all requirements passed\n" : "test-hrx-pool-refusal-budget: %d FAILED\n", failures); return failures == 0 ? 0 : 1;