TL/UCP: Update topo aware ring algorithm - #1288
Conversation
6c2242e to
843043c
Compare
|
/build |
|
@Juee14Desai @janjust I assume this will replace PR #1258 ? |
|
We talked about this, it shouldn't need to. If they are both good to go then let's have both ring and topo-aware ring and we can phase one out as needed. |
b80e124 to
9472c37
Compare
bd96bad to
e827813
Compare
|
/build |
|
| Filename | Overview |
|---|---|
| src/components/tl/ucp/allgather/allgather.c | Adds CUDA-topology-aware score selection, while the previously reported no-topology fallback regression remains. |
| src/components/tl/ucp/allgather/allgather_ring.c | Rewrites allgather around up to eight CUDA topology rings, while the previously reported non-divisible-count truncation remains. |
| src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c | Replaces the fragmented flat ring with topology-aware multi-ring reduction and scratch management, while non-in-place source mutation remains. |
| src/components/tl/ucp/tl_ucp_service_coll.c | Moves service allgather onto a dedicated flat-ring implementation so bootstrap collectives do not depend on CUDA topology. |
| src/components/topo/cuda/ucc_sysinfo_cuda.c | Updates CUDA topology information used to construct topology-aware rings. |
Reviews (8): Last reviewed commit: "TL/UCP: topo aware ring algo for reduce_..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| send_idx = ucc_ring_pattern_get_send_block(ring, ring_id, | ||
| rrank, step); | ||
| ring_offset = ucc_buffer_block_offset(block_cnt, nrings, ring_id); |
There was a problem hiding this comment.
send_posted == recv_posted assertion
The allgather ring progress function asserts task->tagged.send_posted == task->tagged.recv_posted at the top of its loop (since sends and recvs are always posted in pairs). The allreduce ring's RS-phase loop has the send_posted > 0 / recv_posted > 0 guards but omits the equality check, making it harder to catch a bookkeeping bug early. Consider adding the same assertion for consistency and diagnosability.
e827813 to
282b683
Compare
| reduce_target = PTR_OFFSET( | ||
| sbuf, | ||
| (recv_block * block_cnt + ring_offset) * dt_size); |
There was a problem hiding this comment.
For non-in-place reduce_scatter, sbuf = args->src.info.buffer. On every non-final step the reduction writes back into that same buffer:
reduce_target = PTR_OFFSET(sbuf, (recv_block * block_cnt + ring_offset) * dt_size);The subsequent send loop then reads from the now-mutated sbuf location to forward the intermediate partial sum. The old implementation accumulated partial results in the dedicated s_scratch send buffer and left sbuf read-only. Callers with separate source and destination buffers (i.e., any non-in-place reduce_scatter over CUDA memory when cuda_ring is present) will have their source data silently overwritten.
The simplest safe fix is to reject non-in-place in reduce_scatter_ring_init_common (analogous to the UCC_IS_PERSISTENT guard already there):
if (!UCC_IS_INPLACE(*args)) {
return UCC_ERR_NOT_SUPPORTED;
}Or alternatively, allocate a separate per-ring work buffer for intermediate reductions instead of reusing sbuf.
|
/build |
2 similar comments
|
/build |
|
/build |
282b683 to
453ac19
Compare
|
/build |
453ac19 to
e85aca1
Compare
|
/build |
|
/build |
| } | ||
| } | ||
|
|
||
| if (algo_num == UCC_TL_UCP_ALLGATHER_ALG_RING && !team->cuda_ring) { |
There was a problem hiding this comment.
When cuda_ring is NULL (any CPU team, or GPU team without NVLink topology), odd-size teams previously used the flat RING algorithm. After this guard, they fall through to KNOMIAL. This silently changes the default for every odd-size non-CUDA workload, including large CPU clusters.
The underlying reason is that allgather_ring_init_common now hard-fails with UCC_ERR_NOT_SUPPORTED when cuda_ring == NULL, so the score-string must avoid selecting it. However, the correct fallback for the "no-topology-info" case is still the flat ring (old behavior), not knomial. Consider keeping the flat-ring algorithm alive under a separate name/enum and using it as the fallback, or only switching to KNOMIAL when the caller is guaranteed to benefit.
wfaderhold21
left a comment
wfaderhold21
left a comment
There was a problem hiding this comment.
If I understand correct, this PR is removing CPU-based allgather/reduce_scatter ring algorithms. We will need a future PR to bring those back before 1.9.0 release.
| ucc_rank_t tsize = ucc_ring_pattern_size(ring, 0); | ||
| ucc_rank_t block = UCC_TL_TEAM_RANK(team); | ||
| size_t data_size = (count / tsize) * ucc_dt_size(dt); | ||
| ucc_status_t status; |
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright (c) 2021-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| * Copyright (c) 2021-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| if (!UCC_IS_INPLACE(*args)) { | ||
| status = ucc_mc_memcpy(PTR_OFFSET(rbuf, data_size * block), | ||
| sbuf, data_size, rmem, smem); | ||
| sbuf, data_size, rmem, smem); |
| @@ -1,424 +1,308 @@ | |||
| /** | |||
| * Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| * Copyright (c) 2022-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| size_t count = TASK_ARGS(task).dst.info.count; | ||
| size_t data_size = (count / tsize) * ucc_dt_size(TASK_ARGS(task).dst.info.datatype); | ||
| ucc_rank_t sendto, recvfrom, sblock, rblock; | ||
| int step; |
|
add as a target to 1.10 |
Replace the default ring allgather with a topo aware multi ring implementation that uses team->cuda_ring to route data along NVLink optimal paths (up to 8 parallel rings). Algorithm changes: - Ring rank, peer, and block indices are now derived from the cuda_ring topology pattern instead of flat team rank ordering. - Each ring transfers its own slice of each block, enabling concurrent data movement across multiple NVLink paths. - Algorithm auto selected for CUDA memory >4KB when cuda_ring is available; falls back to knomial otherwise. Also fixes CUDA primary context detection in ucc_sysinfo_cuda.c and decouples the service allgather from the topo aware ring. Signed-off-by: Juee Himalbhai Desai <jueehimalbha@nvidia.com>
Replace the default ring reduce_scatter with a topo aware multi ring implementation that uses team->cuda_ring to route data along NVLink optimal paths (up to 8 parallel rings). Algorithm changes: - Ring rank, peer, and block indices are now derived from the cuda_ring topology pattern instead of flat team rank ordering. - Each ring handles its own sub block slice, with per ring GPU reductions via the executor before forwarding to the next peer. - Scratch buffer management simplified to a single mc_alloc/free per task lifetime (removed fragmentation logic). Signed-off-by: Juee Himalbhai Desai <jueehimalbha@nvidia.com>
ca3428e to
317cfcb
Compare
5 participants
What
Add topology aware multi ring algorithms for allgather, reduce_scatter, and allreduce in TL/UCP. The ring algorithms use team->cuda_ring to route data along NVLink optimal paths with up to 8 parallel rings, instead of the default single ring.
Why ?
The default ring algorithms use a flat rank ordering that does not account for the underlying GPU interconnect topology. On multi GPU systems with NVLink, this results in suboptimal data routing transfers may traverse slower paths instead of direct NVLink links.
How ?
Allgather:
Reduce_scatter:
Allreduce: