Skip to content

fix(cuda.core): move VirtualMemoryResource onto the _rt handle layer - #2917

Open
Andy-Jost wants to merge 14 commits into
NVIDIA:mainfrom
Andy-Jost:ajost/vmm-redesign
Open

Andy-Jost wants to merge 14 commits into
NVIDIA:mainfrom
Andy-Jost:ajost/vmm-redesign

Conversation

@Andy-Jost

@Andy-Jost Andy-Jost commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Moves VirtualMemoryResource onto the _rt handle layer, as planned in #2906 (design). Each physical allocation, address reservation and mapping is a std::shared_ptr handle with a deleter that knows the exact driver call to undo it. A buffer owns a range of mappings through its device pointer handle, so everything a buffer maps is released when the last buffer that maps it closes, and a failed multi-step operation unwinds by letting its local handles die. Ranges are immutable: a grow copies the input's mapping list and builds a new range for its result, so two buffers never share mutable state and each buffer owns exactly one range and one recorded stream. The module moves from Python to Cython. The design lands as cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.md.

Behavior changes

  • modify_allocation returns a new VirtualMemoryBuffer and leaves the buffer passed in open. The two alias the same physical memory, which is freed when the last of them closes. The pointer is preserved when the driver grants the adjacent address range. The result is always a new buffer, so closing it never closes the buffer passed in; a request the buffer already covers returns a full alias without a driver call.
  • Buffer.size after a grow is the aligned total.
  • config= applies to the chunk the call adds and is no longer stored on the resource. It must keep the resource's location and passes the constructor's option checks, including the RDMA support check.
  • Buffers from allocate() free themselves when they close and no longer call deallocate(), which now serves pointers wrapped with Buffer.from_handle.
  • A buffer records the stream passed to allocate() and synchronizes it when it closes, before releasing its share of the mappings; a mapping another buffer still holds stays mapped. An explicit close() on a capturing stream other than a default stream raises; a release ordered on a default stream that would disturb a capture in its context is reported as a CUDAWarning and unmaps without the synchronization. The synchronization runs in relaxed capture mode, so it does not invalidate a capture on an unrelated stream.
  • modify_allocation accepts only buffers this resource returned.
  • location_type="host" requires handle_type=None, which the driver requires.
  • allocate(0) returns an empty buffer without a driver call.

Testing

  • The mocked grow test is replaced by real-driver tests. Release is checked per address, by asking the driver that no mapping and no reservation remain at the exact addresses a buffer used, instead of comparing a device-wide free-memory count that other processes can move. Covered: release in three modes (allocate, grow, forced move), grow in place with the pointer preserved, grow that moves with contents preserved, aliasing and cross-alias visibility, unaligned and repeated grows, a failed grow that leaves the input intact, the capture check on close with and without a stream argument, a release from garbage collection during a capture, a close during an unrelated global-mode or thread-local capture, stream synchronization at close in both close orders (deterministic, with a sleep kernel and events), cuMemGetAccess on both chunks after a config= grow, the per-call config checks, the raw-pointer deallocate() path, host location without a current context, size zero with an inherited stream, buffers alive at interpreter shutdown, and four threads growing and closing aliases of one buffer at once, reduced from the reproduction in review.
  • The graph in-flight retention test is parametrized over the device pool and the virtual memory resource: a memcpy node keeps a virtual memory buffer mapped after close() until the launch completes and the graph is gone. A second test keeps the range mapped across a grow and the close of every alias.
  • On an H100 with CUDA 13.4 (driver 615), the full cuda_core suite passed: 4325 passed, 100 skipped, 1 xfailed, 0 failed. The VMM selection passed in fixed order (40 passed, 1 skipped for GPUDirect RDMA) and twice in random order, and the reproduction script from review runs to completion without a crash.

Issues

Fixes #2887
Fixes #2907
Fixes #2908
Fixes #2909
Fixes #2886
Fixes #2345
Fixes #2877

Addresses #2388 items 1, 2 and 3 (the rollback that lost access grants, the dead fast path, and the finalizer warnings; item 4 landed in #2418) and the size-0, misaligned-probe and host handle-type parts of #2910. Part of #2906. Supersedes #2880, #2889 and #2237: they edit the module this PR replaces, and the fixes they carried (#2877, #2886, #2345) are part of the redesign.

🤖 Generated with Claude Code

