Repository navigation
fix(rocr): [ROCM-27751] Make hsa_amd_memory_lock actually pin under the SVM API - #11140
andmar-amd wants to merge 1 commit into
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟡 Changes recommended
The new idle-list unlink path can underflow/corrupt idle accounting when a record has nregs==0 but is not actually on the idle list (e.g., after a failed CLR_FLAGS retry scenario), which can break subsequent pin/unpin behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes repeated pin/unpin workloads by deferring clearing HSA_SVM_FLAG_GPU_ALWAYS_MAPPED on small SVM ranges, keeping ranges GPU-resident across unpins to avoid repeated KFD range split/revalidation and associated page-table/TLB churn. It introduces a bounded “idle” cache of recently-unpinned always-mapped ranges and expands the kfdtest coverage to validate deferral, refcounting, and eviction behavior.
Changes:
- Add an idle list to the AlwaysMapped tracker to defer flag clearing for small (≤2MB) ranges, with caps on record count and total bytes and LRU eviction.
- Update deregistration flow to push eligible ranges into the deferred-clear cache and trim the cache when bounds are exceeded.
- Expand
KFDSVMRangeTest.RegisterMemoryAlwaysMappedto cover deferred behavior, repeated pin/unpin stability, and eviction under the record cap.
File summaries
| File | Description |
|---|---|
| projects/rocr-runtime/libhsakmt/src/fmm.c | Adds deferred-clear idle list and bounded eviction logic for GPU_ALWAYS_MAPPED ranges. |
| projects/rocr-runtime/libhsakmt/tests/kfdtest/src/KFDSVMRangeTest.cpp | Extends tests to validate deferred clearing, refcount semantics, and eviction behavior. |
Review details
Suppressed comments (1)
projects/rocr-runtime/libhsakmt/src/fmm.c:1336
- In the already-covered fast path, was_idle is computed as (r->nregs == 0). After making svm_always_mapped_idle_unlink() a no-op for nregs==0 records that are not actually on the idle list (e.g., kept due to a failed CLR_FLAGS), this will incorrectly re-push such records back onto the idle list on svm_always_mapped_add_reg() allocation failure, potentially deferring/evicting large ranges that should not be deferred.
bool was_idle = r->nregs == 0;
svm_always_mapped_idle_unlink(fmm_ctx, r);
if (svm_always_mapped_add_reg(r, reg))
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
973dcb8 to
cd7e1fb
Compare
cd7e1fb to
f8643d4
Compare
f8643d4 to
df3f111
Compare
andmar-amd
changed the title
d97e6ff to
dec452d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces substantial new low-level memory-management and concurrency logic around kernel SVM ioctls, which warrants final human review despite the added test coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
The SVM-path pinning and its address-keyed refcount tracker look correct — I traced pin-refcount conservation across the record_locked two-pass union-merge, take_locked, and the idle_push/unlink/trim LRU, and the accounting balances across success / error-unwind / double-lock / unlock / eviction / teardown. Lock ordering is clean (always_mapped_mutex always dropped before the aperture fmm_mutex, no nesting), and the atomic always_mapped_nrecords read/mutate ordering matches the documented lock-free read.
I independently verified that the earlier idle-list accounting concern is genuinely resolved: svm_always_mapped_is_idle() now tests idle_prev || idle_next || idle_head == r and idle_unlink() early-returns on !is_idle(), so an unpinned-but-unlisted record no longer decrements idle_count/idle_bytes; the add-reg-failure unwind captures was_idle before unlinking, so no inflation there either.
Two non-blocking notes inline. No new files, so no license-header action needed.
c370ea7 to
fc4fe39
Compare
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up check 2026-09-15
False-positive re-review trigger: the only new activity is the squash/merge commit carrying the PR title itself, not a substantive author response. All original findings were resolved by andmar-amd in c370ea7 and via in-thread explanation. Approve verdict stands.
andmar-amd
changed the title
andmar-amd
changed the title
…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
…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
|
|
||
| /* Remember the range so deregister can clear the flag again */ | ||
| if (nattr == 3 && | ||
| !svm_always_mapped_record_locked(fmm_ctx, aligned_addr, aligned_size)) { |
There was a problem hiding this comment.
There is a bug here - On an allocation failure here the caller is clearing GPU_ALWAYS_MAPPED over a span that a different, still live pin covers. Pinning a 64KB host buffer creates a record with one registration and the flag set. A later pin of a 4KB subrange of that same buffer takes the already-covered path in svm_always_mapped_record_locked, and the realloc inside svm_always_mapped_add_reg fails. The clear then runs over the 4KB the second pin asked for, punching a hole in the flag the first pin still owns. That 4KB is evictable while the first pin is in use.
The allocation failure further down has the same problem and is not limited to a subrange. Pinning [0,64K) and then [32K,96K) skips the already-covered path, and a failure allocating the merged record leaves the clear running over [32K,64K), which the first pin covers.
Please have svm_always_mapped_record_locked report whether the flag over the failing span is still owned by a surviving registration, through a tri-state return or a need_clear out parameter, and set it on both failure exits.
| return err; | ||
| } | ||
|
|
||
| HSAKMT_STATUS hsakmt_fmm_deregister_memory(HsaKFDContext *ctx, void *address) |
There was a problem hiding this comment.
Please review if the SVM-to-legacy fallback may create a registration that has no regs[] entry, but its deregistration still matches by address and may consume a slot belonging to a live pin
For eg: pin address A (tracker entry for page-aligned A). A second lock of a sub-range of the same page hits a realloc failure, as in the previous comment - which returns NO_MEMORY, falls back to fmm_register_user_memory, and succeeds.
The application later unlocks the second buffer. That unlock removes the first pin's regs[] entry, nregs drops to 0, and the flag is cleared while the first pin is live.
Suggested fix: record, per registration, whether the always-mapped tracker was actually updated, and only run the take_locked path for those. The vm_object or a parallel per-registration record is the natural place.
…he SVM API This supersedes #10628, which landed this fix without the deferral below and was reverted in #11139 for the resulting throughput regression (ROCM-30471, ROCM-30504). Pinning and deferring the unpin are one commit here on purpose: pinning alone is exactly the regression that caused the revert, so splitting them would reintroduce it for every bisect that lands between the two. Problem: A pageable hipMemcpy makes ROCr pin the host source with hsa_amd_memory_lock(), whose contract is that the GPU mapping stays valid until the matching unlock. On a kernel exposing the KFD SVM API, libhsakmt implements the registration half of that contract in fmm_register_mem_svm_api() as a single SET_ATTR ioctl carrying two coherency attributes. It never sets HSA_SVM_FLAG_GPU_ALWAYS_MAPPED (0x40, the thunk spelling of the kernel's KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED; "GPU_ALWAYS_MAPPED" below means that one bit), so the "pin" is advisory only. svm_range_evict() keeps a range mapped only when "!p->xnack_enabled || (prange->flags & KFD_IOCTL_SVM_FLAG_GPU_ALWAYS_MAPPED)", and otherwise calls svm_range_unmap_from_gpus(). So with XNACK on, any MMU invalidation overlapping the range -- migration, compaction, khugepaged, reclaim -- unmaps it from the GPU while SDMA may still be reading it. GPU_ALWAYS_MAPPED and !xnack_enabled being the same branch condition is why HSA_XNACK=0 is immune. Normally the resulting retry fault is repaired transparently, but if one restore attempt returns a non-EAGAIN error, amdgpu_vm_handle_fault() poisons the PTE with AMDGPU_VM_NORETRY_FLAGS and the next fault on that page is fatal: VM_L2_PROTECTION_FAULT_STATUS:0x00320031, PERMISSION_FAULTS: 0x3. Exposure scales with page count, not bytes, so disabling THP raises the failure rate roughly 500x on a 1920 MB transfer (~490,000 4 KB pages vs ~960 huge pages). THP is the catalyst, not the cause. libhsakmt already sets GPU_ALWAYS_MAPPED for its own CWSR memory; it was simply never applied to caller pins. Solution: Add an AlwaysMapped bit to HsaMemFlags that a caller sets when it is pinning, and have fmm_register_mem_svm_api() translate it into a third SET_FLAGS(GPU_ALWAYS_MAPPED) attribute. MemoryRegion::Lock() sets the bit; nothing else does, so unrelated registrations are unaffected. The userptr BO path ignores it, being durable already. Gated on KFD interface minor version >= 11, when GPU_ALWAYS_MAPPED became available. GPU_ALWAYS_MAPPED must be dropped again once a range stops being pinned, or it stays permanently resident. The SVM path creates no vm_object, so hsakmt_fmm_deregister_memory() has nothing to look up and cannot recover the size CLR_FLAGS needs; ranges carrying GPU_ALWAYS_MAPPED are tracked in an rbtree instead. Overlapping pins have to share one record, so a registration is merged with every record it overlaps into a union record holding all of their pins. Tracking them separately would let the first deregistration clear GPU_ALWAYS_MAPPED across pages another live pin still covers -- the exact failure this change exists to prevent. Merging over-flags for a while but never under-flags, and keeps records pairwise disjoint, so the lookup a deregistration needs has at most one answer. A record holds the address of each pin it covers rather than a count. hsakmt_fmm_deregister_memory() takes only an address and cannot tell a pin from a plain registration, so a count would be decremented by any plain registration that merely fell inside a pinned union, clearing the flag while the pin was still live. Matching the address instead makes a deregistration a no-op unless it is really releasing a pin. A pin and a plain registration at the identical address stay indistinguishable -- the deregister API carries no flags and the SVM path keeps no per-registration state -- but that is a far smaller hole than a count, which any overlapping plain registration could open. Clearing the flag on the last unpin is what made the first attempt at this regress. Changing a flag on a sub-range makes KFD split the prange out of its neighbours in svm_range_split() and re-run svm_range_validate_and_map(), which rewrites page tables and invalidates the TLB on every GPU. A pin/unpin pair therefore costs two SET_ATTR ioctls and two such rebuilds, and clearing lets the prange merge back so the next pin has to split and revalidate all over again. That is cheap in isolation but not in a loop. An OpenMP reduction hits it hard: the accumulator is a host stack scalar mapped tofrom on every iteration, so libomptarget pins and unpins the same 4 KB page once per iteration for the life of the run. BabelStream's Dot kernel lost 24% against a runtime that does not pin, while Copy, Mul, Add and Triad -- which transfer nothing per iteration and so never pin -- stayed flat. So a record whose refcount reaches zero is not cleared immediately. It moves to an idle list threaded through the existing rbtree records instead of being cleared and freed. Re-pinning a range still on that list takes it back off, and because the flag was never removed the caller's SET_ATTR is an exact-range, same-attributes no-op that KFD satisfies without touching page tables. Re-pinning still always issues the SET_ATTR. Skipping it when the range looks flagged would be wrong: an munmap of a deferred range followed by an mmap that reuses the address leaves a stale record behind, and the new pin would silently be advisory again -- back to the bug at the top of this message. Issuing it is necessary but not sufficient, because a stale record is also a geometry claim. The merge above unions a new pin with every record it overlaps, and it tests only addresses, so a record whose mapping is gone donates its whole span while contributing no pin. Enough munmap-then-reuse traffic walks that union across VA the application has already freed. KFD validates a span end to end -- svm_range_is_valid() returns EFAULT at the first hole, and svm_range_set_attr() checks it before touching anything -- so the eventual CLR_FLAGS fails outright and clears nothing, not even the pages still mapped. Keeping the record to retry, as the deregistration path does for any failed clear, then makes it permanent: it re-merges with each later pin and fails again the same way. So a record with no live pin is checked before it is merged, and dropped rather than absorbed once the mapping under it is gone. mincore() is the test, failing ENOMEM exactly when a range contains unmapped pages, and nothing needs clearing on the way out: munmap() already tore the KFD range down through the MMU notifier and GPU_ALWAYS_MAPPED went with it. The repeat-pin case the idle list exists for is deliberately left alone -- a re-pin of exactly the record's span re-establishes it whatever happened in between, since the caller's SET_ATTR has just flagged that exact span, so it costs no syscall. A failed CLR_FLAGS asks the same question afterwards and reports success when the range has gone, so the record is dropped instead of retried forever. Asking only after the ioctl has failed keeps the check off every successful clear. Deferral is bounded, since a range that is no longer pinned stays GPU-resident until something evicts it. Only ranges of 2 MB or less are deferred -- the two ioctls are a fixed cost, so they dominate only when the range is small, and a large transfer both amortizes them and is exactly the case where holding residency is expensive. The idle list is capped at 256 records and 32 MB; exceeding either evicts oldest-first with a real CLR_FLAGS. Teardown clears whatever is left. Of the two caps only the byte cap really bounds residency, since it does so whatever mix of sizes produces it. The record cap can only bind first for ranges well under the size limit -- exactly the ones deferring helps most -- and it is a cliff rather than a slope there: a workload cycling over more distinct ranges than it holds evicts each record before it can be reused and gets no benefit at all. So it is set high enough to leave the byte cap doing the work. A record is 96 bytes plus a small pin list, so a full list is a few tens of KB. At 64 records, 128 distinct 4 KB pins measured 0% improvement; at 256 the same case improves 96%. What that residency costs is worth stating exactly, since it is the trade this change makes: up to 32 MB per process of the caller's own live memory stays GPU-mapped, and so non-migratable, after its last unpin. It cannot outlive that memory. GPU_ALWAYS_MAPPED guards eviction, not unmap -- svm_range_cpu_invalidate_pagetables() sends MMU_NOTIFY_UNMAP to svm_range_unmap_from_cpu(), which unmaps from the GPUs without consulting the flag and queues SVM_OP_UNMAP_RANGE once the unmap covers the range, while only the other arm of that switch, svm_range_evict(), tests it. So an munmap or an exit releases the memory whatever our record says, and a record outliving its mapping costs bookkeeping -- of the kind handled above -- rather than residency. The bound is per process, since there is one fmm context per KFD context and one primary context per process, so it does not scale with GPUs, agents or threads. Two allocations are kept off the pin path, which is now short enough for them to show. The SET_ATTR callers whose attribute count is bounded at compile time build the ioctl argument in a stack union rather than mallocing it per call -- kfd_ioctl_svm_args ends in a flexible array member, so it cannot be embedded before one, and a union sizes and aligns the storage without that. And the record count is maintained atomically alongside the tree, so a deregistration in a process that has never pinned anything returns without taking a process-wide lock. The count leaves zero only under that mutex, so a stale zero is observable only while the very first pin is still in flight, and a deregistration racing the registration it pairs with is already caller error. Also zeroes HsaMemFlags before use in hsaKmtRegisterMemoryCtx() and hsaKmtRegisterMemoryToNodesCtx(). Pre-existing bug: both left the bitfield uninitialized and set only two members, so every other flag took a stack-garbage value. Latent because the SVM path read only CoarseGrain and ExtendedCoherent; a third consumer makes it live. Correctness reproducer is a pageable H2D hipMemcpy loop with THP disabled, on MI210 / gfx90a with HSA_XNACK=1. - New KFDSVMRangeTest.RegisterMemoryAlwaysMapped, run for both values of the fixture's XNACK parameter. It registers anonymous host memory through hsaKmtRegisterMemoryWithFlags() and reads the flags back with hsaKmtSVMGetAttr(), which reports SET_FLAGS as the AND over the interval and CLR_FLAGS as the complement of the OR, so "set on every page" and "set on no page" are both observable. It checks that a non-pinning registration is left alone, that a pin sets GPU_ALWAYS_MAPPED and deregistration clears it, that two pins need two deregistrations, that releasing an inner pin does not clear GPU_ALWAYS_MAPPED under a live overlapping outer pin, and that deregistering a plain registration inside a pinned range does not release the pin. It fails against the unfixed libhsakmt, and against a version of this change without the union merge it fails only the two overlap assertions. Further sections cover the deferral: that a small range keeps the flag after its last unpin, and that eviction is bounded under both the record cap and the byte cap. Those need separate sections, because ranges small enough to make the record cap interesting never accumulate the bytes to reach the other bound; a section of the largest ranges that still defer is what exercises it, and, since the size limit is inclusive, what pins that boundary too. A last section pins at an address reused after munmap. The stale record makes that look like a repeat pin, but the mapping underneath is new and unflagged, which is what makes issuing the SET_ATTR every time load-bearing rather than redundant. Each new assertion was verified to fail under a deliberate mutation of the code it covers. Full suite 186/186 on MI210, both XNACK values, and 189/189 on MI300A (gfx942), both XNACK values, four nodes each. The two totals are not comparable: run_kfdtest.sh skips a different set per part. - The stale-record hazard is not reachable from kfdtest, which never does munmap-then-reuse-with-overlap, so it is measured with the pin/unpin matrix instead, reading the ioctl error column that first exposed it. Twelve runs per arm on MI300A: 9 of 12 failed before the check with 10,064-11,428 EFAULT each, always over one 9,433,088-byte union; 0 of 12 after, with the call count settling on a single deterministic 209,112 rather than varying with the failures. Note the failing runs are the faster ones in wall clock -- a clear that fails does no work -- so any timing taken from a run that was silently failing reads high. Deferral throughput is unaffected: against the pin-without-deferral arm the same matrix measures 41x on a repeated 4 KB pin, 44x at 64 KB, 116x at 1 MB, and 32-42x cycling over 32 and 128 distinct ranges, while the 8 MB case -- over the threshold, so cleared immediately -- is 1.0x, within run-to-run noise. - 16 paired rounds, 2 x 512 MB, arms interleaved, under deliberate compaction pressure, counting kfd_svm dynamic_debug branches: arm rounds mid-transfer unmaps retry faults safe evicts stock 16 4143 (15 rounds) 730 (6) 0 patched 16 413 (5 rounds) 0 (0) 16 Retry faults -- the step that can poison a PTE -- are zero in every patched round. The residual patched unmaps are invalidations landing outside a lock window or on the runtime's own SVM ranges; the kernel tests GPU_ALWAYS_MAPPED per prange, so those correctly still unmap. - Confirmed on customer hardware, which reproduces the crash at ~50% per run. The development box above does not reproduce it despite being the same part and effectively the same driver -- MI210 (Aldebaran, gfx90a, 1002:740f) on RHEL 8.10, in-tree amdgpu from kernel 4.18.0-553.el8_10 against the customer's 4.18.0-553.129.1.el8_10, whose SVM fault path is byte-identical across that z-stream gap. The mechanism is armed here -- the mid-transfer unmaps and retry faults in the table above are real -- but the box is idle enough that every restore succeeds on retry, so no PTE is ever poisoned. On the customer's system, interleaved 20-round A/B: 10 of 20 stock runs failed, 0 of 20 fixed runs failed. A 200-iteration 1920 MB soak (~375 GB pageable H2D, 12,000 pinned registrations) completed clean. - Regression side: on the BabelStream Dot case this takes a run from 35 SET / 35 CLR to 35 SET / 3 CLR -- the three being the 256 MB arrays, which are over the threshold and so still cleared immediately -- and brings throughput back within run-to-run noise of a runtime that does not pin. Measured against the pin-without-deferral arm that was reverted, Dot goes 939,981 -> 1,181,780 MB/s (25.7% faster) and the run issues 30% fewer ioctls. In the pin/unpin microbenchmark a repeated 4 KB pin costs 96% less, and so does a cycle over 128 distinct 4 KB ranges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fc4fe39 to
b059338
Compare
|
PR #12045 already addresses this issue. The ROCM tickets listed here have been validated against the latest develop tree. #12045 is merged ( Checked on b06-2 against ROCm 10.2.0a20260927 (rocm-systems
|
Motivation
#11139 reverted the original ROCM-27751 fix (#10628) because of the throughput regression it caused, and this PR relands that fix as a single commit with the regression designed out: GPU_ALWAYS_MAPPED is no longer cleared on every unpin, so a repeated pin costs no page-table work.
This supersedes the deferral-only change previously proposed here. The pin and the deferral are one commit deliberately: pinning without the deferral is exactly the regression that caused the revert, so landing them separately would reintroduce it for anyone who bisects or ships from between the two.
Technical Details
Pinning.
hsa_amd_memory_lock()'s contract is that the GPU mapping stays valid until the matching unlock, but on the KFD SVM pathfmm_register_mem_svm_api()only ever sent two coherency attributes, neverGPU_ALWAYS_MAPPED— so the pin was advisory. With XNACK on,svm_range_evict()unmaps such a range on any overlapping MMU invalidation (migration, compaction, khugepaged, reclaim) while SDMA may still be reading it. A newAlwaysMappedbit inHsaMemFlagsis translated into a thirdSET_FLAGSattribute.MemoryRegion::Lock()is the only setter, so unrelated registrations are unaffected; the userptr BO path ignores it, being durable already. Gated on KFD interface minor version >= 11.Tracking. The flag has to come off once a range stops being pinned. The SVM path creates no vm_object, so
hsakmt_fmm_deregister_memory()— which receives only an address — cannot recover the sizeCLR_FLAGSneeds; flagged ranges are tracked in an rbtree. Overlapping pins are merged into one union record, because tracking them separately would let the first deregistration clear the flag across pages another live pin still covers. A record holds the address of each pin rather than a count, so a plain registration that merely falls inside a pinned union cannot decrement it.Deferring the clear. Clearing on the last unpin is what made the first attempt regress: changing a flag on a sub-range makes KFD split the prange and re-run
svm_range_validate_and_map(), rewriting page tables and invalidating the TLB on every GPU, and clearing lets the prange merge back so the next pin splits and revalidates again. So a record whose refcount reaches zero moves to an idle list threaded through the existing rbtree records instead of being cleared and freed. Re-pinning takes it back off, and because the flag was never removed the caller'sSET_ATTRis an exact-range, same-attributes no-op.SET_ATTRis still always issued. Skipping it when the range looks flagged would be wrong: anmunmapof a deferred range followed by anmmapreusing the address leaves a stale record behind, and the new pin would silently be advisory again.Bounds. Only ranges of 2 MB or less are deferred; the idle list is capped at 256 records and 32 MB, exceeding either evicts oldest-first with a real
CLR_FLAGS, and teardown clears the rest. Of the two caps only the byte cap really bounds residency, since it does so whatever mix of sizes produces it. The record cap can only bind first for ranges well under the size limit — exactly the ones deferring helps most — and it is a cliff rather than a slope there: a workload cycling over more distinct ranges than the list holds evicts each record before reuse and gets no benefit at all. So it is set high enough to leave the byte cap doing the work; a record is 96 bytes plus a small pin list, so a full list is a few tens of KB. At 64 records, 128 distinct 4 KB pins measured 0% improvement; at 256 the same case improves 96%.What the residency costs. This is the trade the change makes, so it is worth stating exactly: up to 32 MB per process of the caller's own live memory stays GPU-mapped, and so non-migratable, after its last unpin. It cannot outlive that memory.
GPU_ALWAYS_MAPPEDguards eviction, not unmap —svm_range_cpu_invalidate_pagetables()sendsMMU_NOTIFY_UNMAPtosvm_range_unmap_from_cpu(), which unmaps from the GPUs without consulting the flag and queuesSVM_OP_UNMAP_RANGEonce the unmap covers the range, while only the other arm of that switch,svm_range_evict(), tests it. So anmunmapor an exit releases the memory whatever our record says, and a record outliving its mapping costs 96 bytes of bookkeeping rather than residency. The bound is per process — one fmm context per KFD context, one primary context per process — so it does not scale with GPUs, agents or threads.Uninitialized flags. Also zeroes
HsaMemFlagsbefore use inhsaKmtRegisterMemoryCtx()andhsaKmtRegisterMemoryToNodesCtx(). Pre-existing bug: both left the bitfield uninitialized and set only two members, so every other flag took a stack-garbage value. Latent because the SVM path read onlyCoarseGrainandExtendedCoherent; a third consumer makes it live.Allocation on the pin path. With a pin/unpin pair down to a single ioctl, two allocations were a visible fraction of what remained and are now gone. The
SET_ATTRcallers whose attribute count is bounded at compile time build the ioctl argument in a stack union rather thanmallocing per call (kfd_ioctl_svm_argsends in a flexible array member, so it cannot be embedded before one; a union sizes and aligns the storage without that). And the record count is maintained atomically alongside the tree, so a deregistration in a process that has never pinned anything returns without taking a process-wide lock — the count leaves zero only under that mutex, so a stale zero is observable only while the very first pin is still in flight, and a deregistration racing the registration it pairs with is already caller error.Testing.
KFDSVMRangeTest.RegisterMemoryAlwaysMappedruns for both XNACK values and covers the pin/unpin semantics, the refcount, overlapping pins, eviction under both the record cap and the byte cap, and a pin at an address reused aftermunmap. Each new assertion was verified to fail under a deliberate mutation of the code it covers. Full suite 186/186 on MI210, both XNACK values.Perf, measured against the reverted build: repeated 4 KB pin 28747 → 1108 ns (−96%), 1 MB 188418 → 1124 ns (−99%), 128 distinct 4 KB 29086 → 1235 ns (−96%), ioctls −30%, BabelStream Dot 939,981 → 1,181,780 MB/s (+25.7%). The control behaves as designed: 8 MB, above the defer threshold, −0.4%.
Issue Tracking
JIRA ID : ROCM-27751
JIRA ID : ROCM-30504
JIRA ID : ROCM-30505
JIRA ID : ROCM-30471
JIRA ID : ROCM-30561
Submission Checklist