Reland: fix(rocr/thunk): Revoke SVM-API GPU access on deregister, with register re-grant by ashetaia-amd · Pull Request #9394 · ROCm/rocm-systems

ashetaia-amd · 2026-07-28T20:36:06Z

Motivation

Relands #6699 (reverted in 51c421c / #9187) with the fix for what broke it.

#6699 made deregister revoke GPU access with NO_ACCESS, but register never re-granted it. jemalloc reuses a VA without munmap, so that NO_ACCESS stays on the kernel SVM range and the GPU faults on the next register of the same address. Under ASan + XNACK the hip-tests hung on that fault and CI killed them on timeout, which is what turned ASan red.

Technical Details

Revoke SVM-API GPU access on deregister (original fix(rocr/thunk): Revoke SVM-API GPU access attributes on deregister #6699, reapplied past the revert).
Accept same-base registrations with differing spans (follow-on lost in the revert).
kfdtest coverage for the same-page divergent-span case.
Re-grant GPU access on SVM-API register (new). Register now sets ACCESS_IN_PLACE per GPU so a reused VA regains access. Has to be ACCESS_IN_PLACE, not ACCESS: userptr memory can't migrate, so ACCESS makes the fault handler try a migration that fails and still faults. Matches what fmm_map_mem_svm_api() already uses.
kfdtest for the register re-grant path.

Complementary to Philip Yang's kernel patch (unmap svm range when all GPUs set to no-access); kernel unmaps on no-access, thunk revokes on deregister and re-grants on register. Both are needed.

Issue Tracking

JIRA ID: ROCM-28520

Test Plan

Built the full-ASan gfx942 stack on top of Philip's kernel patch and ran the failing hip-tests (MemoryTest1/2, GraphsTest2) with HSA_XNACK=1, comparing baseline, #6699, #6699 + this fix, and the ACCESS-instead-of-ACCESS_IN_PLACE variant.

Test Result

fix(rocr/thunk): Revoke SVM-API GPU access attributes on deregister #6699 without this fix: MemoryTest1 hangs mid-suite on VM_L2_PROTECTION_FAULT.
fix(rocr/thunk): Revoke SVM-API GPU access attributes on deregister #6699 with this fix: completes all 376 cases, same as baseline (261 pass, 10 pre-existing fails).
ACCESS variant: still hangs, so ACCESS_IN_PLACE is required.

Submission Checklist

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

therock-pr-bot · 2026-07-28T20:36:43Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 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?

therock-pr-bot · 2026-07-28T20:36:45Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

cfreeamd

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

Reviewed the rocr-runtime changes: libhsakmt/src/fmm.c (SVM-API range tracking + register re-grant / deregister revoke) and the new kfdtest coverage (KFDSVMRangeReGrant_test.cpp + the refcount / overlap / shared-page phases in KFDSVMRangeTest.cpp). The register/deregister symmetry, refcounting, and interval-subtraction revoke logic look correct and are well covered by the new tests. Left non-blocking notes inline on fmm.c (a comment that no longer matches the implementation, and an error-path extent-rollback edge case) plus an SPDX header nit on the new test file. All changes are in rocr-runtime scope.

cfreeamd

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

Both prior [Optional] items are resolved:

Over-revoke on register ioctl failure: commit 70415729 now calls svm_api_range_get() only after the SET_ATTR ioctl succeeds, so a failed register no longer inserts/grows a tracked extent that never got GPU access. Matches the suggested fix.
Stale reject-design comment / dead-code error return: the register path was rewritten; the misleading INVALID_PARAMETER comment is gone and the tracking-result now flows through the shared out: free path (no leak).

Only the [Nit] remains: KFDSVMRangeReGrant_test.cpp still uses the legacy full MIT license block rather than the short SPDX-License-Identifier: MIT header — non-blocking.

cfreeamd

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

Prior [Optional] findings resolved:

Finding #2 (register ioctl failure → over-revoke): Fixed in commit 7041572 — svm_api_range_get() is now called after the SET_ATTR ioctl succeeds, so a failed register never grows a tracked extent or inserts a tracking entry.
Finding #1 (stale reject-design comment + dead-code/leaky error return): Resolved — the register path was rewritten; stale comment and dead if (ret != SUCCESS) return ret; are gone, freed cleanly via goto out on all paths.

Finding #3 (SPDX header on KFDSVMRangeReGrant_test.cpp): Still the legacy full MIT block, not the short SPDX form. Non-blocking [Nit]; does not gate approval.

Gating approvals present: dayatsin-amd (2026-07-28), kentrussell (2026-07-29). All substantive findings resolved — ready for @cfreeamd approval.

Host ranges registered via the KFD SVM API (fmm_register_mem_svm_api) only set VA-range attributes and never create a vm_object, so deregister was a silent no-op leaving stale ACCESS_IN_PLACE and COHERENT attributes on the kernel-side SVM range after unpin. Track SVM-API ranges in an rb-tree (aligned base/size, refcounted). On the last deregistration of a base, compute the sub-ranges not covered by any other surviving registration and issue the inverse SET_ATTR: NO_ACCESS for every GPU plus clearing COHERENT/EXT_COHERENT flags. Same-base re-registration with a different size is rejected (INVALID_PARAMETER) so the extent to revoke is unambiguous. Includes kfdtest coverage for refcount, mismatched re-register rejection, and overlapping page-rounded registrations. Co-authored-by: Amir Shetaia <Amir.Shetaia@amd.com> Co-authored-by: David Yat Sin <David.YatSin@amd.com>

Distinct sub-page host buffers can share a page-aligned base but round to different page-aligned spans (small calloc'd buffers packed into one page, one straddling into the next). The same-base/different-size reject wrongly failed the second registration, regressing Unit_hipHostRegister_Nbuf_MultiFlag_RegisterUnregister. Deregister is keyed by the user pointer, so these are distinguishable: refcount them together on the base and grow the tracked extent to the largest span, so deregister revokes the full extent once the last owner is dropped. Overlap with other bases is still resolved by the interval subtraction in svm_api_range_put. Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>

Add KFDSVMRangeTest.SVMApiSharedPageTest guarding the calloc-packed pattern that regressed: two sub-page buffers sharing a page base with different page-aligned spans must both register and be revoked correctly. Update SVMApiDeregisterTest Phase 1, which previously asserted the now-removed reject, to assert the accepted + refcounted + grow behavior. Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>

Registration only set the coherency flags and relied on the SVM range's default access. Deregister sets NO_ACCESS per GPU, and allocators such as jemalloc recycle the VA without munmap, so that NO_ACCESS persists on the kernel SVM range past the original registration. A later register at the reused VA never cleared it, so the GPU faulted (VM_L2_PROTECTION_FAULT) on the re-registered buffer once enough register/deregister churn built up - this is what caused the hip-tests hangs that reverted #6699 under ASan/XNACK. Re-grant ACCESS_IN_PLACE on every GPU at register time so a reused VA regains access. ACCESS_IN_PLACE (not ACCESS) is required: this is registered host (userptr) memory that cannot migrate, and ACCESS makes the retry-fault handler attempt a migration that fails and still faults. Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>

Add KFDSVMRangeReGrant_test.cpp: register a system-memory range, deregister it (leaves NO_ACCESS), then register the same VA again and check the GPU access is restored instead of staying NO_ACCESS. Covers the register-path re-grant in fmm_register_mem_svm_api that the earlier revert was missing. Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>

svm_api_range_get() reserved the tracking entry before the SET_ATTR ioctl. That ordering was there to reject a same-base re-registration with a mismatched size before touching kernel state, but that reject is gone: same-base registrations with differing spans are now refcounted together and the tracked extent grows to the largest span. With the reject gone the early reservation only causes problems. If a second, larger same-base registration fails the ioctl, the error path drops the refcount but not the grown extent, so the surviving first registration ends up tracking pages that never got GPU access and revokes them on deregister, over-revoking a neighbour that owns them. The early return on a get() failure also leaked args. Register after the ioctl instead. A failed register leaves nothing tracked, the extent can only grow for a span the kernel accepted, and the rollback path goes away. Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>

github-actions · 2026-09-15T00:23:46Z

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

Add new commits
Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

…nership fixes Import the pinned ROCr/HSA registration and memory-region corrections with the relevant libhsakmt definitions. Preserve source identities and original component licensing for reproducible runtime builds. Provenance Upstream PRs and authors: - ROCm/rocm-systems#9394 — ashetaia-amd - ROCm/rocm-systems#11140 — andmar-amd - ROCm/rocm-systems#11411 — dayatsin-amd

github-actions · 2026-09-20T00:25:30Z

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

ashetaia-amd requested review from cfreeamd, dayatsin-amd, kentrussell and xiaogang-chen-amd as code owners July 28, 2026 20:36

github-actions Bot added the project: rocr-runtime label Jul 28, 2026

therock-pr-bot Bot added the Not ready to Review label Jul 28, 2026

ashetaia-amd force-pushed the users/ashetaia/fmm-svm-api-reland branch from 8654a89 to 5abec2b Compare July 28, 2026 21:23

systems-assistant Bot added the organization: ROCm label Jul 28, 2026

cfreeamd reviewed Jul 28, 2026

View reviewed changes

Comment thread projects/rocr-runtime/libhsakmt/src/fmm.c Resolved

Comment thread projects/rocr-runtime/libhsakmt/src/fmm.c Resolved

Comment thread projects/rocr-runtime/libhsakmt/tests/kfdtest/src/KFDSVMRangeReGrant_test.cpp Resolved

ashetaia-amd removed the Not ready to Review label Jul 28, 2026

dayatsin-amd approved these changes Jul 28, 2026

View reviewed changes

kentrussell approved these changes Jul 29, 2026

View reviewed changes

dayatsin-amd mentioned this pull request Aug 7, 2026

fix(rocr/thunk): revoke only the deregistered SVM-API extent #9861

Closed

2 of 4 tasks

shwetagkhatri force-pushed the users/ashetaia/fmm-svm-api-reland branch from 5abec2b to 38774a5 Compare August 14, 2026 19:52

ROCm deleted a comment from NightBaRron1412 Aug 14, 2026

ashetaia-amd force-pushed the users/ashetaia/fmm-svm-api-reland branch from 8a004e7 to bb86967 Compare August 14, 2026 20:21

ROCm deleted a comment from NightBaRron1412 Aug 14, 2026

ashetaia-amd force-pushed the users/ashetaia/fmm-svm-api-reland branch from bb86967 to 7041572 Compare August 15, 2026 05:19

cfreeamd reviewed Aug 15, 2026

View reviewed changes

cfreeamd reviewed Aug 17, 2026

View reviewed changes

ashetaia-amd force-pushed the users/ashetaia/fmm-svm-api-reland branch 3 times, most recently from ca2a22f to 900bbc0 Compare August 19, 2026 22:19

saleelk and others added 4 commits August 20, 2026 11:42

ashetaia-amd added 2 commits August 20, 2026 11:42

ashetaia-amd force-pushed the users/ashetaia/fmm-svm-api-reland branch from 900bbc0 to 9cf2ed3 Compare August 20, 2026 15:43

github-actions Bot added the Stale PR has no activity for 25+ days label Sep 15, 2026

github-actions Bot closed this Sep 20, 2026

Navigation Menu

Repository navigation

Reland: fix(rocr/thunk): Revoke SVM-API GPU access on deregister, with register re-grant - #9394

Reland: fix(rocr/thunk): Revoke SVM-API GPU access on deregister, with register re-grant#9394
ashetaia-amd wants to merge 6 commits into
developfrom
users/ashetaia/fmm-svm-api-reland

ashetaia-amd commented Jul 28, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Jul 28, 2026

Uh oh!

cfreeamd left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Uh oh!

cfreeamd left a comment

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

github-actions Bot commented Sep 20, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ashetaia-amd commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

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

Uh oh!

✅ All Policy Checks Passed

Uh oh!

therock-pr-bot Bot commented Jul 28, 2026

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!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

github-actions Bot commented Sep 20, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ashetaia-amd commented Jul 28, 2026 •

edited

Loading

therock-pr-bot Bot commented Jul 28, 2026 •

edited

Loading