rocr: Close shareable dmabuf fds after use instead of holding them open#7786
rocr: Close shareable dmabuf fds after use instead of holding them open#7786dayatsin-amd wants to merge 1 commit into
Conversation
Defer creating the shareable dmabuf_fd until VMemorySetAccessPerHandle needs it for peer import, then close it again via a scope guard. This avoids leaking open fds for the lifetime of a VMM shareable handle while keeping fabric-handle export/import on the existing BO-based path. Co-authored-by: Cursor <cursoragent@cursor.com>
cfreeamd
left a comment
There was a problem hiding this comment.
I don't see any big problems, but have a few suggestions for consideration, but nothing blocking.
| @@ -4148,6 +4153,11 @@ Runtime::MemoryHandle::MemoryHandle(hsa_fabric_handle_t fabric_handle) | |||
| Runtime::MemoryHandle::~MemoryHandle() { | |||
| if (driver_handle.handle != 0 && region != nullptr) | |||
| agentOwner()->driver().DestroyMemoryHandle(&driver_handle); | |||
There was a problem hiding this comment.
Suggestion from Claude related to lines 4155 and the block at 4157. In a nutshell, it's saying if we derive from the base class in the future, we may miss doing the clean up that is happening here.
~MemoryHandle dual-cleanup coupling — runtime.cpp:4153
The destructor now has two cleanup sites:
if (driver_handle.handle != 0 && region != nullptr)
agentOwner()->driver().DestroyMemoryHandle(&driver_handle); // (A)if (driver_handle.dmabuf_fd >= 0) { // (B)
os::DmaBufClose(driver_handle.dmabuf_fd);
driver_handle.dmabuf_fd = -1;
}This is safe today because KfdDriver::DestroyMemoryHandle now zeroes dmabuf_fd before returning (added in this PR), so block (B) won't fire for
owned handles. But the two cleanup sites are coupled through an informal convention: any future Driver subclass that implements
DestroyMemoryHandle without zeroing dmabuf_fd will silently double-close the fd — a latent fd-aliasing bug with no compiler enforcement.Suggestion: Either make the base class zero dmabuf_fd after the virtual call, or merge the dmabuf_fd close into DestroyMemoryHandle entirely and
remove block (B), with a brief comment explaining the imported-handle path.
| * holding an fd open for the lifetime of the handle. Export it lazily here so the target agents | ||
| * can import the memory below, then close it again before returning. | ||
| */ | ||
| bool created_dmabuf_fd = false; |
There was a problem hiding this comment.
I'm not sure how often a single MemoryHandle is mapped across multiple VA chunks--if not often, then this probably isn't worth worrying about, but here's Claude feedback on this:
[MINOR] Per-chunk re-export amplification — runtime.cpp:4166
When a single MemoryHandle is mapped across multiple VA chunks (e.g., the same handle mapped twice via VMemoryHandleMap), VMemorySetAccess calls
VMemorySetAccessPerHandle once per chunk. Each call finds dmabuf_fd == -1 (the scope guard reset it) and does a fresh ExportMemoryHandle → kernel
ioctl. Cost becomes O(chunks × agents) rather than O(agents). Not a correctness issue, but worth lifting the lazy-export out to the per-handle
level in VMemorySetAccess if this path gets hot.
Summary
dmabuf_fdopen for the entire lifetime of a VMM shareable handle;CreateShareableHandlenow leavesdmabuf_fdas-1after converting the allocation to a driver handle.VMemorySetAccessPerHandlewhen peer import needs it, then close it again via a scope guard before returning.DriverMemoryHandle*throughImportMemoryHandle(reading.dmabuf_fdor.fabric_handle) and close fds inMemoryHandledestructor /DestroyMemoryHandle.Fabric-handle export/import is unchanged and continues to use the internal BO handle path.
Test plan
libhsa-runtime64on S83-1 (MI300X) against TheRock toolchainUnit_hipMemExportToShareableHandle_Positive_Basic— passUnit_hipMemImportFromShareableHandle_Positive_Basic— passVirtualMemoryManagementTestsuite (85 cases,HIP_VISIBLE_DEVICES=0): 69 passed / 6 failed / 10 skippedMade with Cursor