Repository navigation
fix(cuda): resolve unbounded KV sentinel to a device bound on the VMM path (closes #1266) - #1281
Merged
Merged
Conversation
… path (fixes #1266) The VMM native-CUDA decode path reserves the full mask island (`batch × capacity.max_len × i64`) and the full KV extent (`full_vmm_kv_allocation_bytes(..., capacity.max_len)`) up front. For a model with no `max_sequence_length` metadata, `resolve_cuda_kv_capacity` returns the deliberate `usize::MAX` "unbounded until growth fails" sentinel (#367), so on the VMM path — where these values are *live* (unlike the non-VMM path fixed in #1264) — the reservation arithmetic overflows and aborts construction. The reservation is virtual-only: `allocate_committed` carves the requested bytes out of a fixed 64 GiB virtual arena and only `committed_ranges` claim physical granules. An over-large-but-finite bound is therefore nearly free, and a decode that outgrows it still fails at the same physical ceiling the sentinel implied. So resolve the sentinel — only for the VMM reservation — to the largest token count the device could ever hold (`device_free / bytes_per_token`). `capacity.max_len` and the non-VMM grow-until-OOM contract are left untouched. Adds a CUDA-gated regression test that drives real VMM-path construction with a metadata-less capacity, asserts it is actually on the commit-on-demand path, and asserts the `usize::MAX` sentinel survives. Reverting the bound resolution makes it RED with the original overflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Benchmarks — No RegressionComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1281 +/- ##
=========================================
+ Coverage 0 80.59% +80.59%
=========================================
Files 0 364 +364
Lines 0 160710 +160710
Branches 0 160710 +160710
=========================================
+ Hits 0 129532 +129532
- Misses 0 26521 +26521
- Partials 0 4657 +4657
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
What this fixes
Closes #1266 — the VMM half of the metadata-less mask/KV reservation overflow that #1264 deliberately left for a design decision.
On the VMM native-CUDA decode path (
ONNX_GENAI_CUDA_VMM=1),DecodeCudaState::newreserves the full mask island (batch × capacity.max_len × i64) and the full KV extent (full_vmm_kv_allocation_bytes(..., capacity.max_len)) up front. For a model with nomax_sequence_lengthmetadata,resolve_cuda_kv_capacityreturnscapacity.max_len == usize::MAX(the deliberate "unbounded until growth fails" sentinel from #367). Unlike the non-VMM path — where those values are dead (fixed in #1264) — on the VMM path they are live, so the reservation arithmetic overflows and aborts construction withfull CUDA mask reservation size overflow.The decisive fact: the VMM reservation is virtual-only
allocate_device_binding_committed(..., allocation_bytes, committed_ranges)→CudaVmmAllocator::allocate_committed(bytes, align, ranges)only carvesbytesout of a fixed 64 GiB virtual arena (RESERVATION_BYTES = 64 << 30,arena.spans.carve). Physical granules are claimed only forcommitted_ranges. So the up-front reservation costs address space, not memory.Options weighed
device_free / bytes_per_token(the largest sequence the device could ever physically hold). Because the reservation is virtual-only, an over-large-but-finite bound is nearly free (≈device_freeof VA + a ~1.5 GB mask, well inside the 64 GiB arena), and a decode that outgrows it still fails at the same physical ceiling the sentinel implied.max_sequence_length— for no gain, since (a) makes VMM work.capacity.max_lenitself, changing the deliberateusize::MAXcontract (feat(kv): one KV capacity mechanism for native CUDA and ORT #367) that the non-VMM grow-until-OOM path andcuda_kv_capacity_without_metadata_is_unbounded_until_growth_failsdepend on. Not worth touching a deliberate contract when a local resolution suffices.The fix therefore resolves the bound only locally, only for the VMM reservation;
capacity.max_lenand the non-VMM path are untouched (the regression test asserts the sentinel survives).Falsification (RTX 4060, driver 591.55, CUDA 13 runtime)
New CUDA-gated regression test
native_cuda_vmm_decoder_constructs_with_unbounded_metadata_less_capacitydrives real VMM-path construction with a metadata-less capacity, asserts it is actually on the commit-on-demand path (kv_commits_on_demand()), and assertscapacity.max_len == usize::MAXstill holds.vmm_reserved_max_len = capacity.max_len) → RED with the real failure:Caused by: full CUDA mask reservation size overflow.The VMM path is proven live, not a proxy: the arena log reports
device allocations go through a VMM arena over 68719476736 bytes of reserved address space, and the assertion catches any silent fall-back to the eager path. The existing real-model growth testnative_cuda_vmm_kv_grows_in_place_and_commits_more_granules(qwen05b-q4,ONNX_GENAI_KV_MIN_BUCKET=8) also passes — it callsensure_capacity(8193)and asserts committed bytes grow by the exact bucket delta withdevice_ptrsunchanged (grown in place), confirming this change does not disturb the metadata-having VMM growth path.Test isolation note
The test deliberately does not set
ONNX_GENAI_CUDA_GRAPH: that is a frozenRuntimeConfigknob (#804), so mutating it after any earlier test froze the process-wide config trips the freeze-guard. Construction never captures a graph, so the knob is irrelevant here;vmm_enabled()readsONNX_GENAI_CUDA_VMMdirectly from the environment, so setting only VMM is safe regardless of freeze order. Also folds in a rustfmt fix for an overlong line #1264 introduced in the same file.GPU verification (RTX 4060,
--features "cuda,native-backend" -- --test-threads=1)ONNX_GENAI_RUN_CUDA_SMOKE=1): 556 passed, 3 failed, 2 ignored. The 3 failures are the known pre-existingmodel.io.token_inputharness ambiguities (native_cuda_capture_replay_...,native_cuda_symbolic_batch_...,native_cuda_symbolic_total_seq_...), unrelated to this change and identical to the set fix(cuda): compute full mask reservation only on the VMM path (fixes red native-CUDA parity test) #1264 documented. Both new metadata-less tests pass; zero new failures.Known caveat
The derived bound is
device_free / bytes_per_token; the summed VA reservation is ≈1.2 × device_free. On this 8 GB card that is8 GB of the 64 GiB arena. On a very large GPU (≳53 GiB free) the metadata-less VMM reservation could exceed the 64 GiB arena and fail the carve loudly at construction (no silent corruption). That is an acceptable, visible failure mode for a narrow combination (metadata-less and VMM and a huge idle GPU); a follow-up could cap the bound to the arena if it ever bites.