Skip to content

fix(rccl): default gfx120x single-node collectives to Simple protocol - #13151

Closed
jinchen62 wants to merge 8 commits into
developfrom
users/jinchen62/ROCM-29721-gfx12-rccl-deadlock
Closed

jinchen62 wants to merge 8 commits into
developfrom
users/jinchen62/ROCM-29721-gfx12-rccl-deadlock

Conversation

@jinchen62

@jinchen62 jinchen62 commented Oct 8, 2026 •

Copy link
Copy Markdown

JIRA ID : ROCM-32656

Motivation

PyTorch FSDP can hang in RCCL collectives on multi-GPU gfx1201 hosts. On a 4x gfx1201 host running a recent ROCm 10.2 nightly, TestStateDict.test_fsdp (test/distributed/checkpoint/test_state_dict.py) timed out after 300 s during distributed backward in 3 of 3 runs. #5480 reports the same signature (all GPUs spinning at 100%, no progress) for vLLM TP=2 on dual gfx1201, and NCCL_PROTO=Simple is the confirmed workaround.

Technical Details

  • rcclUpdateCollectiveProtocol() sends single-node gfx1200/gfx1201 collectives through rcclGetProtoForGfx120x(), which picks LL for small messages (per rank: Broadcast <= 1.5 KiB, Reduce <= 8 KiB, AllGather/ReduceScatter <= 96 KiB, AllReduce <= 16 KiB). The only existing guard forces Simple when NCCL_P2P_DISABLE=1 is set, so both P2P/IPC paths and topology-routed SHM paths (for example GPUs without direct P2P, or NCCL_P2P_LEVEL=PIX) still get LL.
  • On gfx1200/gfx1201, the LL FIFO poll loops in prims_ll.h (readLL, readLLBeginAll, readLLFinish) use plain non-temporal loads. They have neither the acquire ordering that Simple uses on gfx120x (loadStepValue in prims_simple.h), nor the cache-bypassing load gfx11 uses, nor the system-scope load gfx1250 uses.
  • This PR selects Simple for automatic single-node gfx120x collectives. It is the same stopgap proposed in Force Simple protocol for gfx12 single-node collectives to fix deadlock rccl#2187, which was not merged before RCCL moved to rocm-systems.
  • Unchanged: the NCCL_PROTO override, multi-node gfx120x behavior (including the NCCL_P2P_DISABLE override), tuner-plugin and PAT selections (they skip this function), send/recv protocol selection, and all other architectures.
  • rcclGetProtoForGfx120x() and its cutoff table are kept, and still unit-tested, so LL can be re-enabled once the prims_ll.h loads are fixed for gfx120x.
  • Cost: small-message latency on single-node gfx120x matches NCCL_PROTO=Simple. Not benchmarked in this PR.
  • Adds a Resolved issues entry to projects/rccl/CHANGELOG.md.

Issue Tracking

GitHub issue: #5480
JIRA ID: ROCM-32656

Test Plan

  • Host microtests (rccl-UnitTestsMicro):
    • New UpdateCollectiveProtocol_Gfx120xSingleNodeDefaultsSimple: gfx1200 and gfx1201 AllGather, ReduceScatter and AllReduce at sizes the old selector mapped to LL now select Simple.
    • UpdateCollectiveProtocol_Gfx120xMultiNodeP2pDisableForcesSimple (was Gfx120xDelegatesThenP2pDisableForcesSimple) now runs multi-node, the only case where the NCCL_P2P_DISABLE override still applies.
    • Mutation check: restoring the rcclGetProtoForGfx120x() call fails the single-node test, and disabling the NCCL_P2P_DISABLE override fails the multi-node test.
  • Runtime on 2x gfx1201, with the patched librccl.so dropped into a public PyTorch/ROCm nightly environment:
    • AllGather/ReduceScatter repro (40 KB per rank, 10 iterations) in four configurations: default, NCCL_P2P_LEVEL=PIX (SHM), NCCL_P2P_DISABLE=1, and 4 ranks on 2 GPUs with NCCL_MULTI_RANK_GPU_ENABLE=1 (mixed P2P/IPC and SHM).
    • NCCL_PROTO=LL override check.
    • TestStateDict.test_fsdp: one run with NCCL_DEBUG=INFO NCCL_DEBUG_SUBSYS=COLL,TUNING, plus 5 plain runs.

Test Result

  • Host microtests: 327/327 wrap tests and 1,562/1,562 total pass.
  • Collective repro: all configurations complete and select Simple; NCCL_PROTO=LL still selects LL.
  • FSDP: unpatched RCCL selects LL for all 2,107 collectives in test_fsdp (4 B to 60.6 KB per rank). With this patch none select LL, and the test passes 6/6.
  • The 4x gfx1201 timeout did not reproduce on the 2-GPU test host, where unpatched RCCL also passes. Final confirmation is pending on the affected host: running the failing build with NCCL_PROTO=Simple has the same effect on collectives as this patch.

