Skip to content

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

Closed
dayatsin-amd wants to merge 4 commits into
users/ashetaia/fmm-svm-api-relandfrom
users/dayatsin/9394-fix-revoke-extent
Closed

dayatsin-amd wants to merge 4 commits into
users/ashetaia/fmm-svm-api-relandfrom
users/dayatsin/9394-fix-revoke-extent

Conversation

@dayatsin-amd

@dayatsin-amd dayatsin-amd commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Stacked on #9394 (base branch users/ashetaia/fmm-svm-api-reland), so this diff
is one commit on top of that PR.

#9394 tracks SVM-API registrations by page-aligned base. Every buffer sharing a
base therefore shares one tracking entry: one refcount, and one extent grown to
the largest span ever registered at that base and never reduced. Two
consequences:

  1. Over-revoke by grown extent. A hot heap base keeps a large extent for as
    long as any registration at that base is alive. When the last one is dropped,
    the inverse SET_ATTR (NO_ACCESS per GPU plus CLR_FLAGS of COHERENT and
    EXT_COHERENT) is issued over that whole extent instead of over the buffer
    being deregistered. Clearing the coherency flags silently downgrades a range
    from fine- to coarse-grained, so a GPU read of memory that is still live
    elsewhere can return stale data with no fault.
  2. Sweep skips an enclosing range. The interval subtraction that is supposed
    to protect surviving registrations starts at rbtree_lookup_nearest(base, LEFT), which is only the immediate predecessor by address. A longer
    registration further left still covers base but is never visited, so its
    pages get revoked underneath it.

This was seen as output corruption in an inference workload that disappears with
HSA_USE_SVM=0, which disables the SVM-API registration path entirely.

Changes

Not addressed here: register grants ACCESS_IN_PLACE to every GPU in the
process while the map step only grants to the caller's agent whitelist. That
widening needs fmm_register_mem_svm_api to receive the node array and is worth
its own change.

JIRA ID

JIRA ID : ROCM-28520

Test plan

  • libhsakmt compiles clean, no new warnings
  • Traced the kfdtest coverage added by Reland: fix(rocr/thunk): Revoke SVM-API GPU access on deregister, with register re-grant #9394 against the new keying:
    SVMApiDeregisterTest phases 1-3, SVMApiSharedPageTest,
    SVMApiOverlapReproTest and SVMApiRegisterReGrant all still hold,
    including the assertions that motivated the base-keyed design
  • Run the above kfdtests on gfx942 with HSA_XNACK=1
  • Confirm the workload corruption is gone with the SVM-API path enabled
    (HSA_USE_SVM unset)

Made with Cursor

@therock-pr-bot

therock-pr-bot Bot commented Aug 13, 2026 •

Copy link
Copy Markdown

✅ 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

Copy link
Copy Markdown

🚫 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.

@shwetagkhatri
shwetagkhatri self-requested a review August 13, 2026 22:47

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

LGTM

@shwetagkhatri
shwetagkhatri force-pushed the users/dayatsin/9394-fix-revoke-extent branch from 70a955e to 0777e83 Compare August 14, 2026 19:52
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from 0777e83 to 6da92ae Compare August 14, 2026 20:35
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from 6da92ae to 046aae2 Compare August 15, 2026 05:19
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from 046aae2 to c44c984 Compare August 19, 2026 21:37
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from c44c984 to b17bc19 Compare August 19, 2026 21:57
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from b17bc19 to 01aa823 Compare August 19, 2026 22:19
dayatsin-amd and others added 4 commits August 20, 2026 11:42
SVM-API registrations were tracked by page-aligned base, so every buffer
sharing a base shared one tracking entry: a single refcount and a single
extent grown to the largest span ever registered there and never reduced.

A hot heap base therefore keeps a large extent for as long as any
registration at that base is alive. When the last one is dropped, the
inverse SET_ATTR (NO_ACCESS per GPU + CLR_FLAGS COHERENT/EXT_COHERENT) is
issued over that whole extent rather than over the buffer being
deregistered. Clearing the coherency flags silently downgrades a range
from fine- to coarse-grained, so a GPU read of memory that is still live
elsewhere can return stale data with no fault - observed as output
corruption that disappears with HSA_USE_SVM=0.

The interval subtraction meant to protect surviving registrations also
started its sweep at the immediate predecessor of the base, so a longer
registration further left was skipped and its pages revoked underneath it.

Track one entry per registration, keyed by the exact user pointer that
register and deregister both carry, so each registration revokes only its
own extent. The extent can still grow, but only across repeat
registrations of the same pointer, which an allocator cannot produce while
the first registration is live. Start the sweep at the first range that can
still reach the base, bounded by the largest tracked extent. Insert the
tracking entry after the ioctl succeeds, which drops the error-path
rollback and the args leak on that path, since the reject that motivated
reserving it up front is gone.

Signed-off-by: David Yat Sin <David.YatSin@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Deregister sent NO_ACCESS per GPU plus CLR_FLAGS of COHERENT and
EXT_COHERENT. Those flags are page granular and COHERENT is what the
kernel gives a range by default, so a deregister drops whatever else
shares those pages below default. On gfx942/950 EXT_COHERENT selects
the uncached mapping, so a range that is still live elsewhere quietly
goes cached and the GPU can read what the CPU already overwrote.

Registering 16 bytes and dropping them took a whole page from flags
0x83 to 0x1, below the 0x3 it had before any registration, while the
next page kept 0x3. NO_ACCESS alone leaves it at 0x83, and NO_ACCESS is
what the unmap and the eviction skip key off anyway.

Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>
Register rounds out to page boundaries because that is what has to be
mapped. Deregister revoked that same rounded range, so the bytes past
either end lost GPU access too, and those belong to other live
allocations since an allocator packs many objects per page.

It stayed latent while NO_ACCESS only cleared the access bitmaps. Now
that the kernel unmaps on no-access those pages really do go, the
access bits are gone so the fault handler declines to restore them, and
the GPU takes a memory violation on memory we were never handed.

Revoke [PAGE_ALIGN_UP(ptr), PAGE_ALIGN_DOWN(ptr + size)) instead, so a
buffer filling no whole page revokes nothing. Page-aligned tensors are
unaffected: over a 64/16/4MB mix this still revokes 114660 of 114716
pages.

The SVM-API tests expected the old behaviour and are updated. Phase 4
is new, with page-aligned buffers, so the subtraction still has a case
that actually revokes.

Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>
Registering 16 bytes in a page and dropping it must leave that page
alone, since the rest of it belongs to whatever else the allocator put
there. Registering the whole page and dropping it still revokes.

Signed-off-by: Amir Shetaia <Amir.Shetaia@amd.com>
@ashetaia-amd
ashetaia-amd force-pushed the users/dayatsin/9394-fix-revoke-extent branch from 01aa823 to 1c6e518 Compare August 20, 2026 15:43
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