Each physical allocation, address reservation and mapping now lives in
a std::shared_ptr handle whose deleter knows the exact driver call to
undo it. A buffer owns a range of mappings through its device pointer
handle, so everything a buffer maps is released when the last buffer
that maps it closes, and a failed multi-step operation unwinds by
letting its local handles die. The module moves from Python to Cython.
The design is in cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.md.

Behavior changes:

- modify_allocation returns a new VirtualMemoryBuffer and leaves the
  input open; the two alias the same physical memory, which is freed
  when the last of them closes. The pointer is preserved when the
  driver grants the adjacent address range.
- Buffer.size after a grow is the aligned total.
- config= applies to the chunk the call adds and is not stored on the
  resource.
- Buffers from allocate() free themselves on close and do not call
  deallocate(), which now serves pointers wrapped with
  Buffer.from_handle.
- A buffer records the stream passed to allocate(); the last close of
  an aliased range synchronizes every recorded stream before it unmaps.
  An explicit close on a capturing stream raises.
- location_type="host" requires handle_type=None. allocate(0) returns
  an empty buffer without a driver call.

Fixes NVIDIA#2887
Fixes NVIDIA#2907
Fixes NVIDIA#2908
Fixes NVIDIA#2909
Fixes NVIDIA#2886
Fixes NVIDIA#2345
Addresses NVIDIA#2388 item 2 and the size-0, misaligned-probe and host
handle-type parts of NVIDIA#2910. Part of NVIDIA#2906.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Andy-Jost Andy-Jost added this to the cuda.core 1.3.0 milestone Sep 18, 2026
@Andy-Jost Andy-Jost added bug Something isn't working P1 Medium priority - Should do cuda.core Everything related to the cuda.core module labels Sep 18, 2026
@Andy-Jost Andy-Jost self-assigned this Sep 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Andy-Jost and others added 4 commits September 18, 2026 14:29
The child interpreter inherited pytest's working directory, cuda_core/,
so `import cuda.core` resolved to the uncompiled source tree in CI and
failed on `cuda.core._version`. Use the shared run_python_snippet
helper, which starts the child in an empty temporary directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Run the range deleter's stream sync in relaxed capture mode, so a
  capture on an unrelated stream is not invalidated.
- Record a real stream on allocate(0) and inherit it on the grow.
- Require handle_type=None for location "host" only.
- Apply the constructor's option checks to a per-call
  modify_allocation config, including the RDMA support check.
- Narrow the close() capture contract to non-default streams in the
  docstring, design doc and release note.
- Tests: failed grow leaves the input intact, close during an unrelated
  capture, GC release during capture, deterministic stream sync with a
  sleep kernel, cuMemGetAccess on both chunks, graph retention across a
  grow, forced-move leak on 2 MiB that fails rather than skips.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The struct setter in cuda-bindings 13.0 accepts only the enum, and the
Cython helper returns a plain int.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-access case can call this with count == 0 and descs.data() from an empty vector. Constructing access(descs, descs + count) then does pointer arithmetic/range construction on a possibly null pointer. Since self_access=None with no peers is supported, could this special-case zero descriptors before forming the range?

@Andy-Jost Andy-Jost added the PR review get-together Mark PRs you'd like the team to review at the weekly PR review get-together. label Sep 21, 2026
// Copy the descriptors before the driver call so a failed copy leaves
// nothing to undo.
std::vector<CUmemAccessDesc> access(descs, descs + count);
GILReleaseGuard gil;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-access case can call this with count == 0 and descs.data() from an empty vector. Constructing access(descs, descs + count) then does pointer arithmetic/range construction on a possibly null pointer. Since self_access=None with no peers is supported, could this special-case zero descriptors before forming the range?

@sylvesterkaczmarek This is well-defined C++ as written. The standard ([vector.data] C++17, N4659 section 26.3.11.4) guarantees that [data(), data() + size()] is a valid range. This is true whether or not data() returns a null pointer, since zero added to a null pointer yields a null pointer (see [expr.add] C++17, N4659 section 8.7). No workaround appears necessary here. This case is covered in test_vmm_allocate_without_access_descriptors.

@Andy-Jost Andy-Jost removed the PR review get-together Mark PRs you'd like the team to review at the weekly PR review get-together. label Sep 21, 2026

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the correction. The C++17 vector::data contract explicitly requires [data(), data() + size()) to be a valid range, including the empty-vector case, and the dedicated no-access regression covers this path. My concern is resolved.

Andy-Jost and others added 3 commits September 21, 2026 15:52
…d size overflow

Add tests for a host-located grow that moves, for a release ordered on the
legacy default stream while a blocking stream in its context is capturing,
and for a size whose rounding to the granularity does not fit in size_t. The
forced-move test now asserts that neither the grow nor the closes warn
(NVIDIA#2877).

_align_up raises OverflowError instead of wrapping. The docstrings and the
release note say that config has no effect when the buffer already covers
the request and never changes the access of mapped memory, and that a
host-located resource records no default stream. VMM_DESIGN.md describes the
close() override.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…SIGN.md

List the fifteen properties the handle-based VirtualMemoryResource
maintains: once-only and ordered release of reservations and allocations,
what a failed or successful grow leaves behind, stream ordering and graph
capture, ownership by graph nodes and aliases, per-chunk access, range
layout and rounding, the base-address registry, the deallocate() contract,
context independence, and interpreter shutdown. The wording names no
mechanism, so the list stays valid if the release is made stream-ordered.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@0z5a

0z5a commented Sep 28, 2026

Copy link
Copy Markdown

Independent Jetson Thor / ARM64 validation of head 1f9c014b4c1f2d35fa404fd5bde2326b1ee96111, compared with base 530898e6448daf7e6d5a52dabdfd42abf38eb0f4 (CUDA Toolkit 13.2.78, driver 595.78, cuda-bindings 13.2.0). Both revisions were built from source in separate environments; the compiled head extension came from the head checkout. I am following #2906 and am not opening a competing VMM implementation PR.

Results on the real CUDA driver:

  • Head pytest cuda_core/tests/test_memory.py -k vmm -vv -ra: 39 passed, 1 skipped, in each of three consecutive repeats after pausing this validation run's other build processes. Base's own selection: 16 passed, 1 skipped. The base suite does not have the head's additional ownership/grow/lifetime coverage.
  • Head graph selection, pytest cuda_core/tests/graph/test_graph_definition_lifetime.py -k 'vmm or inflight_launch_retains_attachments' -vv -ra: 3 passed.
  • Existing head tests executed the in-place and forced-move grow paths, content/alias preservation, repeated/unaligned grow, allocation release, graph/capture restrictions, and rollback/overflow checks. The sole VMM skip is the policy-configuration test requiring GPUDirect RDMA.
  • Direct device queries: VMM, POSIX-FD handles, host VMM and host-NUMA VMM are supported; GPUDirect RDMA and fabric handles are not supported on this device.
  • Direct cuMemGetAllocationGranularity queries returned 2,097,152 bytes for both minimum and recommended, for both non-exportable and POSIX-FD allocation properties. Sixteen supplemental allocations exercised g-1, g, g+1, and 2g+1 for both handle modes/policies, checking the rounded buffer size and actual driver writes/reads at both ends; all passed.

One qualification: the first non-isolated head run was 38 passed / 1 skipped / 1 failed at test_vmm_deallocate_frees_wrapped_pointer. The assertion observed a 2,228,224-byte free-memory delta against a 2,097,152-byte allocation while other builds/transfers were active. It did not recur in the three repeats; no threshold or implementation was changed. This observation does not establish a leak or its cause, but the global free-memory assertion may be sensitive to concurrent activity on unified-memory systems. The existing inference service remained active throughout.

This is platform-specific evidence, not an approval of every ownership/API design choice. No Windows/fabric/RDMA execution coverage is claimed.

Comment thread cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi Outdated
Comment thread cuda_core/cuda/core/_memory/_virtual_memory_resource.pyx Outdated

cdef inline bint _is_default_token(cydriver.CUstream s) noexcept nogil:
cdef uintptr_t h = <uintptr_t>s
return h == 0 or h == <uintptr_t>cydriver.CU_STREAM_LEGACY or h == <uintptr_t>cydriver.CU_STREAM_PER_THREAD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have the same purpose as

cdef inline bint Stream_is_default_token(Stream self) noexcept nogil:
"""Return True for CU_STREAM_LEGACY and CU_STREAM_PER_THREAD.
These tokens carry no context of their own; they refer to whatever context
is current, so nothing resolved from one may be cached on the object.
"""
cdef uintptr_t h = <uintptr_t>as_cu(self._h_stream)
return h == <uintptr_t>cydriver.CU_STREAM_LEGACY or h == <uintptr_t>cydriver.CU_STREAM_PER_THREAD
except the h==0 detail. We should only need one or at least one implementation both agree on right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're also importing/using the above already in this file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll refactor to eliminate the duplication.

@brandon-b-miller

Copy link
Copy Markdown
Contributor

A broad question about api.hpp. This pings a little like a public file / something that aspires towards true c++ interop. But it seems like it isn't yet, there's no statement about api stability or so forth. At the same time, it looks like we're shipping it today in our wheels tempting someone to maybe link against it. What's the end goal here?

@Andy-Jost
Andy-Jost requested a review from rparolin September 28, 2026 17:58
@Andy-Jost

Copy link
Copy Markdown
Contributor Author

A broad question about api.hpp. This pings a little like a public file / something that aspires towards true c++ interop. But it seems like it isn't yet, there's no statement about api stability or so forth. At the same time, it looks like we're shipping it today in our wheels tempting someone to maybe link against it. What's the end goal here?

api.hpp is the public interface to a private layer that might become shared in the future. We are covered in terms of policy risk; cuda-core is protected because that file appears in a private module (_rt) and the support policy reserves the right to change those unconditionally. For now, this is just cuda-core's runtime layer, implemented in C++.

Andy-Jost and others added 2 commits September 28, 2026 13:50
… the default-token check

`VirtualMemoryResource.device` and `.config` are now `cdef readonly`; no
other resource in the layer exposes writable attributes. The raw-handle
default-stream check moves to the stream module as
`Stream_handle_is_default_token`, and `Stream_is_default_token` delegates
to it, so the two modules agree on one definition. `VirtualMemoryBuffer.
close()` treats an empty handle as "no stream recorded" explicitly instead
of folding it into the token check.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… free memory

The counter from cuMemGetInfo covers the whole device, so any other
process moves it and the tests fail on shared machines. Each test now
asks the driver about the exact addresses it used after close: the
mapping lookup must fail and freeing the reservation must fail because it
no longer exists. Closes run under assert_no_cuda_warning, so a failed
unmap, address free or release fails the test. The leak test lists one
reservation per allocate and one more per grow, for both the in-place and
the moved case. One deterministic pass replaces the eight-iteration loop.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Andy-Jost and others added 4 commits September 28, 2026 17:39
A range is now an immutable list of mapping handles that lives in the
buffer's device pointer box. A grow copies the input's list, appends or
replaces mappings, and builds a new range for its result; the input's
range never changes. Mapping handles are shared between ranges, so each
mapping unmaps when the last range that holds it goes.

Two buffers therefore never share mutable state, which is what made
concurrent grows of aliased buffers unsafe: the shared range's mapping
vector was appended and iterated without a lock, and a grow that lost a
race could dereference a handle another thread had emptied. With one
range and one recorded stream per buffer, teardown follows the ordinary
Buffer model, so the stream union, the range mutex, the base-address
registry and the range header are gone.

modify_allocation never returns its input any more. A request the buffer
already covers returns a full alias without a driver call, so closing
the result never closes the buffer passed in. It also reads the input's
handle once, so a close from another thread defers the release instead
of emptying what the call reads.

The box behind a VMM handle is a VmmDevicePtrBox, a DevicePtrBox with
the range as a member and no virtual functions. Every handle on a
VirtualMemoryBuffer comes from deviceptr_create_vmm, including the
size-zero buffer, which now sits on a VMM box with an empty range, so
the class check in modify_allocation is what makes the downcast valid.

Adds a test that grows and closes aliases of one buffer from four
threads, reduced from the report on this pull request.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ndle

deallocation_stream() was the one accessor in the layer that
dereferenced an empty handle instead of returning empty, as its sibling
set_deallocation_stream and every as_cu() overload do. A buffer closed
by one thread while another still reads its handle now gets an empty
stream rather than a crash.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Join each worker with the suite's sanitizer-aware timeout and assert
that none is still alive before the shared buffers are closed, as the
other threading tests do. Drop "undefined" from the docstring.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Andy-Jost

Andy-Jost commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Since the last round of review, one design change was applied, prompted by the segfault Brandon found, and a few smaller changes came with it.

The design change: VmmRange is now an immutable list of mapping handles that lives in the buffer's own box, a VmmDevicePtrBox that inherits DevicePtrBox with no virtual functions. A grow copies the input's list and builds a new range for its result, so two buffers never share mutable state, and each buffer owns one range and one recorded stream. That removed the stream union in the range deleter, the range mutex, the base-address registry, and vmm_range.hpp.

What came with it:

  • modify_allocation always returns a new buffer, so closing the result never closes the input.
  • A new test grows and closes aliases of one buffer from four threads, reduced from Brandon's script.
  • The release tests ask the driver about the exact addresses a buffer used instead of comparing device-wide free memory, which other processes can move.
  • device and config are read-only, and the default-stream check is shared with the stream module.

The PR body and the range section of VMM_DESIGN.md are current. Full suite on an H100 with CUDA 13.4: 4325 passed, 0 failed.

mapping this buffer was the last to hold unmaps, and its reservation frees and its allocation
releases as their last references go. This is the same model as every other `Buffer`: the
recorded stream orders the release of this buffer's memory, and a caller who touches that memory
from another stream must order that work before the close. The deleter blocks on the sync, like

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may under state what could manifest as somewhat difficult to reason about user behavior. One thing I didn't realize about VMM in general (which I guess makes sense in retrospect) is that the free is not a stream ordered operation, e.g. cuMemunmap doesn't take a stream parameter).

