Repository navigation
[RCCL] Fix p2p channel invariant: ensure p2pnChannels >= p2pnChannelsPerPeer - #6307
Merged
Merged
Conversation
…PerPeer Without this fix, arch-specific caps (e.g. gfx1250 per-peer doubling) could set p2pnChannelsPerPeer higher than p2pnChannels. The device-side helper ncclP2pChannelToPart silently aliases part indices >= nP2pChannels onto lower ones, producing wrong data in alltoall on MI455 2x1p1g topologies. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
mustafabar
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness issue in RCCL’s P2P channel selection by enforcing an invariant between the total P2P channel count (p2pnChannels) and the per-peer partitioning (p2pnChannelsPerPeer), preventing device-side channel→part inverse mapping from aliasing higher parts onto lower ones.
Changes:
- Ensures
comm->p2pnChannelsis rounded/clamped to be at leastcomm->p2pnChannelsPerPeer(both pow2-rounded), within the existing4*CHANNEL_LIMITcap. - Adds a final safety clamp to guarantee
p2pnChannelsPerPeer <= p2pnChannelsafter architecture-specific caps may reducep2pnChannels.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
LGTM. tested with alltoall on 2x1p1g, 2x1p4g, 3x1p4g, 4x1p4g. |
nileshnegi
approved these changes
May 28, 2026
Contributor
|
LGTM |
isaki001
approved these changes
May 28, 2026
sudhu2k
added a commit
to ROCm/Megatron-LM
that referenced
this pull request
Oct 27, 2026
… cold-rank desync Skip the suite-wide NCCL_MAX_NCHANNELS=1 default on ROCm. With RCCL <= 2.27.7 (ROCm 7.2.x), a low channel cap leaves fewer P2P channels than P2P parts per peer, so send/recv messages larger than ~32 MB are silently half-delivered. This corrupted context-parallel attention (test_thd_correctness) and the MoE dispatcher and custom process-group tests. Fixed upstream in ROCm/rocm-systems#6307; keep the default for CUDA. Move TestMoriColdRank out of test_token_dispatcher_mori.py into its own test_token_dispatcher_cold_rank_mori.py module so the zero-token EP rank case runs in a fresh torchrun process and a regression there cannot desynchronize the MORI op and process-group state shared by the other MORI tests. Signed-off-by: sudhu2k <Sudharshan.Govindan@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
On MI455 2x1p1g topologies, alltoall produces wrong data when topology fallback yields 2 p2p channels but the gfx1250 single-node per-peer doubling raises
p2pnChannelsPerPeerto 4. The device-side helperncclP2pChannelToPartcannot recover part indices >=nP2pChannels; higher parts silently alias onto lower ones, corrupting results.Technical Details
comm->p2pnChannels = std::min(pow2Up(...), 4*CHANNEL_LIMIT)withstd::min(std::max(pow2Up(comm->p2pnChannels), pow2Up(comm->p2pnChannelsPerPeer)), 4*CHANNEL_LIMIT)sop2pnChannelsis always rounded up to at leastp2pnChannelsPerPeer.comm->p2pnChannelsPerPeer = std::min(comm->p2pnChannelsPerPeer, comm->p2pnChannels)after all arch-specific caps to preserve the invariant even when those caps shrinkp2pnChannels.JIRA ID
AICOMRCCL-1061
Test Plan
Test Result
Pending
Submission Checklist
🤖 Generated with Claude Code