Repository navigation
UCP/UCT/CUDA: Avoid gdr_copy for non gdr capable memory - #12039
Merged
Merged
Conversation
gdr_copy pinning fails with EINVAL for memory localized to a GPU locality domain and for stream-ordered allocations, because nvidia_p2p_get_pages refuses memory the driver did not place as GDR-capable. Registrability cannot discriminate them, as an exportable memory pool is registrable. Add an orthogonal UCS_MEM_FLAG_PEER_MEM_PINNABLE flag, cleared by cuda_copy for both cases and required by gdr_copy. Select the mem type endpoint RMA lane by the buffer memory flags, instead of always taking the first lane. The datatype iterator already carries the flags, so the proto v2 path adds no memory type query. The remaining callers pass NULL, and the flags are resolved only if a memory domain requires them.
|
🤖 Starting review — findings will be posted here when done. |
tvegas1
commented
Oct 7, 2026
Drop the registrability assertion from the localized memory test, as exporting a dma_buf fd for such memory is platform dependent, and remove the github reference from its comment. Fix two comments in test_ucp_mem_type which still described selecting the lane by a memtype cache lookup and a quiet registration attempt, instead of resolving the memory flags once on the lane which requires them. Update the copyright year of the modified files.
tvegas1
force-pushed
the
gdr_copy_peer_pin
branch
from
October 7, 2026 14:05
7b23b60 to
ed89e64
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gaps worth noting: the |
tomerg-nvidia
left a comment
tomerg-nvidia
left a comment
Contributor
There was a problem hiding this comment.
Also address the bot comments
|
🤖 Starting review — findings will be posted here when done. |
tomerg-nvidia
previously approved these changes
Oct 8, 2026
tomerg-nvidia
previously approved these changes
Oct 8, 2026
Several places fill in memory flags for a buffer whose capabilities were never detected, and all of them hardcoded UCS_MEM_FLAG_REGISTRABLE. Now that UCS_MEM_FLAG_PINNABLE gates gdr_copy, such a buffer would never be considered pinnable, silently disabling gdr_copy. Add UCS_MEM_FLAGS_DEFAULT with all capabilities a memory domain can require, and use it as the undetected default. Detection results are left untouched, so a buffer the cuda_copy MD identifies as localized or mempool-backed still reports no PINNABLE flag. UCS_MEM_FLAG_MEMTYPE_COPY_INTER_NODE is excluded from the default, as it is not part of any required_mem_flags and must only be reported when actually detected.
Co-authored-by: tomerg-nvidia <tgilad@nvidia.com>
tomerg-nvidia
previously approved these changes
Oct 9, 2026
|
🤖 Starting review — findings will be posted here when done. |
Only gdr_copy requires the pinnable flag, and it only registers CUDA device memory, so rename it to UCS_MEM_FLAG_GDR_CAPABLE and report it only when cuda_copy detects it, instead of assuming it by default for every memory type. All the defaults go back to UCS_MEM_FLAG_REGISTRABLE, so UCS_MEM_FLAGS_DEFAULT is removed. Detect the flag from what the driver reports: the gpuDirectRDMACapable flag of VMM allocations, and the CU_POINTER_ATTRIBUTE_IS_GPU_DIRECT_RDMA_CAPABLE attribute queried together with the other pointer attributes otherwise. Guessing from the allocation kind missed device VMM allocated without gpuDirectRDMACapable, host-located VMM, and all memory allocated under an MPS MLOPart device, where localized memory is reported as plain device memory. The memory pool query and the localized marker are no longer needed.
|
🤖 Starting review — findings will be posted here when done. |
tvegas1
changed the title
tomerg-nvidia
previously approved these changes
Oct 9, 2026
Co-authored-by: tomerg-nvidia <tgilad@nvidia.com>
tomerg-nvidia
approved these changes
Oct 9, 2026
|
🤖 Starting review — findings will be posted here when done. |
tvegas1
enabled auto-merge (squash)
October 9, 2026 20:45
This was referenced Oct 10, 2026
This was referenced Oct 13, 2026
This was referenced Oct 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Do not use
gdr_copyfor memory it cannot pin: memory localized to a GPU locality domain, and stream-ordered (memory pool) allocations. Let the memory type endpoint pick another RMA lane instead of aborting.Replaces #11759 and #12004.
No added memory type query on the proto v2 path, as the datatype iterator already carries the flags (confirmed performance gap is within noise, 0.3%).
Why?
Both abort on send:
gdr_copypins throughnvidia_p2p_get_pages, which refuses memory the driver did not place as GDR-capable. Registrability cannot discriminate them, as an exportable memory pool exports a dma_buf fd and stays registrable.How?
UCS_MEM_FLAG_PEER_MEM_PINNABLE, cleared bycuda_copyfor both cases and required bygdr_copy, asib_mdalready does forUCS_MEM_FLAG_REGISTRABLE.ucp_mem_type_pack()/unpack()pick the first mem type RMA lane whoserequired_mem_flagsthe buffer satisfies.