Revert "fix(rocr): Dup dmabuf fd on VMemoryImportShareableHandle for deferred import"#8436
Merged
rahulc-gh merged 1 commit intoJul 11, 2026
Conversation
…deferred…" This reverts commit 8d1a7f6.
rahulc-gh
requested review from
atgutier,
cfreeamd,
dayatsin-amd,
kentrussell and
shwetagkhatri
as code owners
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reverts a prior ROCr change that duplicated DMA-BUF file descriptors during hsa_amd_vmem_import_shareable_handle, and removes the associated test coverage that validated correct FD lifetime behavior for deferred import.
Changes:
- Removes
os::DmaBufDupAPI and its Linux/Windows implementations. - Reverts
Runtime::VMemoryImportShareableHandleto store the caller-provideddmabuf_fddirectly (no duplication). - Removes the rocrtst subtest that closed the importer FD immediately after import and then called
set_access.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocr-runtime/runtime/hsa-runtime/core/util/win/os_win.cpp | Removes DmaBufDup Windows stub. |
| projects/rocr-runtime/runtime/hsa-runtime/core/util/os.h | Removes DmaBufDup declaration. |
| projects/rocr-runtime/runtime/hsa-runtime/core/util/lnx/os_linux.cpp | Removes DmaBufDup implementation (was dup(2) wrapper). |
| projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp | Reverts import path to keep caller FD (no owned duplicate). |
| projects/rocr-runtime/rocrtst/suites/test_common/main.cc | Stops invoking the FD-lifetime regression test. |
| projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.h | Removes declarations for the FD-lifetime regression test helper(s). |
| projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.cc | Removes implementation of the FD-lifetime regression test and drops <unistd.h>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
4461
to
4465
| hsa_status_t Runtime::VMemoryImportShareableHandle(int dmabuf_fd, | ||
| hsa_amd_vmem_alloc_handle_t* memoryOnlyHandle) { | ||
| /* The per-GPU import of this dmabuf is deferred until hsa_amd_vmem_set_access is called, but the | ||
| * caller is free to close their fd as soon as this function returns. Duplicate the fd so the | ||
| * MemoryHandle owns a copy that stays valid until the deferred import. The MemoryHandle destructor | ||
| * closes this owned fd. */ | ||
| int owned_fd = os::DmaBufDup(dmabuf_fd); | ||
| if (owned_fd < 0) return HSA_STATUS_ERROR_OUT_OF_RESOURCES; | ||
|
|
||
| std::lock_guard<std::shared_mutex> lock(memory_lock_); | ||
| auto memoryHandle = std::make_unique<MemoryHandle>(owned_fd); | ||
| auto memoryHandle = std::make_unique<MemoryHandle>(dmabuf_fd); | ||
| *memoryOnlyHandle = MemoryHandle::Convert(memoryHandle.get()); |
Comment on lines
551
to
554
| vmt.CPUAccessToGPUMemoryTest(); | ||
| vmt.GPUAccessToCPUMemoryTest(); | ||
| vmt.GPUAccessToGPUMemoryTest(); | ||
| vmt.ImportedShareableHandleSetAccessAfterFdClose(); | ||
| RunCustomTestEpilog(&vmt); |
amd-justchen
approved these changes
Jul 11, 2026
amd-justchen
left a comment
amd-justchen
left a comment
Contributor
There was a problem hiding this comment.
Approve revert since https://github.com/ROCm/TheRock/actions/runs/29129964526/job/86485750376?pr=6483#step:20:579 failing in ROCm/TheRock#6483 bump pr
rahulc-gh
deleted the
revert-8304-users/jaypatel/vmem-import-dmabuf-fd-lifetime
branch
3 tasks
dayatsin-amd
added a commit
that referenced
this pull request
Jul 11, 2026
… import (#8440) ## Summary Re-land the dmabuf fd duplication fix from #8304, which was reverted in #8436 to unblock TheRock R-S bump. After #6471 deferred per-GPU dmabuf import to `hsa_amd_vmem_set_access` time, `VMemoryImportShareableHandle` must own a dup of the caller's fd so deferred import still works after the caller closes its fd (normal IPC ownership). This restores correct behavior for multi-rank VMM/IPC peer-access flows (e.g. RCCL CE collectives). Also removes `LogPrint` from `DmaBufDup` on Linux to avoid rocdxg WSL link issues (same pattern as #8436 follow-up). Adds a rocrtst regression test that imports a shareable handle, closes the caller fd before `set_access`, and verifies GPU access succeeds. ## JIRA ID JIRA ID : ROCM-27777 ## Test plan - [x] Run `ImportedShareableHandleSetAccessAfterFdClose` rocrtst subtest on Linux with VMM support - [x] Verify HIP/RCCL multi-rank VMM peer-access reproducer from ROCM-27777 (hip-allgather-batch-cumem1.cpp) - [x] Confirm WSL/rocdxg build succeeds without LogPrint link errors in `DmaBufDup` Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Reverts #8304 blocking TheRock R-S bump PR ROCm/TheRock#6483