Repository navigation
refactor(memory)!: make VMM the sole built-in CUDA memory mechanism (#1186 Phase 7) - #1465
justinchuby wants to merge 1 commit into
Conversation
Part of #1186 -- Phase 7 only. The CUDA EP carried two built-in device memory mechanisms: an eager `cuMemAlloc` allocator and the VMM arena, with the arena selected by an opt-in environment flag and the eager allocator serving as the silent fallback when the arena could not be built. That is the shape the memory model argues against: a missing capability quietly selected a different mechanism whose bytes were not charged the same way, and the operator's only evidence was a log line. Delete the eager allocator and the dual-selection state. The arena is now constructed unconditionally, and failing to construct it is fatal at provider construction with a diagnostic naming the device, the driver's own message, the driver entry points that constitute the support boundary, any requested managed limit, and `with_memory` as the supported way to supply a different mechanism. Removing the built-in implementation does not remove the capability. `DeviceAllocator` is unchanged. `with_memory` becomes authoritative rather than refusing: it retires the arena and the injected mechanism serves everything afterwards. It is refused, before the offered allocator is used at all, only for a foreign device or for a mechanism that still has memory outstanding -- both return `Err`, so no successful builder call is ignored. Removed: - `CudaDeviceAllocator`, `QuarantinedCudaAllocation` (crates/onnx-runtime-cuda-memory/src/device_allocator.rs) - `CUDA_VMM_ENV` / `ONNX_GENAI_CUDA_VMM`, `vmm_enabled()` - `VmmInitialization`, `resolve_vmm_initialization` - `CudaMemory::Allocator` (renamed `Injected`) Behaviour change worth calling out: `production_physical_pool_enabled()` no longer requires the removed flag, so a caller who set `ONNX_GENAI_CUDA_PHYSICAL_HANDLE_POOL_BYTES` alone previously had it ignored and now has it honoured. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## justinchuby-sturdy-potato #1465 +/- ##
=============================================================
+ Coverage 79.38% 80.17% +0.79%
=============================================================
Files 373 374 +1
Lines 165958 168899 +2941
Branches 165958 168899 +2941
=============================================================
+ Hits 131740 135413 +3673
+ Misses 29372 28622 -750
- Partials 4846 4864 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison 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
|
> **A100 safety update (head `13c37a7f`):** expected shared-prefix admission failures are now all-or-private: K/V commit is transactional, a failed V share rolls K back before enqueue, and only a successful rollback permits private-KV fallback. Fatal kernel faults remain process-fatal; the new cross-process VMM probe proves a worker fault does not affect an actively executing peer, while the current server is still in-process and does not yet consume physical shared-prefix metadata. See the [final verified conclusion](#1579 (comment)). Collapses phases 1–7 of the #1186 memory architecture rework onto current `main` as a single merge. The stack forked 270 commits ago (176 of them touching `crates/`), so rebasing it layer by layer would mean solving the same 17 conflicts fourteen times, with no reviewer ever looking at the intermediate states. Closes the stack: #1252 #1263 #1279 #1283 #1301 #1341 #1349 #1426 #1440 #1448 #1454 #1462 #1465 #1468 #1533. **A review guide is in the first comment.** It is the part worth reading — this diff is 103 files, but only three decisions in it are ones a compiler cannot check. ## What was already reviewed, and what wasn't Every phase was reviewed and approved by a session that was not its author, under the rejection-lockout rule (a rejected author never writes the next revision). | Phase | PR | Verdict | |---|---|---| | 1–5 | #1252 → #1426 | approved | | 6 | #1440, #1448, #1454 | **rejected** ×3 | | 6 | #1462 | approved | | 7 | #1465 | **rejected** | | 7 | #1468 | approved | | 7 test fixes | #1533 | approved, A100-verified | The four rejected rounds were not replaced — the later rounds are stacked **on top of** them. So the tree here is the approved state, but the history contains the rejected commits. **#1440 / #1448 / #1454 / #1465 should not be reviewed individually**; they close automatically. **Not reviewed anywhere:** the conflict resolutions themselves. That is what this PR is for. ## Verification Run here, on this merged tree: - `cargo check --workspace --all-targets` — clean - `cargo check -p onnx-genai-engine --features cuda,native-backend --all-targets` — clean. Worth calling out: the default feature set does **not** compile the `cfg(cuda)` code, which is where the riskiest edits are. Checking only the default set would have missed a real break (see the guide). - `cargo test` over 7 crates — **1105 passed, 2 failed, 88 ignored** - `cargo clippy --workspace --all-targets` - `cargo fmt` **The 2 failures and the 1 clippy error are pre-existing and proven so, not merge damage:** - `platform_capacity::{disk_capacity_is_measured_for_the_working_directory, an_explicit_byte_limit_is_honored_without_a_device_query}` — `platform_capacity.rs` is byte-identical to `main` (`git diff origin/main HEAD -- ` that file is empty). The cause is a macOS-only FFI layout bug: `fsblkcnt_t` is 4 bytes on macOS (confirmed: `sizeof(struct statvfs)=64`, `sizeof(fsblkcnt_t)=4`) while the Rust struct declares `f_blocks`/`f_bfree`/`f_bavail` as `u64`. CI is Linux, where it is 8 bytes. Left alone: it is a real bug but not this PR's. - `optimizer.rs` clippy `approx_constant` — present on both parents; the merged file is byte-identical to `main`. - Three files had `rustfmt` drift already present on `main` (`matmul_nbits.rs`, `normalization.rs`, `optimizer.rs`). Reverted rather than swept in, to keep the diff readable. **Final A100 revalidation (head `13c37a7f`):** full CUDA-memory GPU suite passed; CUDA EP default-parallel lib suite passed (**488 passed / 17 ignored**); real fp16 GQA covered three requests × two interleaved decode steps with two shared peers and one transactional private fallback, byte-identical to independent GPU and CPU references. A device-started/event-gated worker remained healthy through a peer process `CUDA_ERROR_ILLEGAL_ADDRESS`, owner exit, replacement worker, and process restart. Governor tests, targeted Miri, CI Clippy, fmt, and CUDA honesty also passed. ## Still open after this merges - `memory-plugin-provider-wiring` — the back half of Phase 6 criterion 8 (provider/context pinning), which fell in the gap between phases. **#1186 must not be closed until it lands.** This merge makes it more tractable, not less: see decision 3 in the guide. - `memory-deferred-invariant-asserts` — two surviving mutants found by #1462's reviewer, adjudicated NON-BLOCKING and not defects. - #1533's CUDA honesty guard warns on a legitimately portable anchor. Should be moved out of the CUDA test binary or allow-listed rather than left warning. --------- Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Copilot-Session: 46c5d75b-8146-489c-b82f-08ee29c27ce4 Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100 Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
|
Closing: superseded by #1579, which collapsed phases 1–7 into a single merge — landed as This PR was rejected under the lockout rule and superseded by the approved revision that was stacked on top of it (not by a replacement of it). Per #1579's own guidance, it was never meant to be reviewed individually. Containment verified, not assumed: Every one of the 15 stack heads (#1252 → #1533) is a literal ancestor of the merged head, and I separately confirmed the merged head's content reached Why this didn't close itself: #1579's body says #1186 stays open: #1579 explicitly gates it on |
Part of #1186 — Phase 7 only.
Removes the built-in CUDA eager
cuMemAllocallocator and makes the VMM arena the sole built-in CUDA memory mechanism for EP-managed device allocations. Stacks on Phase 6 (justinchuby-sturdy-potato,21370921).What changed
The CUDA EP carried two built-in mechanisms — an eager
cuMemAllocallocator and the VMM arena — with the arena behind an opt-in flag and the eager allocator as the silent fallback when the arena could not be built. A missing capability quietly selected a different mechanism whose bytes were not charged the same way, and the operator's only evidence was a log line.ONNX_GENAI_CUDA_VMMis deleted, not deprecated.CudaVmmAllocator::build()already exercises the capability (cuMemAddressReserve, and the driver's reported allocation granularity, which a device without VMM support refuses). That failure now returnsErrthroughvmm_unavailable(...), naming the device ordinal, the driver's own message, the entry points that constitute the support boundary, any requested managed limit, andwith_memoryas the way out.DeviceAllocatoris untouched.with_memoryretires the arena and the injected mechanism serves everything afterwards. It is refused — before the offered allocator is used — only for a foreign device, or for a mechanism with memory outstanding. Both returnErr; no successful builder call is ignored.CudaMemoryisVmm | Injected. There is deliberately no third built-in-eager variant, and reintroducing the fallback does not compile (see M8).Behaviour change worth calling out
production_physical_pool_enabled()wasvmm_enabled() && pool_bytes.is_some()and is nowpool_bytes.is_some(). A caller who setONNX_GENAI_CUDA_PHYSICAL_HANDLE_POOL_BYTESwithout the removed flag previously had it ignored and now has it honoured. This is the intended consequence of the arena becoming unconditional — the pool bound was only ever meaningful for the arena — but it is a real change and is called out rather than buried.Deliberately not changed
auto_dynamic_lending.then_some(256 MiB)on the governed path is left as-is. It is tempting to give every governed provider a default retained pool now that VMM is universal, but the pool is authority-owned andadopt_memory_governorerrors whenarena.physical_pool_authority() != governor.authority_id(). Adding a default pool to the governed non-lending path would newly subject it to that check and could introduce adoption failures that cannot be tested on this host. Flagged for the CUDA host.Not a single CUDA path executed. Read the criteria table for exactly what that leaves unverified. Criterion 10 is outstanding. No benchmark was run, no number is estimated, and the box below is unticked on purpose.
Acceptance criteria
the_default_provider_allocates_through_the_built_in_vmm_arena, which is GPU-gated and did not execute.cuMemAllocthe_cuda_memory_crate_has_no_eager_allocation_sites— zeromalloc_sync(/free_sync(inonnx-runtime-cuda-memory/src. Ran here, green. Killed by M5.built_in_vmm_failure_is_fatal_and_names_the_support_boundary,a_requested_managed_limit_is_named_in_the_unavailability_diagnostic— both ran here, green, killed by M1. Boundary documented inMEMORY_MANAGEMENT_MODEL_DESIGN.md. No host without VMM support was available to produce a real refusal. See "no new capability probe" below.injection_is_refused_for_a_device_this_provider_does_not_serve,replacing_a_mechanism_that_already_served_memory_is_refused_on_both_axes— ran here, green, killed by M2/M3/M4. The authoritative swap itself isan_injected_external_eager_allocator_replaces_the_built_in_arena, GPU-gated, did not execute.impl DeviceAllocator for CudaVmmAllocatorand itsVirtualBacking/shared-mapping surfaces are untouched by this PR;the_memory_crate_provides_exactly_one_built_in_mechanismpins that it is the only one.CudaMemoryisVmm | Injected.VmmInitialization/resolve_vmm_initializationdeleted. M8 shows the old shape does not compile.--workspace --all-targets, plusonnx-genai-engine --features cuda,native-backend, plusonnx-runtime-ep-cuda --features gpu-tests). Every one of their behavioural tests is GPU-gated and did not execute. Compilation is not behavioural compatibility and is not claimed to be.cuMemAlloccallsno_built_in_eager_allocator.rs, 6/6, mutation-killed by M5/M6/M7). The "default reaches VMM" half is GPU-gated and did not execute.mapped_bytes/owned_bytes,allocation_bytes/unmapped_bytes) are untouched. No counter was observed at runtime, because none of these paths can run here.MEMORY_MANAGEMENT_MODEL_DESIGN.mdand in the beginner wiki.timemachinelabel + removal recorddocs/memory/MEMORY_MANAGEMENT_MODEL_DESIGN.md,docs/memory/MEMORY_ARCHITECTURE.md,docs/ep-plugin/EP_PLUGIN_EXPORT_INVENTORY.md,wiki/memory/Memory Management for Beginners.md.Why no new driver capability probe was added (criterion 3)
cudarc 0.19.8 does not expose
CU_DEVICE_ATTRIBUTE_VIRTUAL_MEMORY_MANAGEMENT_SUPPORTED. Using the raw attribute integer would add CUDA surface that cannot be exercised on the host this was written on, which is how untested code gets shipped. Instead the existing init-time exercise is made fatal:build()already callsgranularity()(errors on zero) andreserve()(cuMemAddressReserve), which is what a device without VMM support refuses. A CUDA host should confirm this produces the intended diagnostic on real unsupported hardware.Mutation testing
Every mutation was applied, the mutated line printed to confirm it landed, the suite run, and the source restored.
built_in_vmm_failure_is_fatal_and_names_the_support_boundaryreject_foreign_devicesilently accepts a foreign device (false && ...)injection_is_refused_for_a_device_this_provider_does_not_servecommittedaxis ofreject_live_mechanism_replacement..._refused_on_both_axesservedaxis ofreject_live_mechanism_replacement..._refused_on_both_axesDeviceAllocatorin the memory cratecuMemAlloccall site in the EP..._exactly_the_two_disclosed_onesthe_scan_can_observe_an_eager_call_site_that_is_known_to_existcrate::device_allocatordoes not exist, andCudaMemory::Allocatordoes not existSurvivors and disclosures
One survivor found, and it was in my own mutation harness, not the code. My first attempt at M3/M4 used a shell loop that split the before/after strings on
|— which is inside the expressionserved > 0 || committed > 0. The "mutation" therefore edited whitespace and nothing else, the suite stayed green, and I recorded a survivor that did not exist. This is Round 1's "fixtures that lied" reproduced one level up, in the tooling that judges the fixtures. It was caught by printing the mutated line; every mutation in the table above was re-run with that verification, and M3/M4 both kill.Disclosed weaknesses in the surviving assertions:
the_cuda_memory_crate_has_no_eager_allocation_sitesandthe_removed_type_and_flag_are_absent_from_production_codesurvive individually — a blinded scanner makes "count is zero" trivially true. This is inherent to negative structural assertions and is exactly whythe_scan_can_observe_an_eager_call_site_that_is_known_to_existexists as an anchor. The anchor dies under M7. The two negative tests should never be read without it.the_removed_type_and_flag_are_absent_from_production_codeexcludes comment lines, so it cannot see a removed name mentioned in prose. That is deliberate (criterion 12 requires the removal to stay explained invmm_allocator.rs), and the exclusion is itself pinned bythe_removal_stays_explained_in_prose_and_the_code_scan_can_tell_the_difference, which asserts the raw scan sees the mention and the code scan does not — so the two scans provably differ on a live case.commits_on_demand()is a behavioural property of the live mechanism (the arena maps granules on demand; an eager allocator takes physical memory when asked), not a counter the provider sets about itself, and the test contains an explicit premise assertion proving an eager allocator reportsfalse— so "the default reached something else" cannot pass. It still did not execute here, and I am not claiming it did.Validation actually performed on this host
cargo check --workspace --all-targets --exclude onnx-genai-benchcargo check -p onnx-genai-engine --features cuda,native-backend --all-targetscargo check -p onnx-runtime-ep-cuda --features gpu-tests --all-targetsmemory-abi+memory-host+memory-testplugin)no_built_in_eager_allocator.rs(new, non-GPU)cargo fmt --checkon changed cratescargo clippy --all-targetson changed cratescargo doc --no-depson changed crates459 ignored is almost entirely GPU-gated tests. That number is the honest size of what could not run.
Not run here: every
#[cfg_attr(not(feature = "gpu-tests"), ignore)]test; the full workspace test suite (blocked by the known pre-existing macOS MLAS link failure inmlas-sys); all benchmarks.Pre-existing and untouched:
completions.rsrustfmt drift,matmul_nbits_marlin_numerics, Windows CUDAunnecessary_unwrap, macOS MLAS,onnx-runtime-memory-governorrustdoc links.onnx-runtime-ep-cpualso failsclippy -D warningsat the base commit (verified by stashing) — also not mine.What a CUDA host must still check before this is mergeable
device_allocator_gpu.rsand everyprovider::testsGPU test with--features gpu-tests. Four of them are new or rewritten.reserve()/granularity()failing rather than from a capability attribute.nsys/ncuforcuMemAlloc_v2in the steady-state decode region; only the two disclosed kernel-scratch sites should appear.Criterion 12 — removal record (
timemachine)Removed types
CudaDeviceAllocator—crates/onnx-runtime-cuda-memory/src/device_allocator.rs(whole file, 206 lines)QuarantinedCudaAllocation— same fileVmmInitialization<T>—crates/onnx-runtime-ep-cuda/src/provider.rsCudaMemory::Allocatorvariant — renamed toCudaMemory::InjectedRemoved flags
ONNX_GENAI_CUDA_VMM(constantCUDA_VMM_ENV) — the arena on/off switchRemoved functions / call paths
vmm_allocator::vmm_enabled()provider::resolve_vmm_initialization()eager()construction closure and theif vmm_enabled() || auto_dynamic_lendingselection branch inCudaExecutionProviderconstructionpub use onnx_runtime_cuda_memory::device_allocatorre-export fromonnx-runtime-ep-cudaconstruction_selected_vmm_rejects_injection_instead_of_ignoring_it,default_allocator_cumemalloc_scales_one_for_one_with_requests; unit testsmanaged_vmm_failure_is_fatal_before_allocator_fallback,compatibility_vmm_failure_keeps_fallback_availableLast commit that supported them:
21370921(Phase 6 head, base of this PR)Why removed: the eager allocator existed only as the fallback for a VMM arena that was not yet the default. Once the arena is the default, keeping it means keeping a path that can be entered silently, whose allocations are not charged the way the arena's are, and whose selection is invisible except in a log line. Criterion 6 names the dual-mechanism provider state directly. The capability is preserved through
DeviceAllocatorinjection, so nothing a caller could do before is now impossible — it just has to be asked for explicitly.How to recover the implementation:
Note that
tests/device_allocator_gpu.rsin this PR containsExternalEagerAllocator, a working eagercuMemAllocallocator built from nothing but public API — which is both a test fixture and the migration example for anyone who needs the removed behaviour back.Do not merge. This stays open for human review, like every PR in the memory stack.