Repository navigation
fix(rccl): restore ncclScatter bandwidth on AINIC with two channels per peer - #13468
Merged
arvindcheru merged 2 commits intoOct 16, 2026
Merged
arvindcheru merged 2 commits into
arvindcheru merged 2 commits into
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
1 task done
amd-arozanov
requested changes
Oct 15, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Existing host microtests have incompatible calls and lack the new helper definition, blocking compilation and linking.
3 open findings
What changed in this PR
Restores RCCL Scatter performance on multi-node AINIC configurations affected by the earlier increase to two channels per peer, while preserving other operations’ channel policy.
Changes:
- Adds a shared rule limiting Scatter to one part when peers exceed available channels.
- Applies the rule to planning, preconnection, and buffer registration.
- Documents the performance fix.
| File | Description |
|---|---|
| projects/rccl/src/include/channel.h | Declares the shared part-selection helper. |
| projects/rccl/src/enqueue/task_prep/task_posttuning.cc | Applies part limits during preconnection and registration. |
| projects/rccl/src/enqueue/enqueue.cc | Implements the rule and integrates it into planning and connection setup. |
| projects/rccl/CHANGELOG.md | Records the Scatter performance fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
i-kosarev
added a commit
that referenced
this pull request
Oct 15, 2026
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>
i-kosarev
marked this pull request as ready for review
October 15, 2026 10:31
i-kosarev
requested review from
a team,
amd-arozanov and
i-kosarev
and
a balanced review from Copilot
October 15, 2026 10:31
i-kosarev
added a commit
that referenced
this pull request
Oct 15, 2026
…nel) for ROCM-31938 validation Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/CHANGELOG.md
amd-arozanov
approved these changes
Oct 15, 2026
amd-arozanov
left a comment
amd-arozanov
left a comment
Contributor
There was a problem hiding this comment.
Please update the PR description before merge: it still says rcclP2pTaskParts returns 1, lists postTuneP2pRegisterBuffer among the uses (reverted), and says there is no unit test. The 3-node result from #13528 would also be good to have here.
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/CHANGELOG.md:59(Low) CHANGELOG Scatter entry says 15 to 30% from 16 MB, but the PR's own data shows 15 to 21% from 32 MBprojects/rccl/src/enqueue/enqueue.cc:1455(Low) addP2pToPlan part skip (part >= taskParts[dir]) has no test; reverting it passes every testprojects/rccl/test/host/enqueue-test.cc:371(Low) P2pTaskParts case table never has the pool fit exceed perPeer, so dropping std::min in rcclP2pTaskParts still passesprojects/rccl/test/host/task-posttuning-test.cc:1403(Nit) Scatter transition test never checks that the second, full-part call requests a preconnect (needPreconnect still true)projects/rccl/src/enqueue/enqueue.cc:3884(Nit) Wrapped if/for bodies without braces at enqueue.cc:3883, enqueue.cc:1392 and task_posttuning.cc:168, which the RCCL guide requires braced
Bottom line: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
wenkaidu
reviewed
Oct 15, 2026
| // on values both ends of a pair share, or they disagree on the parts. | ||
| int rcclP2pTaskParts(struct ncclComm* comm, ncclFunc_t collAPI) { | ||
| if (collAPI == ncclFuncScatter && comm->nNodes > 1 && comm->config.nChannelsPerNetPeer == NCCL_CONFIG_UNDEF_INT && | ||
| rcclUseAinic()) { |
Contributor
There was a problem hiding this comment.
is this only impact AINIC?
Contributor
Author
There was a problem hiding this comment.
Yes, we enabled this only for AINIC, as the regression was observed exclusively on AINIC nodes. We want to avoid introducing unnecessary changes to other hardware configurations.
wenkaidu
approved these changes
Oct 15, 2026
wenkaidu
left a comment
wenkaidu
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Is it necessary to limit fix to AINIC only?
nikitaxgusev
force-pushed
the
users/nikitaxgusev/rocm-31938-ainic-scatter-single-channel
branch
from
October 16, 2026 09:43
8efe3b0 to
5d26669
Compare
Contributor
Author
|
CI is failing due to infra issue: Error: File was unable to be removed Error: EBUSY: resource busy or locked, unlink '/shared_inference/rccl-ci/ghr/oci-runner-1/_work/rocm-systems/rocm-systems/.nfs000000000f3c943b00003dbc |
…er peer 242e0a9 raised p2pnChannelsPerPeer to 2 on AINIC up to 8 nodes so one peer can saturate a NIC. Scatter already drives all nRanks-1 peers from one root and lost 15-30% of bandwidth from 8 MB up (ROCM-31938). Scatter now uses and connects only the first part per peer when the peers cannot each own p2pnChannelsPerPeer channels. The decision depends only on collAPI and comm-wide sizes, so both ends of a pair agree. Other collectives and point-to-point keep two channels; for them the code path is unchanged. Limiting only the channel count is not enough: when scatter itself connects the second part, it stays at or below the two-channel speed in most runs. Measured on 4 MI355X nodes (32 ranks), AINIC: - scatter 32M-16G: dev/revert 1.16-1.25, fix/revert 1.00 - scatter after alltoall on the same comm: fix stays at revert level - all other collectives up to 2G: unchanged, 0 wrong results
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
force-pushed
the
users/nikitaxgusev/rocm-31938-ainic-scatter-single-channel
branch
from
October 16, 2026 19:35
5d26669 to
02f846f
Compare
Contributor
|
Requesting an override of |
arvindcheru
deleted the
users/nikitaxgusev/rocm-31938-ainic-scatter-single-channel
branch
October 16, 2026 23:24
Contributor
TheRock Submodule Bump ActivityNewest first
|
1 task
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
Commit 242e0a9 (#11635) raised
p2pnChannelsPerPeerfrom 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, butncclScatterregressed by 15 to 21% from 32 MB upward on multi-node AINIC jobs compared with 7.14. Scatter already drives allnRanks - 1peers 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
rcclP2pTaskParts(comm, collAPI)returns how many of thep2pnChannelsPerPeerparts a P2P task uses and connects. ForncclFuncScatteron multi-node AINIC it returns the largest power of two not abovep2pnChannels / (nRanks - 1), capped atp2pnChannelsPerPeerand at least 1, so the root's flows still fit the P2P channel pool. A user-setNCCL_NCHANNELS_PER_NET_PEERis kept, as inncclTopoComputeP2pChannels. Otherwise it returnsp2pnChannelsPerPeer.collAPIand comm-wide sizes, so sender and receiver always agree on the number of parts.p2pnChannelsPerPeerback to 2 on multi-node (fix(rccl): fixed default behavior on AINIC #11635), and the only one measured. On other NICs the halving loop inncclTopoComputeP2pChannelsalready reduces the parts per peer when the peers do not fit the pool.rcclP2pPolicyChannels), also with P2P disabled, since the limit is about network flows;addP2pToPlan;p2pTaskAppend,postTuneP2pRecordPreconnect).postTuneP2pRegisterBuffer) keepsp2pnChannelsPerPeer: Scatter tasks haveallowUBoff and never reach it.sendSeen/recvSeenstay clear, so a later task to the same peer that uses all parts still connects the rest.p2pnChannelsPerPeer, which equalsnChannelsMaxinaddP2pToPlan, so their code path takes the same decisions as before.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=1there is no regression, because the root is limited by its own NIC.Issue Tracking
JIRA ID : ROCM-31938
Test Plan
NCCL_NET=ROCM-IB,RCCL_AINIC_ROCE=1), PXN on.-c 1),-n 20 -w 5, 5 cycles per run.p2pnChannelsPerPeerraise from 242e0a9 (matches 7.14);NCCL_PXN_DISABLE=1and on 2 nodes.EnqueueMicrotest.*andTaskPostTuningMicrotest.*.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.
Scatter by node count, root 0, time ratio vs develop over 16M–1G (from #13528, merged here):
NCCL_PXN_DISABLE=1: all builds at about 64 GB/s at 256 MB, no regression.#wrong = 0and no out-of-bounds values in every run.EnqueueMicrotest.*andTaskPostTuningMicrotest.*pass.Submission Checklist