TL/CUDA: add support for ucc mem map - #1315
Conversation
|
risk is low - only there when you call ucc_memmap() and pass memhandle, AND use this algo. so pretty minimal. |
6ade86f to
fe6af93
Compare
57db563 to
11caf1c
Compare
|
🤖 CI Triage Agent — TL;DR: The codestyle "commit title" check failed because 7 of the 9 commits in PR #1315 have titles longer than the 50-character limit; the fix is to reword those commit titles to ≤50 chars (or ≤50 excluding the prefix) and force-push. Full analysisSummary: The Root cause: The workflow's
The two commits under 50 chars passed. This is purely a commit-message hygiene failure, not a code/test defect. (Note: the check measures the entire title including the Implicated commit: The failing HEAD is File: Suggested fix: Interactively rebase the branch ( Related: none found (deterministic lint rule in |
ddd923c to
cfbfa69
Compare
|
🤖 CI Triage Agent — TL;DR: The codestyle commit-title lint failed because commit Full analysisSummary: The Root cause: The commit-title validator (inline script in Implicated commit: File: Suggested fix: Reword the offending commit title to use a valid prefix, e.g. Alternatively, if Related: none found (didn't need issue search — the failure is fully explained by the log). Note: the git-credentials AUTHORIZATION header appears redacted (
|
cfbfa69 to
02db9dc
Compare
02db9dc to
ab444c9
Compare
|
🤖 CI Triage Agent — TL;DR: The codestyle job's commit-title check rejected the commit Full analysisSummary: The Root cause: The lint script validates each commit title against an allowed header regex built from two lists:
The regex Implicated commit: File: Suggested fix: Reword the commit title to conform to the allowed header format. Options:
Related: none found.
|
75eabbc to
d038b0b
Compare
|
| Filename | Overview |
|---|---|
| src/components/tl/cuda/tl_cuda_context.c | Implements CUDA memory-handle mapping, serialization, bounded import parsing, and unmapping; the core importer now rejects truncated and oversized records before invoking it. |
| src/core/ucc_context.c | Adds serialized-buffer length tracking and subtraction-based record validation before dispatching imported handles to transport components. |
| src/components/tl/cuda/alltoallv/alltoallv_push.c | Implements the shared push state machine, peer IPC mapping, completion synchronization, topology gating, and balanced setup-error cleanup. |
| src/components/tl/cuda/alltoall/alltoall_push.c | Adapts the push state machine to uniform alltoall counts and globally mapped destination handles. |
| tools/perf/ucc_pt_coll_alltoallv.cc | Exchanges and imports global handles while preserving identical cross-rank collective ordering and synchronizing failures before cleanup. |
| test/gtest/core/test_mem_map.cc | Extends core coverage for memory-handle serialization and import behavior. |
Reviews (4): Last reviewed commit: "TL/CUDA: a2av push: pass raw mapped addr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (strncmp(tl_name, "cuda", UCC_MEM_MAP_TL_NAME_LEN) == 0 && | ||
| *packed_size > 0) { | ||
| /* Format: [ipc_handle] [base_address] [length] [offset] */ | ||
| size_t poff = 0; | ||
| memcpy(&m_data->ipc_handle, | ||
| PTR_OFFSET(packed_data, poff), | ||
| sizeof(cudaIpcMemHandle_t)); | ||
| poff += sizeof(cudaIpcMemHandle_t); | ||
| memcpy(&m_data->base_address, | ||
| PTR_OFFSET(packed_data, poff), sizeof(void *)); | ||
| poff += sizeof(void *); | ||
| memcpy(&m_data->length, | ||
| PTR_OFFSET(packed_data, poff), sizeof(size_t)); | ||
| poff += sizeof(size_t); | ||
| memcpy(&m_data->offset, | ||
| PTR_OFFSET(packed_data, poff), sizeof(ptrdiff_t)); |
There was a problem hiding this comment.
When an imported CUDA record has a nonzero packed_size but does not contain the complete fixed payload, this code unconditionally reads the IPC handle, pointer, length, and offset beyond the record, causing an out-of-bounds host read or corrupted mapping state. Validate both the record size and aggregate buffer bounds before reading. How this was verified: The import callback receives no available-length bound and performs every fixed-size copy after checking only that packed_size is nonzero.
| status = ucc_tl_cuda_map_memhandle( | ||
| peer_mi.ptr, peer_mi.length, peer_mi.handle, &mapped, cache); | ||
| if (ucc_unlikely(status != UCC_OK)) { | ||
| return status; | ||
| } | ||
| task->alltoallv_push.peer_map_addr[i] = PTR_OFFSET(mapped, peer_mi.offset); | ||
| } | ||
|
|
||
| status = ucc_ec_create_event(&task->alltoallv_push.evt_completion, | ||
| UCC_EE_CUDA_STREAM); | ||
| if (ucc_unlikely(status != UCC_OK)) { | ||
| return status; |
There was a problem hiding this comment.
When mapping a later peer or creating the completion event fails after earlier peer mappings succeeded, this function returns without decrementing those mappings' cache references. Because the finalize callback is installed only after setup completes, repeated initialization failures leave IPC mappings open and eventually exhaust CUDA IPC or cache resources; unwind every previously acquired mapping on these error paths.
Knowledge Base Used: Transport-layer components
| for (int i = 0; i < comm->get_size(); i++) { | ||
| ucc_mem_map(ctx, UCC_MEM_MAP_MODE_IMPORT, &mem_map_params, | ||
| &dst_memh_size_max, &dst_memh_global[i]); | ||
| } |
There was a problem hiding this comment.
When any global destination or source handle import fails, setup ignores the returned status and still attaches that handle array to the collective arguments. The benchmark then proceeds with unusable peer handles, causing collective initialization failure or invalid-handle access instead of reporting and cleaning up the mapping error; apply the same checked error path used for exports to every import.
Knowledge Base Used: Performance benchmarking
| /* Format: [ipc_handle] [base_address] [length] [offset] */ | ||
| size_t poff = 0; | ||
|
|
||
| if (*packed_size < UCC_TL_CUDA_MEMH_PACKED_SIZE) { |
There was a problem hiding this comment.
When an imported handle declares a sufficiently large CUDA payload but the aggregate serialized buffer is truncated or a preceding record advances beyond it, this check passes and the subsequent copies read outside the buffer, causing a crash or corrupted mapping state. Validate each record header and payload against the total imported blob length before reading or advancing. How this was verified: The parser derives every header and payload from an unbounded offset, while its callback input provides no available packed-buffer length.
Knowledge Base Used: Transport-layer components
| if (st != UCC_OK) { | ||
| /* Import failed: dst_memh_global[i] is still a raw serialized | ||
| * blob, not a valid handle. Free it and any not-yet-imported | ||
| * entries so the error path only unmaps valid handles. */ | ||
| for (int j = i; j < comm->get_size(); j++) { | ||
| ucc_free(dst_memh_global[j]); | ||
| dst_memh_global[j] = NULL; | ||
| } | ||
| goto exit; |
There was a problem hiding this comment.
When handle import fails on one rank but succeeds on another, this rank jumps directly to cleanup and throws without communicating the failure, while successful peers proceed to the next team barrier. The failed rank enters communicator teardown instead of that barrier, causing the benchmark to hang or terminate abnormally; synchronize import status across ranks before either teardown or timed collectives begin.
Knowledge Base Used: Performance benchmarking
TL/CUDA: add push algorithm TL/CUDA: add alltoallv memh TEST: MPI add user memh reg. (local + global) TEST/MPI: add dispatch hook for test_mem_map.cc so mem_map tests execute TEST/MPI: fix global memh and mem-map test lifecycle TL/CUDA: unify alltoall/alltoallv push algorithms
970801b to
102e849
Compare
| } | ||
| } | ||
|
|
||
| if (!is_inplace) { |
There was a problem hiding this comment.
When destination-handle import fails on one rank in a non-in-place benchmark, that rank advances to the failure-status allreduce while successful ranks first enter the source-size allreduce and source-handle broadcasts. The mismatched collective sequence leaves successful ranks waiting in an unmatched broadcast, causing the benchmark to hang.
Knowledge Base Used:
A local dst/src handle export failure used UCCCHECK_GOTO(exit), tearing down one rank while peers advanced into the size allreduce and handle bcasts, hanging the benchmark. Route export failures through the existing map_failed flag so every rank reaches the failure-status allreduce together. Also pass a size_t temp to ucc_mem_map's memh_size out-param (matching its signature) while keeping uint64_t operands for the UCC_DT_UINT64 allreduce.
peer_map_addr stored the offset-adjusted peer rbuf base, but ucc_tl_cuda_unmap_memhandle expects the raw cudaIpcOpenMemHandle address as mapped_addr (used for cudaIpcCloseMemHandle when refcount hits 0 and force=1). Store the raw address in a new peer_map_raw field and pass it to both unmap sites.
3 participants
What
Adds support for ucc_mem_map with TL/CUDA by implementing ucc_tl_cuda_mem_map, ucc_tl_cuda_memh_pack, and ucc_tl_cuda_mem_unmap. Additionally, adds an alltoall and alltoallv algorithm that takes advantage of the memory handles.