Submission Checklist

jinchen62 and others added 2 commits October 8, 2026 16:52
LL can deadlock single-node gfx1200/gfx1201 collectives: the LL FIFO polls
in prims_ll.h use non-temporal loads without the acquire ordering that the
Simple protocol uses on gfx120x. Automatic selection still picked LL for
small Broadcast/Reduce/AllGather/ReduceScatter/AllReduce calls unless
NCCL_P2P_DISABLE=1 was set; every collective in PyTorch's FSDP state-dict
test (4 B to 60.6 KB per rank) selected LL on gfx1201.

Select Simple for automatic single-node gfx120x collectives. NCCL_PROTO
still overrides the choice, and multi-node behavior, including the
NCCL_P2P_DISABLE override, is unchanged.

JIRA ID: ROCM-29721
Refs: #5480

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@jinchen62
jinchen62 requested review from a team and a balanced review from Copilot October 8, 2026 21:09
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rccl/src/rccl_wrap.cc, projects/rccl/test/host/wrap-test.cc; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The rearchitected enqueue path bypasses this override and can still automatically select unsafe LL protocols.

1 open finding
What changed in this PR

Defaults single-node gfx120x collectives to Simple protocol to avoid LL deadlocks.

Changes:

  • Forces Simple for automatic single-node gfx120x selection.
  • Updates protocol-selection microtests.
  • Documents the fix in the changelog.
File Description
projects/​rccl/​src/​rccl_wrap.cc Applies the Simple protocol fallback.
projects/​rccl/​test/​host/​wrap-test.cc Tests single- and multi-node gfx120x behavior.
projects/​rccl/​CHANGELOG.md Records the resolved hang.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread projects/rccl/src/rccl_wrap.cc Resolved

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: Looks approval ready; the three findings are test and comment hygiene, not correctness.

Comment @argus review to re-run, or @argus re-review after pushing.

Comment thread projects/rccl/test/host/wrap-test.cc Resolved
Comment thread projects/rccl/src/rccl_wrap.cc Resolved
Comment thread projects/rccl/test/host/wrap-test.cc Resolved
jinchen62 and others added 2 commits October 12, 2026 01:40
…CM-29721-gfx12-rccl-deadlock

Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	projects/rccl/CHANGELOG.md
Add UpdateCollectiveProtocol_Gfx120xUserOverrideKeepsLL so NCCL_PROTO=LL
must survive on single-node gfx1200/gfx1201; the gfx950 override test
could not catch the gfx120x arm ignoring NCCL_PROTO.

Use a struct of cases in Gfx120xSingleNodeDefaultsSimple instead of
parallel arrays, and stop calling gfx120x LL functional in the
NCCL_P2P_DISABLE comment, which now only matters for multi-node.

JIRA ID: ROCM-29721

Co-authored-by: Cursor <cursoragent@cursor.com>
@jinchen62

Copy link
Copy Markdown
Author

@argus re-review

@amd-argus

amd-argus Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

@jinchen62 I can't trigger on this PR yet -- your association here reads as NONE, and the current policy is members. Ask a maintainer to add jinchen62 to ARGUS's allowed-users list.

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Findings

1 finding(s) that could not be posted as inline comments
  • projects/rccl/src/rccl_wrap.cc:124 (Low) rcclGetProtoForGfx120x has no production caller left after this change; only the microtests reach it rcclGetProtoForGfx120x at rccl_wrap.cc:124 has no production caller after this change: the single call site was the one replaced at rccl_wrap.cc:186, and the only remaining references are the microtests at wrap-test.cc:179-223. Its sibling rcclGetProtoForGfx110x is still live at rccl_wrap.cc:196, so the two read as symmetric but are not. Since the plan is to keep the table until the prims_ll.h loads are fixed, could you put that note on the function itself, so the next reader can tell it is intentionally dormant rather than in use?

Bottom line: Looks approval ready. All three prior findings are fixed; what is left is a follow-up question on whether multi-node gfx120x was deliberately left on LL.

Comment @argus review to re-run, or @argus re-review after pushing.

Comment thread projects/rccl/src/rccl_wrap.cc Resolved
Comment thread projects/rccl/test/host/wrap-test.cc Resolved
Add Broadcast and Reduce cases at 4 B per rank to
Gfx120xSingleNodeDefaultsSimple, so routing only the small-cutoff funcs
back to rcclGetProtoForGfx120x() now fails the test.

Note on rcclGetProtoForGfx120x() that it has no production caller while
single-node gfx120x defaults to Simple, and is kept to re-enable LL once
the prims_ll.h loads are fixed.

JIRA ID: ROCM-32656

Co-authored-by: Cursor <cursoragent@cursor.com>
@jinchen62

