Repository navigation
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
|
No test is added. The failure only reproduces on ODP-capable HW with |
|
🤖 Starting review — findings will be posted here when done. |
| /* CUDA managed memory cannot be pinned, it can only be registered on demand | ||
| * (e.g. by ODP), regardless of UCX_REG_NONBLOCK_MEM_TYPES. Request | ||
| * blocking registration for any other memory type. */ | ||
| if (UCP_MEM_IS_CUDA_MANAGED(mem_type)) { |
There was a problem hiding this comment.
why not handle this in ucp_memh_register_internal()? ucp_mem_map() with UCP_MEM_MAP_LOCK on cuda-managed memory hits the same failure, and a central fix would cover all callers.
| ucs_status_t status; | ||
| unsigned num_elems; | ||
|
|
||
| /* CUDA managed memory cannot be pinned, it can only be registered on demand |
There was a problem hiding this comment.
can we cover the other managed types here? if IB_ODP_MEM_TYPES is set to rocm-managed or ze-managed, those frag pools fail the same way, since reg_mem_types gets the ODP types in uct_ib_md_check_odp().
| sys_dev, UCP_MEMH_FLAG_NO_RCACHE, uct_flags, 0, | ||
| ucs_mpool_name(mp), &chunk_hdr->memh); | ||
| if (status != UCS_OK) { | ||
| return status; |
There was a problem hiding this comment.
minor: chunk_hdr is leaked here; pls free it before returning. Pre-existing, but this is exactly the failure path this PR fixes.
| ucs_status_t status; | ||
| unsigned num_elems; | ||
|
|
||
| /* CUDA managed memory cannot be pinned, it can only be registered on demand |
There was a problem hiding this comment.
The fix seems odd: the frag mpool hardcodes a memory type to bypass LOCK, so other callers (e.g. ucp_mem_map() with UCP_MEM_MAP_LOCK on managed memory) still fail. Note that LOCK was never implemented by IB MD; it only uses NONBLOCK to select ODP vs pinned registration.
Suggestions WDYT:
-
to instead have the memory detector report it? cuda_copy already identifies managed memory by
CU_POINTER_ATTRIBUTE_IS_MANAGEDand assumes it is registered via ODP (cuda_copy_md.c:893), so it could set a newUCS_MEM_FLAG_NOT_PINNABLEinuct_cuda_copy_md_detect_mem_flags(), anducp_memh_register_internal()would replaceLOCKwithNONBLOCKfor such memh. This covers all callers and can be extended to rocm/ze-managed. It needs OR semantics for this flag on rcache merge (ucp_mm.c:1765), since a negative flag is lost with AND. -
Otherwise, maybe fix it in IB MD: UCP already passes
mem_typefor cuda-managed, souct_ib_verbs_mem_reg()anduct_ib_mlx5_devx_mem_reg()can do:
/* CUDA managed memory cannot be pinned, it can be registered only by ODP */
if ((mem_type == UCS_MEMORY_TYPE_CUDA_MANAGED) &&
(md->reg_nonblock_mem_types & UCS_BIT(mem_type))) {
flags |= UCT_MD_MEM_FLAG_NONBLOCK;
}
Why?