The result is that the caller has to use its own bookkeeping to decide when to issue the real free. This PR does that correctly it appears in virtual_memory.cpp, and blocks:

status = p_cuStreamSynchronize(s);

The potential weirdness here is that depending on how much work is queued, this might happen inside a reference cycle or some other weird part of the users python code that doesn't even relate to CUDA at all. How can I debug why my main thread is waiting for 20 seconds of GPU work at a random point?

This PR intends to move the entire VMM mechanism to c++ and I am +1 on that generally, but I wonder if this particular issue might be ameliorated by moving just the blocking, synching part of the process into some piece of the python layer?

the buffer releases them. :meth:`deallocate` is not involved in that path.
"""

cdef:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this class to a cdef class without adding __weakref__ here actually drops the __weakref__ the original class inherited from the python object class. I think it's probably worth keeping since this is a resource and it looks like most other cuda core resources inherit it or get it elsewhere.

# passes an unbound default-stream token, which has no context to
# synchronize in. There is nothing queued on it to wait for.
if self.device is not None or not Stream_is_default_token(s):
s.sync()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sync let's a deleter raise which violates the contract.

mr = VirtualMemoryResource(dev)
pool_buf = DeviceMemoryResource(dev).allocate(4096, stream=dev.default_stream)
# A: bound to a name -- destructor runs after the error propagates
named = Buffer.from_handle(int(pool_buf.handle), 4096, mr)
mr.modify_allocation(named, 4 << 20)
#   TypeError: buf must be a VirtualMemoryBuffer, got Buffer


# B: passed as a temporary -- destructor runs during unwinding
mr.modify_allocation(Buffer.from_handle(int(pool_buf.handle), 4096, mr), 4 << 20)
#   SystemError: error return without exception set
#   __context__: None

What's happening is modify_allocation raises TypeError, and while it propagates the temporary's deleter in deviceptr_create_with_mr acquires the GIL and calls mr.deallocate without saving the pending exception.

I asked an agent to suggest what to do about this, I think it's solution is pretty clean

Suggested fix. report_message already does the right thing at py_report.cpp:30 ("Deleters can run while a Python exception is propagating; keep it"). Factoring that into an RAII guard, which allocates nothing and is noexcept as the pw_ section requires:
struct PendingExceptionGuard {
    PendingExceptionGuard() noexcept {
#if PY_VERSION_HEX >= 0x030C0000
        pending_ = PyErr_GetRaisedException();
#else
        PyErr_Fetch(&type_, &value_, &tb_);
#endif
    }
    ~PendingExceptionGuard() noexcept {
        PyErr_Clear();  // drop anything the cleanup call itself raised
#if PY_VERSION_HEX >= 0x030C0000
        PyErr_SetRaisedException(pending_);
#else
        PyErr_Restore(type_, value_, tb_);
#endif
    }
    PendingExceptionGuard(const PendingExceptionGuard&) = delete;
    PendingExceptionGuard& operator=(const PendingExceptionGuard&) = delete;
private:
#if PY_VERSION_HEX >= 0x030C0000
    PyObject* pending_ = nullptr;
#else
    PyObject *type_ = nullptr, *value_ = nullptr, *tb_ = nullptr;
#endif
};

makes it one line in the deviceptr_create_with_mr deleter in memory.cpp:

            GILAcquireGuard gil;
            if (gil.acquired()) {
                PendingExceptionGuard keep_exception;
                if (mr_dealloc_cb) {

@brandon-b-miller brandon-b-miller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very solid. Couple more Q's

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cuda.core Everything related to the cuda.core module P1 Medium priority - Should do

Projects

None yet

4 participants