Copy link
Copy Markdown
Author

Re the rcclGetProtoForGfx120x() finding: added a note on the function saying it has no production caller while single-node gfx120x defaults to Simple, and is kept to re-enable LL once the prims_ll.h loads are fixed.

@jinchen62

Copy link
Copy Markdown
Author

@argus re-review

@amd-argus

amd-argus Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@jinchen62 I can't trigger on this PR yet -- your association here reads as NONE, and the current policy is members. Ask a maintainer to add jinchen62 to ARGUS's allowed-users list.

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

Comment @argus review to re-run, or @argus re-review after pushing.

@alex-breslow-amd alex-breslow-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test the 10.2 RCCL? This is likely fixed in 10.2 after #10670.

@jinchen62

Copy link
Copy Markdown
Author

@alex-breslow-amd The failure is already on a 10.2 nightly: Bilal's runs used rocm10.2.0a20261008 on a 4x gfx1201 host, built 13 days after #10670 merged on Sep 25, and test_fsdp timed out in backward in 3 of 3 runs with the default protocol (LL).

My 2x gfx1201 host can't reproduce the hang on any build, so it can't settle this. Current develop (with #10670) and NCCL_PROTO=LL passes test_fsdp 4/4 with every collective on LL, but the Aug 22 build without #10670 also passed there. The deciding run is the 4-GPU A/B on that same image, default vs NCCL_PROTO=Simple, which is pending.

From the code, I don't expect #10670 to cover this: on gfx1200/gfx1201, readLL, readLLBeginAll and readLLFinish still poll with loadLLLine, a plain non-temporal 128-bit load with no acquire or system scope, while Simple uses an acquire load in loadStepValue. #10670 changed the access width, not the ordering.

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

Comment @argus review to re-run, or @argus re-review after pushing.

@xeacottx

xeacottx commented Oct 14, 2026 •

Copy link
Copy Markdown

also watching this. just ran into the same thing, and your flag worked instantly. removed the deadlock.

/edit - my setup is 9600s, but same architecture id. running rocm 10.1/7.2.4 for A/B testing, qwen 3.8 with 4 GPUs. NCCL_PROTO=Simple is exactly what moved the needle.

what other flags are passable? curious. especially for GPU-GPU comms to experiment with latency differences

@jinchen62

Copy link
Copy Markdown
Author

@xeacottx Thanks, that's a useful data point: another 4-GPU gfx120x setup hitting the same deadlock, fixed by NCCL_PROTO=Simple.

Could you post the RCCL version line that NCCL_DEBUG=VERSION prints on your 10.1 and 7.2.4 setups? Both should predate #10670 (merged Sep 25), which a reviewer thinks may already fix this in 10.2. If you're able to build RCCL from this repo's develop branch and run once without NCCL_PROTO, that would answer the question directly.

On other settings: the environment variables are documented in env.rst, with tuning notes in performance_and_tuning.rst. For GPU-to-GPU latency, the main knobs are NCCL_ALGO (Ring or Tree), NCCL_MIN_NCHANNELS / NCCL_MAX_NCHANNELS, NCCL_P2P_LEVEL / NCCL_P2P_DISABLE (P2P versus shared-memory paths), and NCCL_BUFFSIZE. NCCL_DEBUG=INFO NCCL_DEBUG_SUBSYS=TUNING,COLL shows what RCCL picks for each collective, and rccl-tests (all_reduce_perf and friends) is the usual way to measure. On these GPUs, keep LL out of NCCL_PROTO, since LL is what deadlocks, and don't force LL128: it isn't built for gfx120x, and the docs warn that forcing it where unsupported can corrupt data.

@xeacottx

Copy link
Copy Markdown

thank you for getting back to me @jinchen62 let me give this a go tomorrow or friday. but with 10.1, the version that loaded was... i think 2.30.7. prior to that 7.2.4 i was using 2.27.0 and didn't experience any weirdness

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

Comment @argus review to re-run, or @argus re-review after pushing.

@jinchen62

Copy link
Copy Markdown
Author

Closing this for now. Bilal's results on the 4x gfx1201 host showed the test_fsdp timeout wasn't RCCL: it came from slow small fp32 GEMMs in hipBLASLt (fix in ROCm/rocm-libraries#12465), and NCCL_PROTO=Simple didn't help. With the hipBLASLt fixes, the same 10.2 image passed 10 of 10 with default RCCL settings.

That leaves no 10.2 reproducer for an LL hang on gfx120x. One note on #10670: before it, gfx1201 polled with two 64-bit non-temporal loads, but the 10.1.0a20260822 nightly (Clang 23) already fused them into a single global_load_b128 with th:TH_LOAD_NT, the code #10670 produces. So on that build #10670 doesn't change gfx1201; it matters if a Clang 24 build splits the load, as described for gfx950.

@jinchen62 jinchen62 closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants