Skip to content

fix(cuda.core): order VMM unmaps on the stream - #2889

Draft
aryanputta wants to merge 5 commits into
NVIDIA:mainfrom
aryanputta:aryan/fix-vmm-stream-ordering-2886
Draft

aryanputta wants to merge 5 commits into
NVIDIA:mainfrom
aryanputta:aryan/fix-vmm-stream-ordering-2886

Conversation

@aryanputta

Copy link
Copy Markdown
Contributor

Summary

Fixes #2886.

VirtualMemoryResource.deallocate() now synchronizes a supplied stream before calling cuMemUnmap. This preserves the stream-ordering contract documented by Buffer.close() and matches the synchronous resource implementations.

Changes

  • synchronize the validated stream before VMM unmapping;
  • update the deallocation docstring;
  • add regression coverage proving synchronization occurs before unmapping.

Validation

  • pre-commit passed: Ruff, Ruff format, SPDX, mypy, and repository checks;
  • git diff --check passed;
  • full CUDA/GPU pytest validation was unavailable on the macOS development host.

The branch is DCO signed and contains only the issue-focused changes.

Signed-off-by: Aryan <aryansputta@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Andy-Jost Andy-Jost 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 PR. VMM work is consolidated under #2906, and this PR is on the list to land for cuda.core 1.3.0. Before review:

#2886 is a real gap, and the sync is the right fix for device-located buffers. One regression to fix first: host-located resources record no deallocation stream (is_device_accessible is False), so the deleter passes an unbound default stream, and .sync() raises when no context is current. The callback then reports a CUDAWarning and skips unmap and address-free, which leaks. Guard the sync on self.is_device_accessible.

For the test, drop the monkeypatch and add a driver test that allocates and closes one buffer from a device-located resource and one from a host-located resource with an explicit stream, asserting no CUDAWarning. Rebase on main (#2235 has merged; deallocate() no longer retains or releases a handle) and add a 1.3.0 release note.

Please keep discussion of the overall plan on #2906.

aryanputta commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Changed:

  • Guarded stream synchronization with self.is_device_accessible so host-located resources do not try to sync an unbound default stream during teardown.
  • Replaced the monkeypatch regression with a real-driver test covering explicit-stream close for both device- and host-located VMM resources, asserting no CUDAWarning.
  • Added the cuda.core 1.3.0 release note.

Local validation passed: Ruff, Python syntax compilation, and git diff --check. Full CUDA/GPU pytest validation is unavailable on this macOS host. The branch also remains based on the current PR base used by #2889; please rebase if the main branch advanced after #2235.

@Andy-Jost Andy-Jost 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 rework. The device-side guard and sync are right, and the real-driver test is the right shape. Three things block this as it stands:

  1. The merge with main reintroduced the leak that #2235 fixed. deallocate() now calls cuMemRetainAllocationHandle(ptr) and never releases the handle it gets back, so every close leaves the physical allocation with one extra reference and it is never freed. Drop that call: on main, the mapping alone owns the allocation and cuMemUnmap frees it.
  2. The release-note edit removed an existing entry. The bullet for #2876 (the Program destructor fix) was replaced instead of a new bullet being added. Restore it and add the new bullet after it, citing #2886 rather than this PR.
  3. The host case of the new test cannot pass on a GPU. VirtualMemoryResourceOptions(location_type="host") keeps the default posix_fd handle type, which the driver rejects for host memory, so allocate() fails before the test reaches close(). Pass handle_type=None for the host case. The test also needs @pytest.mark.thread_unsafe(reason=...), because assert_no_cuda_warning captures process-global warnings.

#2917 (the redesign from #2906) fixes #2886 in the new implementation and removes the file this PR edits, so these changes would not survive a rebase once it lands. Unless this needs to land first, we suggest closing this PR as superseded when #2917 merges.

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

Labels

cuda.core Everything related to the cuda.core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cuda.core: VirtualMemoryResource.deallocate() unmaps without ordering on the stream

2 participants