TL/UCP: Update topo aware ring algorithm #1288
Conversation
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
from
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. |
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
3 times, most recently
from
b80e124 to
9472c37
Compare
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
2 times, most recently
from
bd96bad to
e827813
Compare
|
/build |
|
| Filename | Overview |
|---|---|
| src/components/tl/ucp/allgather/allgather.c | Score string generation rewritten to prefer the topo-aware ring for CUDA; introduces an unintended algorithm change (RING → KNOMIAL) for odd-size non-CUDA teams when cuda_ring is NULL. |
| src/components/tl/ucp/allgather/allgather_ring.c | Fully rewritten to use cuda_ring topology pattern and multiple parallel rings; missing count-divisibility guard (present in the parallel reduce_scatter path) and the counting sentinel is undocumented. |
| src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c | Old bidirectional-schedule implementation replaced by a simpler single-task topo-aware ring; non-persistent non-in-place reduction writes back into src buffer (flagged in prior thread); missing equality assertion between send/recv posted counters. |
| src/components/tl/ucp/reduce_scatter/reduce_scatter.c | New score_str_get function added to enable dynamic algo selection; pattern is consistent with the allgather counterpart. |
| src/components/tl/ucp/tl_ucp_service_coll.c | Service allgather correctly decoupled into dedicated flat-ring start/progress functions so internal collectives are unaffected by the topo-aware rewrite. |
| src/components/topo/cuda/ucc_sysinfo_cuda.c | Adds a fallback to scan primary CUDA contexts when cuCtxGetCurrent fails; logic and goto use are correct; adds a debug log for visibility. |
| src/components/tl/ucp/tl_ucp_task.h | Adds scratch_mc_header fields to reduce_scatter_ring and reduce_scatterv_ring task structs; straightforward and consistent. |
| src/components/tl/ucp/tl_ucp_coll.c | Switches reduce_scatter default descriptor from static string to dynamic str_get_fn; correct use of the existing pattern. |
| src/components/tl/ucp/reduce_scatter/reduce_scatter.h | Exports new ring init/progress symbols and updates default algo select string macro to accept a printf argument; consistent with usage. |
Comments Outside Diff (1)
-
src/components/tl/ucp/allgather/allgather_ring.c, line 255-283 (link)Missing count-divisibility check — silent data truncation
reduce_scatter_ring_init_common(the parallel path introduced in the same PR) guards againsttotal_count % tsize != 0and returnsUCC_ERR_NOT_SUPPORTED. The newallgather_ring_init_commonhas no such check.When
args->dst.info.countis not divisible byucc_ring_pattern_size(ring, 0), the integer divisionblock_count = count / tsizesilently truncates and the lastcount % tsizeelements are never transferred, producing an incorrect allgather result with no error reported.tsize = ucc_ring_pattern_size(ring, 0); if (count % tsize != 0) { tl_error(UCC_TASK_LIB(task), "allgather ring: count %zu not divisible by tsize %u", count, tsize); return UCC_ERR_NOT_SUPPORTED; }
Reviews (5): 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.
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
from
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 |
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
from
282b683 to
453ac19
Compare
|
/build |
Juee14Desai
force-pushed
the
ucc-ring-algo
branch
from
453ac19 to
e85aca1
Compare
|
/build |
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>
|
/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.
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: