Repository navigation
fix(rccl): fit AINIC scatter parts to the P2P channel pool - #13528
Merged
i-kosarev merged 6 commits intoOct 15, 2026
Conversation
Scatter drives all nRanks-1 peers from one root at once. Instead of dropping to a single part whenever (nRanks - 1) * p2pnChannelsPerPeer exceeds p2pnChannels, ncclTopoComputeP2pChannels now sets p2pnChannelsPerPeerScatter to the largest power-of-two part count whose flows still fit the pool, capped at p2pnChannelsPerPeer: nodes x 8 GPUs p2pnChannels perPeer before: scatter now: scatter 2 32 2 2 2 3 64 4 1 2 4 32 2 1 1 5 64 2 1 1 8 64 2 1 1 So 4, 5 and 8 nodes keep the single part, while 3 nodes uses 2 parts (46 flows on 64 channels) instead of 1. A user-set nChannelsPerNetPeer is left alone. The P2P-disable comment in rcclP2pPolicyChannels moves below the scatter rule it does not describe, and the measured range in the comment and CHANGELOG is corrected. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
…rt split postTuneP2pRecordPreconnect gained a collAPI parameter in this branch and now calls rcclP2pTaskParts, so rccl-UnitTestsMicroTaskPrep stopped building: task-posttuning-test.cc still used the old signature, and the binary, which fakes enqueue.cc, had no rcclP2pTaskParts to link. The existing calls now pass ncclFuncSendRecv, which keeps their meaning, and enqueue_fakes gains an rcclP2pTaskParts hook whose default reads the same comm fields as the real one. The scenes also set p2pnChannelsPerPeerScatter as ncclTopoComputeP2pChannels does, a new test covers a scatter task that claims fewer parts than p2pnChannelsPerPeer, and rcclP2pTaskParts / rcclP2pPolicyChannels get direct tests. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
requested review from
amd-arozanov and
nikitaxgusev
and
a balanced review from Copilot
October 15, 2026 06:57
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Only a nonblocking regression-test coverage suggestion remains; the supplied performance and correctness results support the focused change.
1 open finding
What changed in this PR
Refines #13468’s AINIC Scatter policy to use the largest per-peer power-of-two part count that fits the channel pool.
Changes:
- Preserves two Scatter parts on three-node configurations and honors explicit channel overrides.
- Repairs host microtest integration and adds part-selection and preconnect tests.
- Updates the changelog.
| File | Description |
|---|---|
| projects/rccl/test/host/task-posttuning-test.cc | Updates preconnect calls and tests partial connections. |
| projects/rccl/test/host/fakes/enqueue_fakes.h | Declares the task-parts test hook. |
| projects/rccl/test/host/fakes/enqueue_fakes.cc | Implements and resets the hook. |
| projects/rccl/test/host/enqueue-test.cc | Tests Scatter part selection. |
| projects/rccl/src/include/comm.h | Stores the Scatter-specific part count. |
| projects/rccl/src/graph/paths.cc | Computes the pool-fitting part count. |
| projects/rccl/src/enqueue/enqueue.cc | Uses the computed Scatter limit. |
| projects/rccl/CHANGELOG.md | Documents the revised policy. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drop p2pnChannelsPerPeerScatter and its computation in ncclTopoComputeP2pChannels: rcclP2pTaskParts derives the limit itself from comm-wide values, like the rule it replaces. Multi-node AINIC scatter takes the largest power of two not above p2pnChannels / (nRanks - 1), capped at p2pnChannelsPerPeer. Review fixes on #13468: - keep a user-set nChannelsPerNetPeer (NCCL_NCHANNELS_PER_NET_PEER), as ncclTopoComputeP2pChannels does; - shorten the rcclP2pTaskParts comment, move the P2P-disable comment next to its check and fix the gfx110 note about Scatter; - postTuneP2pRegisterBuffer again uses p2pnChannelsPerPeer: Scatter never reaches it (allowUB is false). The host tests now drive the real rule through g_rcclUseAinicValue, covering 2, 3, 4 and 8 nodes, power-of-two rounding, the one-part floor and the nChannelsPerNetPeer override. The TaskPrep fake returns p2pnChannelsPerPeer, and its Scatter test installs the limit. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
Since the NCCL 2.28.9 merge (#6024), ncclP2pSchedule groups ranks by NCCL_P2P_SCHEDULE_GROUP_SIZE, which defaults to NCCL_MAX_DEV_WORK_P2P_PER_BATCH: 8 in NCCL, a whole node, but 2 in RCCL. The rounds, and with them the P2P channels, now interleave nodes two local ranks at a time. A scatter root drives every peer at once and loses 7-15% from 32 MB up on that layout (3 and 8 nodes of 8 GPUs). NCCL_P2P_SCHEDULE_GROUP_SIZE=8 brings it back, but costs ring send/recv up to 40%. So on multi-node AINIC with the same number of ranks on every node, scatter takes its channel base from the node-major index of the (sender, receiver) pair, which both ends compute alone. A round that pairs a scatter direction with a task on other channels becomes two one-sided works, since a work carries one channel base. Both preconnect paths use the same base. Scatter neither relies on nor sets send/recvSeen, which cover the schedule-round channels. The other operations keep the schedule. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
changed the title
… node" This reverts commit a4b7065.
…e pool boundary The preconnect test now runs on the send and the recv copy: a one-part scatter leaves the side unseen and claims part 0 only, and a later full-part task adds just the missing channel. The part-rule test gains the pool-equality boundary (16 peers x 2 parts fill 32 channels, 17 peers do not). JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
changed the title
i-kosarev
merged commit Oct 15, 2026
8efe3b0
into
users/nikitaxgusev/rocm-31938-ainic-scatter-single-channel
5 checks passed
1 task done
nikitaxgusev
pushed a commit
that referenced
this pull request
Oct 16, 2026
On multi-node AINIC, rcclP2pTaskParts gives Scatter the largest power of two not above p2pnChannels / (nRanks - 1) parts per peer, capped at p2pnChannelsPerPeer, instead of one part whenever the peers oversubscribe the pool. Same parts as before on 2, 4, 5 and 8 nodes of 8 GPUs; 2 instead of 1 on 3 nodes, where one part left two-thirds of the pool idle (scatter 0.85-0.87 of develop instead of 1.04-1.13). Review fixes: a user-set NCCL_NCHANNELS_PER_NET_PEER is kept, as in ncclTopoComputeP2pChannels; shorter comments, the P2P-disable comment next to its check and the gfx110 note fixed; postTuneP2pRegisterBuffer back to p2pnChannelsPerPeer, since Scatter never reaches it. Host microtests: rccl-UnitTestsMicroTaskPrep builds again (collAPI argument, rcclP2pTaskParts fake), with tests for the part rule and for the partial-to-full preconnect on both sides. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
nikitaxgusev
pushed a commit
that referenced
this pull request
Oct 16, 2026
On multi-node AINIC, rcclP2pTaskParts gives Scatter the largest power of two not above p2pnChannels / (nRanks - 1) parts per peer, capped at p2pnChannelsPerPeer, instead of one part whenever the peers oversubscribe the pool. Same parts as before on 2, 4, 5 and 8 nodes of 8 GPUs; 2 instead of 1 on 3 nodes, where one part left two-thirds of the pool idle (scatter 0.85-0.87 of develop instead of 1.04-1.13). Review fixes: a user-set NCCL_NCHANNELS_PER_NET_PEER is kept, as in ncclTopoComputeP2pChannels; shorter comments, the P2P-disable comment next to its check and the gfx110 note fixed; postTuneP2pRegisterBuffer back to p2pnChannelsPerPeer, since Scatter never reaches it. Host microtests: rccl-UnitTestsMicroTaskPrep builds again (collAPI argument, rcclP2pTaskParts fake), with tests for the part rule and for the partial-to-full preconnect on both sides. JIRA ID : ROCM-31938 Co-authored-by: Cursor <cursoragent@cursor.com>
arvindcheru
pushed a commit
that referenced
this pull request
Oct 16, 2026
…er peer (#13468) ## Motivation Commit 242e0a9 (#11635) raised `p2pnChannelsPerPeer` from 1 to 2 on AINIC for jobs of up to 8 nodes, so that a single peer can saturate a NIC. That helps point-to-point traffic, but `ncclScatter` regressed by 15 to 21% from 32 MB upward on multi-node AINIC jobs compared with 7.14. Scatter already drives all `nRanks - 1` peers from one root, so splitting each peer into two parts adds no bandwidth and costs some. This PR restores scatter performance without changing the channel count for the other collectives. ## Technical Details - New helper `rcclP2pTaskParts(comm, collAPI)` returns how many of the `p2pnChannelsPerPeer` parts a P2P task uses and connects. For `ncclFuncScatter` on multi-node AINIC it returns the largest power of two not above `p2pnChannels / (nRanks - 1)`, capped at `p2pnChannelsPerPeer` and at least 1, so the root's flows still fit the P2P channel pool. A user-set `NCCL_NCHANNELS_PER_NET_PEER` is kept, as in `ncclTopoComputeP2pChannels`. Otherwise it returns `p2pnChannelsPerPeer`. - The decision depends only on `collAPI` and comm-wide sizes, so sender and receiver always agree on the number of parts. - Limited to AINIC: it is the only NIC path that raises `p2pnChannelsPerPeer` back to 2 on multi-node (#11635), and the only one measured. On other NICs the halving loop in `ncclTopoComputeP2pChannels` already reduces the parts per peer when the peers do not fit the pool. - Used in three places: - channel count selection (`rcclP2pPolicyChannels`), also with P2P disabled, since the limit is about network flows; - the connector checks and buffer registration in `addP2pToPlan`; - preconnect (`p2pTaskAppend`, `postTuneP2pRecordPreconnect`). - NIC buffer registration (`postTuneP2pRegisterBuffer`) keeps `p2pnChannelsPerPeer`: Scatter tasks have `allowUB` off and never reach it. - When only some parts are connected, `sendSeen` / `recvSeen` stay clear, so a later task to the same peer that uses all parts still connects the rest. - For every other collective and for send/recv the helper returns `p2pnChannelsPerPeer`, which equals `nChannelsMax` in `addP2pToPlan`, so their code path takes the same decisions as before. - Host microtests: - `EnqueueMicrotest.P2pTaskParts_*`: the part rule (pools of 2, 3, 4, 8 and 16 nodes, the pool boundary, power-of-two rounding, the one-part floor, non-AINIC, single node, user-set count); - `EnqueueMicrotest.P2pPolicyChannels_ScatterBelowPerPeerReturnsItsParts`: the channel count; - `TaskPostTuningMicrotest.P2pRecordPreconnect_ScatterWithFewerParts_ClaimsItsPartsAndLeavesTheRestForLater`: the partial-to-full preconnect on both sides. Limiting only the channel count is not enough. When scatter sends on part 0 but connects both parts itself, it stays at or below the two-channel speed in most runs (4 of 5 runs at about 110 GB/s instead of 137 GB/s). Not connecting part 1 makes the result stable. Part 1 connected later by another collective on the same communicator (alltoall) does not slow scatter down. The exact runtime cause of the slowdown is not identified yet: the connection setup logs of fast and slow runs are identical. With `NCCL_PXN_DISABLE=1` there is no regression, because the root is limited by its own NIC. ## Issue Tracking JIRA ID : ROCM-31938 ## Test Plan - 4 nodes x 8 MI355X (32 ranks), AINIC (`NCCL_NET=ROCM-IB`, `RCCL_AINIC_ROCE=1`), PXN on. - rccl-tests with data check on (`-c 1`), `-n 20 -w 5`, 5 cycles per run. - Three builds from the same base, run back to back in every repeat: - revert: develop without the AINIC `p2pnChannelsPerPeer` raise from 242e0a9 (matches 7.14); - develop; - this PR. - All P2P collectives (scatter, gather, alltoall, alltoallv, all_gather, reduce_scatter) up to 16 GB, 2 repeats. - All collectives (also sendrecv, all_reduce, broadcast, reduce) up to 2 GB, 3 repeats. - Scatter on one communicator before and after alltoall and after ring sendrecv, with every received byte checked. - Scatter with `NCCL_PXN_DISABLE=1` and on 2 nodes. - Scatter 16M–1G, root 0, on 2, 3, 4, 5 and 8 nodes, interleaved with develop and the first version of this PR (one part per peer whenever the parts did not fit); all roots on 3 and 4 nodes (from #13528). - Host microtests `EnqueueMicrotest.*` and `TaskPostTuningMicrotest.*`. ## Test Result Scatter, 4 nodes, 32 ranks, median time over 10 samples. Measured with the first version of this PR; on 4 nodes the current rule picks the same single part. | Size | revert (us) | develop (us) | this PR (us) | revert busbw (GB/s) | develop busbw (GB/s) | this PR busbw (GB/s) | develop / revert | this PR / revert | |---:|---:|---:|---:|---:|---:|---:|---:|---:| | 8M | 48.9 | 49.9 | 48.6 | 166.2 | 162.9 | 167.2 | 1.02 | 0.99 | | 16M | 69.9 | 73.3 | 70.1 | 232.5 | 221.7 | 231.9 | 1.05 | 1.00 | | 32M | 109.2 | 132.2 | 111.0 | 297.7 | 245.9 | 292.8 | 1.21 | 1.02 | | 64M | 190.4 | 228.6 | 190.7 | 341.4 | 284.4 | 340.9 | 1.20 | 1.00 | | 128M | 346.9 | 413.8 | 347.5 | 374.8 | 314.2 | 374.2 | 1.19 | 1.00 | | 256M | 655.5 | 768.9 | 655.3 | 396.7 | 338.2 | 396.8 | 1.17 | 1.00 | | 512M | 1281.6 | 1490.8 | 1278.1 | 405.8 | 348.9 | 406.9 | 1.16 | 1.00 | | 1G | 2537.6 | 2951.9 | 2534.4 | 409.9 | 352.4 | 410.4 | 1.16 | 1.00 | | 2G | 5048.6 | 5865.6 | 5050.6 | 412.1 | 354.7 | 411.9 | 1.16 | 1.00 | | 4G | 10082.9 | 11698.2 | 10084.8 | 412.7 | 355.7 | 412.6 | 1.16 | 1.00 | | 8G | 20167.1 | 23342.6 | 20175.5 | 412.6 | 356.5 | 412.5 | 1.16 | 1.00 | | 16G | 40433.2 | 46671.4 | 40393.4 | 411.6 | 356.6 | 412.0 | 1.15 | 1.00 | Scatter by node count, root 0, time ratio vs develop over 16M–1G (from #13528, merged here): | Nodes | Parts per peer (develop / first version / this PR) | first version | this PR | | --- | --- | --- | --- | | 2 | 2 / 2 / 2 | 0.99–1.01 | 0.96–1.00 | | 3 | 4 / 1 / 2 | 1.04–1.13 | 0.85–0.87 | | 4 | 2 / 1 / 1 | 0.80–0.86 | 0.79–0.86 | | 5 | 2 / 1 / 1 | 0.92–1.11 | 0.91–1.08 | | 8 | 2 / 1 / 1 | 0.97–1.02 | 0.94–1.00 | - Scatter matches revert from 8 MB to 16 GB. Develop is 15 to 21% slower from 32 MB up. - 3 nodes: the first version dropped 4 parts to 1 and left two-thirds of the 64-channel pool idle; this PR keeps the 2 parts that fit. All roots on 3 nodes: median 0.82–0.90 vs develop (first version: 0.99–1.10). All roots on 4 nodes: both at 0.84–0.90. - Scatter after alltoall on the same communicator stays at revert level (1 GB: about 2530 us, develop about 2980 us). - With root != 0, scatter shows run-to-run bimodality (about 2.5 vs about 3.5 ms at 1 GB), as in the revert build (pre-242e0a93afa, one part per peer); develop, with two parts per peer, does not show it. The median over all roots is 0.82–0.89 of develop, and root 0 is stable at revert level in every run. - gather, all_gather, reduce_scatter, all_reduce, broadcast, reduce, sendrecv: no difference from develop beyond run-to-run noise. Outliers above 5% only at latency-bound sizes from 1 KB to 1 MB (a few us at a 20 to 30 us spread), also in collectives that do not go through the changed code. - alltoall and alltoallv show the same difference against revert in develop and in this PR (alltoall: develop 1.05, this PR 1.02 to 1.045 at 256 MB to 16 GB). This PR does not add to it. - 2 nodes: no regression in develop, this PR is unchanged. - `NCCL_PXN_DISABLE=1`: all builds at about 64 GB/s at 256 MB, no regression. - Joint validation on ROCm 10.2 (branch ROCM-31938-validation = develop + #13451 + this PR + #13536, 4 nodes, median of 10 from two launches in opposite build order): scatter from 16 MB 0.78–0.86 of develop; against 7.2.3 16 MB 1.13, 32 MB 1.07, from 128 MB 0.95–0.98. The other collectives match develop within noise; on alltoallv two copies of the same develop build differ by up to 6% (A/A run), and validation is 0.98–1.04 of develop over 20 measurements. - Correctness: `#wrong = 0` and no out-of-bounds values in every run. - Host microtests `EnqueueMicrotest.*` and `TaskPostTuningMicrotest.*` pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Ilia Kosarev <Ilia.Kosarev@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Motivation
#13468 moves
ncclScatterto one part per peer whenever the root's peers times parts exceedp2pnChannels. On 3 nodes of 8 GPUs that turns 4 parts into 1 and leaves two-thirds of the 64-channel pool idle, which is slower than develop, while 2 parts fit the pool. The branch also no longer buildsrccl-UnitTestsMicroTaskPrep, and the review on #13468 has open comments.Technical Details
rcclP2pTaskParts: multi-node AINIC Scatter takes the largest power of two not abovep2pnChannels / (nRanks - 1), capped atp2pnChannelsPerPeer. Same parts as fix(rccl): restore ncclScatter bandwidth on AINIC with two channels per peer #13468 on 2, 4, 5 and 8 nodes; 2 parts instead of 1 on 3 nodes.NCCL_NCHANNELS_PER_NET_PEERis kept, as inncclTopoComputeP2pChannels; shorterrcclP2pTaskPartscomment; the P2P-disable comment sits next to its check; the gfx110 note no longer says Scatter keeps multiple channels;postTuneP2pRegisterBufferusesp2pnChannelsPerPeeragain, since Scatter never reaches it.task-posttuning-test.ccpassescollAPItopostTuneP2pRecordPreconnectandenqueue_fakesgainsrcclP2pTaskParts, sorccl-UnitTestsMicroTaskPrepbuilds again. New tests cover the part rule (pool sizes of 2, 3, 4 and 8 nodes, the pool boundary, power-of-two rounding, the one-part floor, non-AINIC, single node, user-set count) and the partial-to-full preconnect on both sides.Issue Tracking
JIRA ID: ROCM-31938
Test Plan
MI355X with AINIC, 8 GPUs per node, RCCL built against ROCm 7.0.1:
scatter_perf16M–1G, root 0, interleaved with develop and fix(rccl): restore ncclScatter bandwidth on AINIC with two channels per peer #13468, on 2, 3, 4, 5 and 8 nodes; all roots on 3 and 4 nodes.alltoall_perf,alltoallv_perf,gather_perfandsendrecv_perf4M–1G on 4 nodes, develop vs this PR.EnqueueMicrotest.*andTaskPostTuningMicrotest.*.Test Result
scatter_perftime, root 0, ratio vs develop over 16M–1G:Submission Checklist