Skip to content

feat(rocr): Restore SVM-API host unregister with debug skip flag - #11411

Closed
dayatsin-amd wants to merge 5 commits into
developfrom
users/dayatsin/skip-svm-host-unregister-debug
Closed

dayatsin-amd wants to merge 5 commits into
developfrom
users/dayatsin/skip-svm-host-unregister-debug

Conversation

@dayatsin-amd

@dayatsin-amd dayatsin-amd commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Restore teardown of host ranges registered through the KFD SVM API, behind a debug env var that is off by default.

Those registrations only set SVM attributes and do not create a vm_object, so hsaKmtDeregisterMemory was a silent no-op. After a revoke, a later register of a reused VA (typical of jemalloc) could still see NO_ACCESS in the kernel. When enabled, this change:

  • tracks SVM-API host ranges (keyed by the user pointer, refcounted)
  • issues NO_ACCESS for only the sub-ranges no surviving registration still covers, on last deregister
  • re-grants ACCESS_IN_PLACE plus the coherency flags on re-register
  • serializes the grant against the revoke (see below)

Enabling it

HSA_SVM_HOST_UNREGISTER_DEBUG set to a non-0 value turns the unregister path on.

Unset - the default - deregistering an SVM-API host range stays the no-op it has always been, and register only sets the two coherency flags, exactly as before this PR. The restored teardown therefore ships dormant and is opted into for testing rather than opted out of. With it off, svm_api_mutex is never taken and no range tracking happens.

Register/revoke serialization

The first commit picked the revoke ranges under svm_api_mutex but issued NO_ACCESS after dropping it, and register granted ACCESS_IN_PLACE before inserting into the tracking tree. Either gap lets a NO_ACCESS reach the kernel after a later grant for the same pages, so a live registration loses GPU access and the copy in flight on it faults with Write access to a read-only page.

This was easy to hit because hipMemcpy2DAsync pins the page-rounded-out host range: two adjacent malloc(65536) buffers get 17-page pins that share one page, and that page was revoked whenever either buffer's refcount reached zero. On 8x gfx950 Unit_hipMemset2DAsync_MultiThread aborted on every run.

The fix tracks the range before the grant ioctl and holds svm_api_mutex across both the grant and the revoke, so the tree and the kernel agree on one order. The reference is given back if the grant fails.

History

This re-lands #6699 (ce90901121a), which was reverted in #9187 after #9202 reported ~20 math-lib jobs on gfx942/gfx950 returning wrong values. Relative to that original commit, this adds interval subtraction so page-sharing neighbors keep their access, keying by the user pointer instead of the page-aligned base, the ACCESS_IN_PLACE re-grant on re-register, the serialization above, and the default-off gate.

JIRA ID

JIRA ID : ROCM-30561

Test plan

Built libhsa-runtime64 against /opt/rocm and loaded it ahead of the installed copy. "off" is the default; "on" is HSA_SVM_HOST_UNREGISTER_DEBUG=1.

Full suites on 8x gfx950 (amdgpu 7.1.9-2402117, ROCm 10.2.0), run in both modes:

  • rocrtst64: 108/108 passed off and on (Memory_Max_Mem excluded)
  • HIP catch (gfx950 ignore list): 4941/4941 passed off and on
  • rocprim / rocthrust / rocrand, 20 binaries: 0 failures off and on
  • rocsparse-test (*bsrmv*:*bsrilu0*:*gebsr2gebsr*): 7395/7395 off and on

Race fix, same 8x gfx950 box, unregister on:

  • Unit_hipMemset2DAsync_MultiThread: 0-1 of 15 passed before the serialization fix, 20/20 after
  • Threaded hipMemset2DAsync + hipMemcpy2DAsync reproducer: 0/10 passed before, 20/20 after
  • Both 12/12 again on the final tree, off and on

Race fix, 1x gfx950 in CPX (8 partitions), which exercises the same 8-way attribute arrays:

  • Unit_hipMemset2DAsync_MultiThread: 9/15 passed before the fix at default xnack and 2/15 with HSA_XNACK=0; 15/15 after the fix in both
  • The reproducer hangs rather than aborts there, with MAPPING_ERROR and a retry-fault storm in dmesg; 20/20 clean after the fix
  • Single-threaded and page-aligned-buffer variants pass either way, confirming the failure was the register/revoke ordering rather than the revoke extent

Suites on 1x gfx950, both modes:

  • rocrtst64: 109/109 passed in both

  • HIP catch: same 2 failures in both, and both also fail with the stock libhsa-runtime64.so.1 (Unit_hipStreamAttachMemAsync_Negative, Unit_hipIpcMemAccess_Semaphores)

  • rocprim / rocthrust / rocrand / rocsparse, 21 binaries incl. 7395 rocsparse-test cases: 0 failures in both

  • AMDKFD_IOC_SVM ioctl sizes confirm the two modes differ: off sends the 2-attr register only, on also sends the per-GPU NO_ACCESS revoke and the per-GPU ACCESS_IN_PLACE re-grant

@therock-pr-bot

therock-pr-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

✅ 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/rocr-runtime/libhsakmt/src/fmm.c; 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?

@dayatsin-amd
dayatsin-amd marked this pull request as draft September 9, 2026 22:52
@dayatsin-amd
dayatsin-amd marked this pull request as ready for review September 10, 2026 17:30
@dayatsin-amd
dayatsin-amd marked this pull request as draft September 10, 2026 17:30

@cfreeamd cfreeamd 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.

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

Files that look correct:

  • Interval arithmetic in svm_api_range_put_locked: Verified. Scan bound base - svm_api_max_extent is correct (any reachable range has key ≥ bound). o->start monotonic non-decreasing in tree-key order so the left-to-right cur sweep is valid. Gap/tail emission and the end ≤ base early-out are correct.
  • Locking (commit 2): mutex held across tree-update + ioctl in both directions. Failed-grant path correctly gives the reference back and drops the revoke set under the same lock.
  • Teardown (hsakmt_fmm_destroy_process_apertures): takes svm_api_mutex, drains tree, frees nodes, resets svm_api_max_extent. No ioctl on teardown. No thread-join under the lock. Safe.
  • fmm_unregister_mem_svm_api error path: ioctl failure correctly treated as non-fatal (VA may be already unmapped; kernel reclaims via MMU notifier).

[Important] Reland provenance not documented. The title says "Restore" — confirming this reinstates previously removed functionality. A re-land inherits the original revert's risk (commonly a perf regression in this codebase). The PR description is empty. Please add: (a) what was removed and why, (b) what this version does differently, and (c) benchmark results comparing with HSA_SKIP_SVM_HOST_UNREGISTER_DEBUG=1 as the skip-path baseline on a workload that exercises the hot path (hipMemcpy2DAsync on gfx950 xnack-off is a natural candidate given the commit message). The debug flag makes A/B measurement straightforward — please post the numbers.

@kentrussell kentrussell 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.

Nothing glaring from me, but definitely not my area of expertise. Will defer to Chris and Philip, and can Approve if they are fine with it

@cfreeamd cfreeamd 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.

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

All prior [Question] and [Optional] findings from my 2026-09-10 review have been addressed:

  • perf-residency-pinning [Question]: Feature is now opt-in off-by-default (HSA_SVM_HOST_UNREGISTER_DEBUG must be explicitly set). No per-operation overhead for existing workloads. Accepted.
  • Inward-rounding [Question]: Design intent confirmed — inward rounding preserves kernel mapping for pages shared with surviving neighbor registrations. The jemalloc VA-reuse scenario is safe. Accepted.
  • Hot-path mutex [Question]: Per-extent exclusion deferred as too complex (would require "revoke in flight" placeholders). Acceptable given opt-in default limits exposure to explicit users. Accepted.
  • Failed-grant rollback [Optional]: Fixed in 9722f5c3 via svm_api_range_unget_locked() — drops the node without the interval-subtraction sweep, so the rollback is now allocation-free. Addressed.

Recommend approve.

@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/skip-svm-host-unregister-debug branch from 9722f5c to c5eb743 Compare September 18, 2026 19:44
Terrydaktal added a commit to Terrydaktal/vllm-coherence that referenced this pull request Sep 18, 2026
…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
Terrydaktal added a commit to Terrydaktal/vllm-coherence that referenced this pull request Sep 18, 2026
…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
@jayhawk-commits
jayhawk-commits force-pushed the users/dayatsin/skip-svm-host-unregister-debug branch from c5eb743 to a2377f5 Compare September 21, 2026 15:49
dayatsin-amd and others added 5 commits September 22, 2026 11:55
Host ranges registered through the KFD SVM API have no vm_object, so
deregister was a silent no-op and a later jemalloc reuse of the same VA
could still see NO_ACCESS. Track those ranges, revoke GPU access on the
last drop, re-grant ACCESS_IN_PLACE on re-register, and allow skipping
that path with HSA_SKIP_SVM_HOST_UNREGISTER_DEBUG.

Co-authored-by: Cursor <cursoragent@cursor.com>
The revoke path picked its ranges under svm_api_mutex but issued
NO_ACCESS after dropping it, and register granted ACCESS_IN_PLACE before
inserting into the tracking tree. Either gap lets a NO_ACCESS reach the
kernel after a later grant for the same pages, so a live registration
loses GPU access and the copy in flight on it faults with "Write access
to a read-only page".

hipMemcpy2DAsync pins the page-rounded-out host range, so two adjacent
malloc'd buffers get pins that share a page and take this path
constantly. Unit_hipMemset2DAsync_MultiThread aborted that way on gfx950
with xnack off.

Track the range before the grant ioctl and hold svm_api_mutex across
both the grant and the revoke so the tree and the kernel agree on one
order. Give the reference back if the grant fails.

Co-authored-by: Cursor <cursoragent@cursor.com>
HSA_SKIP_SVM_HOST_UNREGISTER_DEBUG becomes HSA_SVM_HOST_UNREGISTER_DEBUG
with the opposite sense: the host unregister runs only when the variable
is set to a non-0 value.

Unset, which is now the default, deregistering an SVM-API host range
stays the no-op it has always been and register only sets the coherency
flags, so the restored teardown ships dormant and is opted into for
testing rather than opted out of.

Co-authored-by: Cursor <cursoragent@cursor.com>
When the SET_ATTR that grants access fails, the tracking reference taken
just before it has to go back, but nothing reached the kernel so there
is nothing to revoke. Routing that through svm_api_range_put_locked ran
the interval-subtraction sweep and could allocate a revoke array only to
free it again, on an error path that may itself be an out-of-memory one.

Add svm_api_range_unget_locked(), which returns the reference and drops
the node without computing a revoke set.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ct is released

Paged host memory now allocated without a userptr BO registers itself
through fmm_register_mem_svm_api(), which takes a reference on the
tracking range this series added. That path releases the object instead
of deregistering it, so the reference was never given back: the range
outlived the VA, and a later registration reusing that VA inherited the
stale entry and its refcount.

Drop the reference in __fmm_release() for is_svm_paged objects, which
also issues the matching revoke, so allocate/release is symmetric with
register/deregister.
@dayatsin-amd
dayatsin-amd force-pushed the users/dayatsin/skip-svm-host-unregister-debug branch from a2377f5 to 220d97d Compare September 22, 2026 15:56
@dayatsin-amd

Copy link
Copy Markdown
Contributor Author

Superseded by #12045.

Same commits, rebased onto current develop (the pagedSvm conflict in fmm.c is resolved) plus one new commit that drops the SVM-API tracking reference in __fmm_release(), which develop's new SVM-paged host allocation path otherwise leaks.

Reopened as a plain PR against develop because this one is part of stack #11602 and GitHub will not let its companion change base while the stack exists.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants