fix(rocr,clr): refresh owning agent for dmabuf-imported VMM handles by AidanBeltonS · Pull Request #14279 · ROCm/rocm-systems

AidanBeltonS · 2026-10-26T17:16:47Z

Motivation

RCCL has seen a significant performance regression in throughput bound operations with memcpy-batch. This is due to the copy engine not being selected when performing the operation. This PR aims to regain the lost performance

Technical Details

ROCr-Runtime: When imported memory has a DMA buffer backing we can retrieve the device agent. So we return this instead of an empty agent where possible
CLR: Update the refresh of the imported agent cache to pull in this value and store it for use.

Performance: 8 GPU memcpy-batch async benchmark: Develop 21588.01us vs VMM-import branch 10954.91 us. This is a toy benchmark so the exact change seen in RCCL should take this into consideration. I am able to provide further performance results offline

To allow quick testing of this features performance and potential bugs they are both guarded with flags.
ROCR changes are guarded with flag: HSA_ENABLE_VMEM_IMPORT_OWNER
CLR changes are guarded with flag: ROC_VMM_REFRESH_OWNING_AGENT

Issue Tracking

JIRA ID: ROCM-33394

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

therock-pr-bot · 2026-10-26T17:17:43Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/clr/rocclr/device/rocm/rocdevice.cpp`, `projects/clr/rocclr/device/rocm/rocdevice.hpp`, `projects/clr/rocclr/device/rocm/rocmemory.cpp`, `projects/clr/rocclr/device/rocm/rocmemory.hpp`, `projects/clr/rocclr/device/rocm/rocvirtual.cpp` (+4 more); no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot

🟡 Changes recommended

Copy and fill paths can select a GPU without mapping access, and the ownership test fails with the supported rollback flag disabled.

2 open issues

What changed in this PR

This PR aims to restore memcpy-batch throughput by reporting recoverable imported-memory ownership in ROCr and refreshing CLR’s cached owning agents after mapping.

Changes:

Report exporting GPU ownership for supported DMA-buffer imports, with a rollback flag.
Centralize CLR’s post-map ownership refresh behind a separate flag.
Extend imported-memory pointer-info tests and add safeguards for missing owners.

File	Description
`projects/rocr-runtime/runtime/hsa-runtime/core/util/flag.h`	Add the imported-owner reporting flag.
`projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp`	Recover imported ownership and handle missing owners.
`projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.h`	Declare the host-backed import test.
`projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.cc`	Check ownership for GPU, fabric, and host-backed imports.
`projects/clr/rocclr/utils/flags.hpp`	Add the ownership-refresh flag.
`projects/clr/rocclr/device/rocm/rocvirtual.cpp`	Delegate ownership refresh to shared map bookkeeping.
`projects/clr/rocclr/device/rocm/rocmemory.cpp`	Preserve cached ownership when queries cannot resolve it.
`projects/clr/rocclr/device/rocm/rocdevice.hpp`	Declare the ROCm ownership-refresh override.
`projects/clr/rocclr/device/rocm/rocdevice.cpp`	Refresh ownership on existing context-device views and the dispatching device.
`projects/clr/rocclr/device/device.hpp`	Add a backend ownership-refresh hook.
`projects/clr/rocclr/device/device.cpp`	Invoke the hook after map bookkeeping.

_{Review effort: Balanced}

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — [CLR][ROCr-Runtime] Import owning agent correctly for memcpy-batch async

The ROCr-side defensive fixes in FillMemory, CopyMemory, and IPCCreate all guard real null-deref paths (importing memory can now yield agentOwner.handle == 0, and Agent::Convert(hsa_agent_t{0}) is a null pointer). The CLR RefreshMappedMemoryOwner override and the post-map ownership refresh logic in rocmemory.cpp look structurally sound. The VMemoryPtrInfo guard for imported handles (memHandle->region != nullptr checked before agentOwner()) reads correctly — memRegion is set to nullptr for imported handles so the old code path does not run for them. The auto-scanner's rocrtst_yaml_missing flag is a false positive here: HostBackedImportedHandlePointerInfo is a new private method called from the existing TestImportedHandlePointerInfo orchestrator — no new TEST()/TEST_P() macro registration.

[Required] PR title [CLR][ROCr-Runtime] Import owning agent correctly for memcpy-batch async does not follow the required type(component): format (ROCm squash-merges, so this becomes the permanent commit title). Suggested: fix(rocr): refresh owning agent for dmabuf-imported VMM handles

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update

Two prior questions addressed: EnsureImportInfo is now called in VMemoryHandleMap (gated by enable_vmem_import_owner()), and the early-return in refreshOwningAgentFromPointerInfo is documented as intentional (keeps create()-time agent on failure/zero-handle). The access-restriction safety question on VMemoryPtrInfo remains open (see inline). Title format (type(component):) is still required.

Nits in new code (commit 2): resport → report in HostBackedImportedHandlePointerInfo banner; independant → independent in VmemImportResolvedAgent comment.

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — follow-up re-review #2

No new commits since last re-review. One test correctness issue identified on re-analysis; prior [Question] on access-restriction safety and [Required] title format remain open.

mangupta

🤖 AI-assisted pre-review (for @mangupta)

LeoZhao-BJ

also need to add case in hip-tests to do functional check on rccl usage with copy engine path, but we can separate it into another PR to avoid confusion on the goal of this fix.

…cking

…ehaviour

mangupta · 2026-10-27T14:46:49Z

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (fc84a3d72d92) — no new findings at or above the posting threshold.

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author update

Prior access-restriction safety concern resolved: OwnerCanBlit correctly gates blit dispatch on VA accessibility for both CopyMemory and FillMemory. Test rollback-mode fix (VmemImportOwnerEnabled()) is clean. Two new [Important] issues in the newly-added test registration (VirtMemory_Imported_Blit_Without_Owner_Access_Test) — see inline comments.

Also: PR title is now fix(rocr, clr): which is a clear improvement from the original but the comma-scoped form is still flagged as non-conforming; fix(rocr): (primary touched component) would satisfy the checker.

vmmOwningAgent() and hsa_amd_pointer_info reach the same field in ROCr: both read the imported MemoryHandle's resolved region owner, populated by the same EnsureImportInfo() call. Keeping both left two doors to retrieving the same value.

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after latest commits

Latest commit (cfab862) adds to — the [Important] YAML gap is resolved.

On the rebuttal: accepted. The pattern honors the skip verdict via the return value; the broken pattern was calling it without checking. Retracting the [Important] finding on that.

Remaining before merge:

[Required] PR title — comma-scoped form still non-conforming; suggest .
[Nit] Typos and in virtual_memory.cc still present.
dayatsin has two open threads that need an author response: (1) simplification, (2) whether should be made permanently-on.

mangupta · 2026-10-27T16:21:24Z

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (cfab8627e56b) — no new findings at or above the posting threshold.

Query the dmabuf while constructing MemoryHandle from the fd, under the caller's memory lock, and drop the lazy EnsureImportInfo cache. Imported handles then carry their region before map and handle queries. JIRA ID : ROCM-33394 Co-authored-by: Cursor <cursoragent@cursor.com>

Keep the dmabuf constructor query and restore FindDriverMemoryHandle, CopyMemory, FillMemory, VMemoryPtrInfo, and IPCCreate to their previous behavior. JIRA ID : ROCM-33394 Co-authored-by: Cursor <cursoragent@cursor.com>

mangupta · 2026-10-28T01:49:09Z

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (e0a100450e3e) — no new findings at or above the posting threshold.

…GENT

mangupta · 2026-10-28T14:36:11Z

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (ad5181a3376a) — no new findings at or above the posting threshold.

AidanBeltonS requested review from a team, atgutier, cfreeamd, dayatsin-amd, kentrussell and shwetagkhatri as code owners October 26, 2026 17:16

AidanBeltonS requested review from LeoZhao-BJ and saleelk and a balanced review from Copilot October 26, 2026 17:16

github-actions Bot added project: rocr-runtime project: clr labels Oct 26, 2026

Copilot started reviewing on behalf of AidanBeltonS October 26, 2026 17:17

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

Comment thread projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.cc Resolved

cfreeamd reviewed Oct 26, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

Comment thread projects/clr/rocclr/device/rocm/rocmemory.cpp Resolved

Comment thread projects/clr/rocclr/utils/flags.hpp Resolved

systems-assistant Bot added the organization: ROCm label Oct 26, 2026

cfreeamd reviewed Oct 26, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

cfreeamd reviewed Oct 26, 2026

View reviewed changes

Comment thread projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.cc Resolved

mangupta reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

Comment thread projects/rocr-runtime/rocrtst/suites/functional/virtual_memory.cc Resolved

Comment thread projects/clr/rocclr/device/rocm/rocdevice.cpp Resolved

mangupta added the lrt-bot: review label Oct 27, 2026

LeoZhao-BJ reviewed Oct 27, 2026

View reviewed changes

AidanBeltonS changed the title ~~[CLR][ROCr-Runtime] Import owning agent correctly for memcpy-batch async~~ fix(rocr, clr): refresh owning agent for dmabuf-imported VMM handles Oct 27, 2026

dayatsin-amd reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

dayatsin-amd reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/util/flag.h Resolved

AidanBeltonS added 6 commits October 27, 2026 14:26

Return valid owning agent for imported memory where possible

5ae5979

Refresh imported VA after being imported and given physical memory ba…

895a825

…cking

Guard blit fills/copys against imported VA without mapping access

b7740ed

Guard test expectations based on HSA_ENABLE_VMEM_IMPORT_OWNER flag

0efbcba

Add comment clarifying case isn't fallback

08bf366

Make ROC_VMM_REFRESH_OWNING_AGENT properly revert therefresh owning b…

658d387

…ehaviour

cfreeamd reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rocr-runtime/rocrtst/suites/test_common/main.cc Resolved

Comment thread projects/rocr-runtime/rocrtst/suites/test_common/main.cc Resolved

mangupta added lrt-bot: ready and removed lrt-bot: review labels Oct 27, 2026

AidanBeltonS force-pushed the users/abeltons/vmm-import-owning-agent branch from fc84a3d to c03c7b9 Compare October 27, 2026 15:15

Register VirtMemory_Imported_Blit_Without_Owner_Access_Test with CTest

cfab862

cfreeamd reviewed Oct 27, 2026

View reviewed changes

dayatsin-amd reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/runtime.cpp Resolved

dayatsin-amd and others added 2 commits October 27, 2026 18:53

fix(rocr): Drop the other VMM import edits in runtime.cpp

e0a1004

Keep the dmabuf constructor query and restore FindDriverMemoryHandle, CopyMemory, FillMemory, VMemoryPtrInfo, and IPCCreate to their previous behavior. JIRA ID : ROCM-33394 Co-authored-by: Cursor <cursoragent@cursor.com>

AidanBeltonS added 4 commits October 28, 2026 13:42

Remove HSA_ENABLE_VMEM_IMPORT_OWNER flag

31b1e37

Remove unneccessary tests

a8d0c91

Refactor refreshOwningAgent, and remove flag ROC_VMM_REFRESH_OWNING_A…

258a90c

…GENT

Harden implementation with asserts, and proper error handling

ad5181a

dayatsin-amd mentioned this pull request Oct 29, 2026

fix(rocr): Report the owning agent in pointer info for imported VMM mappings #13991

Draft

1 task done

dayatsin-amd approved these changes Oct 30, 2026

View reviewed changes

cfreeamd changed the title ~~fix(rocr, clr): refresh owning agent for dmabuf-imported VMM handles~~ fix(rocr,clr): refresh owning agent for dmabuf-imported VMM handles Oct 30, 2026

cfreeamd approved these changes Oct 30, 2026

View reviewed changes

shwetagkhatri approved these changes Nov 4, 2026

View reviewed changes

Conversation

AidanBeltonS commented Oct 26, 2026

Motivation

Technical Details

Issue Tracking

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mangupta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeoZhao-BJ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mangupta commented Oct 27, 2026

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

mangupta commented Oct 27, 2026

Uh oh!

Uh oh!

mangupta commented Oct 28, 2026

Uh oh!

mangupta commented Oct 28, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

therock-pr-bot Bot commented Oct 26, 2026 •

edited

Loading

LeoZhao-BJ left a comment •

edited

Loading