Repository navigation
Provision free-budget headroom for the session-persistent workspace zone (fix #810 regression) - #816
Closed
justinchuby wants to merge 1 commit into
Closed
justinchuby wants to merge 1 commit into
justinchuby wants to merge 1 commit into
Conversation
…one (#810, #736) #810 governed the GQA BSH<->BNSH transpose scratch as a session-persistent Workspace reservation. On the managed no-spill VMM native-CUDA decode path it became the *first* consumer of the EP arena-zone mapped allowance, whose limit starts at zero and grows only by borrowing limit from a registered reclaimable weight/KV holder. When the model fits in VRAM, weights are committed directly into tier usage and no such holder is registered, so mapped-growth lending had no victim to borrow from -- the 2 MiB prefill workspace grant was refused with a budget of zero even though ~7.3 GiB of device budget was free. This broke mobius_seqmajor_growth_parity_native_cuda on main. Root cause is budget arithmetic, not over-reservation: the failing route is head-major prefill (Sq > 1), which genuinely needs both transposes per #810, so the reservation is correct. The gap was that MappedGrowthAuthority::prepare_ mapped_growth could only satisfy a request from the requester's own available limit or by evicting a registered victim -- it never drew from genuinely-free tier budget (limit - used - allowance_reserved - growth_reserved). Fix: before evicting any live holder, grow the requester's own allowance from free tier budget. Eviction remains the fallback for a truly full tier, and a genuine shortage still surfaces as the typed pre-header 429 TierExhausted path (#743), never a late 500/OOM. Preferring free budget over eviction also honors G3 (never take live state while free capacity remains). The transpose scratch stays governed and still charges zero on the Sq == 1 decode route; head-major behaviour is byte-identical (verified capture ON/OFF, vmm-inplace and realloc). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Closing as a duplicate — #814 landed the equivalent fix (claim unassigned/free mapped-growth authority capacity before holder reclaim, with rollback + typed The two unique deliverables from this investigation (the validation-gap recommendation and the collateral inventory/attribution note) have been posted as a comment on #814 so they aren't lost. No code from this branch is needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References #810, #806, #736.
Regression
Merging the #736 GQA workspace-governance slices broke
mobius_seqmajor_growth_parity_native_cudaonmain(d7db6f2c, reproduced independently on a cleanorigin/mainworktree):The GPU was 100% idle — not contention. A budget of zero for the session-persistent workspace class on an idle device is the signature of "this class was never provisioned for", not "the budget was exhausted".
Root cause: budget arithmetic, established not assumed
I checked both candidate causes against the code before changing anything, because the #736 audit has found route-conditional over-reservation five times out of six and I did not want to skip that check.
It is the governor's budget arithmetic, not over-reservation.
GroupQueryAttentionKernel::composite_workspace_requirementsizes every region from the concrete input geometry (batch,q_seq,head_dim) and conditions each on the route:want_stagingonly for packed QKV (Govern GroupQueryAttention packed QKV-projection staging (#736) #806),want_scoresonly on the f32 reference path (Govern GroupQueryAttention WS_SCORES workspace (#736) #795), and the BSH↔BNSH transpose scratch (Govern GQA BNSH transpose workspace #810) viagqa_transpose_scratch, which returnsoutput: q_seq > 1andquery: q_seq != 1 || packed || rotary. So Govern GQA BNSH transpose workspace #810 charges zero on theSq == 1decode route, and on the failing head-major prefill (Sq > 1) route the reserved bytes are genuinely consumed. Not over-reservation on a non-using route.Instrumenting the governor at the point of failure showed the real defect:
On the managed no-spill VMM path with a model that fits in VRAM, weights are committed directly into tier usage (~358 MiB) and no reclaimable weight/KV holder is registered. The EP arena-zone mapped allowance backing
Workspace{step_scoped:false}starts at limit 0 andMappedGrowthAuthority::prepare_mapped_growthcould grow it only from the requester's own available limit or by evicting a registered victim. With no victim, the 2 MiB grant was refused with a budget of zero — even though ~7.3 GiB of device budget was genuinely free.On the cumulative #806 + #810 attribution
A second agent attributed the break to #806 and #810 together. That is structurally correct — #795/#806/#810 all fold into one
WorkspaceLifetime::SessionPersistent,Workspace{step_scoped:false}composite reservation, so the same governor gap bites whichever slice first produces a non-zero demand on a given route. But on this route the tipping byte is #810's alone: the mobius native path is fp16, unpacked, flash-prefill, sowant_stagingis false (#806 contributes 0 bytes) andwant_scoresis false (#795 contributes 0 bytes). #810's Q-BNSH + output-BNSH transpose scratch is the first non-zero consumer of the arena zone on this path — which is exactly why the suite broke at #810 and not at #806's merge. The ~2 MiB magnitude corroborates this: a reference-scores demand would bebatch·heads·q_seq·kv·4 B(tens of MiB on a multi-hundred-token prefill) and packed staging larger still, whereas ~2 MiB matches only the transpose scratch2·batch·q_seq·q_hidden·2 B.Fix
In
prepare_mapped_growth, before evicting any live holder, grow the requester's own allowance from genuinely-free tier budget (limit − used − allowance_reserved − growth_reserved). Eviction remains the fallback for a truly full tier. Properties:TierExhaustedpath (Return overload response for KV admission refusal #743), never a late 500/OOM.Sq == 1decode; head-major behaviour is byte-identical (verified capture ON/OFF, vmm-inplace + realloc).New governor unit tests pin both halves: free-budget-backed growth with no victim, and typed refusal when neither free budget nor an evictable holder can cover the request. One existing test (
registration_is_weak_explicit_and_priority_ordered) relied on incidental free slack forcing eviction; it now consumes that slack explicitly so it still exercises priority-ordered reclaim.Collateral inventory
The fix lives in the shared governor mapped-growth path, so it structurally covers every governed reservation that reaches the arena zone. Audit slices #751/#795/#799/#802/#806/#810 each add governed reservations against the same authority; any that becomes the first non-zero arena-zone consumer on a fits-in-VRAM managed-VMM run would hit the identical zero-budget refusal. Verified no other e2e path is now over- or under-subscribed:
mobius_seqmajor_growth_parity_native_cuda— green (all 4 phases: capture ON/OFF, vmm-inplace + realloc; byte-identical head vs seq).qwen2_5_0_5b_managed_vmm_default_e2e— green (the exact managed-VMM default path).weight_offload_native_cuda_e2e— green.group_query_attention_gpu(26/26),gqa_seqmajor_parity_gpu(2/2),standard_attention_gpu(24),attention_gpu(6),allocation_counts_gpu(2) — the failure was purely in the engine-governor prefill workspace preparation path, not the kernels.Validation-gap recommendation
This regression merged past a green
cargo test -p onnx-runtime-sessionbecause the failing test is#[ignore]d and only runs with--features cuda,native-backend --ignored— so the mandatory pre-merge set said nothing about it. I recommend addingmobius_seqmajor_growth_parity_native_cuda(andqwen2_5_0_5b_managed_vmm_default_e2e) to the standard pre-merge set for any change touching workspace governance or the memory governor. Six audit slices have each added governed reservations against the same authority; without this coverage the class of budgeting-seam break will recur (cf. #751, #797 earlier this week).This fix is also on the critical path for #811's mobius re-measurement, which is blocked until prefill workspace preparation succeeds again.
Validation performed
cargo fmt -p onnx-runtime-memory-governor -- --checkcargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warningscargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warningscargo test -p onnx-runtime-memory-governor(38 + 5 passed)cargo test -p onnx-runtime-session(153 core + all integration/doc suites)Do not merge — leaving this for the author to review and merge, since they merged the regression.