Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
75e0a2f to
a60cc5c
Compare
|
🎉 All checks passed! This PR is ready for review. |
a60cc5c to
9523a12
Compare
amd-arosa
changed the title
9523a12 to
283f7b0
Compare
MAP_MEMORY_TO_GPU only entered an allocation into the process page table when it already had a host pointer. A caller-placed allocation never has one: ROCr reserves the VA itself with a PROT_NONE mmap and asks the driver to back it, and alloc_memory_ioctl deliberately creates neither a memfd nor a host mapping for a caller-supplied va_addr in local mode. No mmap of the KFD fd follows for device memory, so MAP was the last chance to back the range and it did nothing: the allocation stayed absent from the page table and accesses only "worked" by falling through to passthrough, bypassing GPU translation entirely. Adopt the caller's reservation in map_memory_ioctl the way dispatch_mmap already adopts MAP_FIXED pages: mprotect the range READ|WRITE through libc_passthrough and take it as host_ptr, left caller-owned so teardown never unmaps pages the driver did not create. USERPTR already carries the caller's own pages and DOORBELL must keep reaching the doorbell path, so both are excluded, and daemon mode keeps its memfd path. The cost, noted at the call site, is that the caller's PROT_NONE reservation becomes host-readable for the life of the allocation, so a stray CPU dereference of a device pointer no longer faults the way it would on hardware -- the same trade dispatch_mmap already makes. When the mprotect fails the range is logged and left unbacked, so a client that passed a VA it never reserved keeps its pre-fix passthrough behaviour instead of meeting a newly failing ioctl. errno is captured before the logger runs, and the line goes to the cp group alongside the other MAP_MEMORY records rather than hiding under vm. Backing these allocations makes a latent teardown asymmetry live: free_memory_ioctl skipped its unmap_from_gpu for any caller-placed allocation, which was inert while such an allocation never had a host pointer, but now strands page-table entries whenever a client FREEs without UNMAPping first. unmap_from_gpu only drops PTEs and never munmaps, so the guard comes off. tests/kfd_ioctl_test.cpp gains three regressions: a PROT_NONE reservation placed by the caller is mapped page-by-page onto the reserved pages after MAP and is writable afterwards (this failed before the fix, with the first page absent from the page table), FREE without a preceding UNMAP leaves no entry behind, and a GPU-only VA that is not mapped in the process still reports success and stays unbacked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
283f7b0 to
b308623
Compare
|
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:
This PR will be automatically closed in 5 days if no further activity occurs. |
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Minor issue in a test found by the agent, so approving with comment:
[MINOR] emulation/rocjitsu/tests/kfd_ioctl_test.cpp:2180 — Hard-coded GPU-only VA breaks the TSan
suite
Reasoning: The test assumes 0x0000210000000000 is absent from every process address space. TSan maps
that address, so the precondition at line 2186 fails before the KFD behavior is exercised.
Evidence: In PR CI, mincore(kGpuOnlyVa, ...) returned 0 rather than -1/ENOMEM. This was the sole
failing test in the TSan suite: KfdIoctlTest.MapMemoryToleratesCallerPlacedVaThatIsNotAHostVa.
Suggested fix: Obtain a process-specific hole dynamically—for example, create an anonymous mapping,
capture its address, unmap it, verify mincore returns ENOMEM, and immediately use that address for
the allocation—instead of relying on a fixed virtual address.
Confidence: high; confirmed directly from the TSan job log.
|
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. |
Motivation
SimulatedKfd::map_memory_ioctlnever backs a caller-placed allocation.In local (non-daemon) mode
alloc_memory_ioctlcreates neither a memfd nor ahost mapping when the caller supplies
va_addrand the allocation is notUSERPTR, soalloc.host_ptrstays null.map_memory_ioctlthen gates itsbacking on
if (alloc.host_ptr && !alloc.user_va)and skips it, and MAP is thelast chance to back the allocation — no
mmapof the KFD fd follows for devicememory.
This is the shape ROCr actually uses: it reserves the VA itself with a
PROT_NONEmmap and asks the driver to back it. Today that only appears towork because the access falls through to passthrough.
Technical Details
At MAP time, adopt the caller's reservation the way
dispatch_mmapalreadyadopts
MAP_FIXEDpages: lift thePROT_NONEwithmprotectand record thepages as caller-owned (
host_ptr_owned = false) so teardown never unmapsmemory the caller owns.
Guarded to the case it is meant for — local mode only, only when there is a
user_vaand nohost_ptr, and never forUSERPTR(which already carries thecaller's own pages) or
DOORBELL(which must keep reaching the doorbell path).If the
mprotectfails, the GPU VA was never a host VA the caller reserved.That is left unbacked and the ioctl still reports success, because such a
client previously fell through to passthrough and a failing ioctl would be a
regression for it. The case is logged through
util::Logger::vm.Changed files:
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cppemulation/rocjitsu/tests/kfd_ioctl_test.cppScope note
An earlier revision of this PR also tagged tensor-DMA byte copies with the
issuing process id. That half is now already fixed on
develop—Wavefront::write_gpu_memory()passesprocess_id_through tomemory->write_block(), andcopy_dense_tensorroutes through it (#9775).This PR has been narrowed to the KFD half, which is still present.
Issue Tracking
GitHub issue: #10331
Test Plan
Written test-first: the regression tests were added and observed to fail
against
developbefore any source change, then the fix was applied and thetests re-run.
Test Result
Full suite on current
develop(60c29ef): 100% tests passed, 0 failed outof 3218.