Repository navigation
fix(rccl): Fix DDA IPC hang - #13487
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No undocumented RCCL or NCCL environment-variable changes. |
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
|
@cppccppccppc @sunguobao PTAL |
![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
1 more finding(s), not posted inline
projects/rccl/test/host/dda-ipc-lifecycle-test.cc:524(Low) No test covers the abort/revoke path that is meant to skip the teardown barriers and still close mappings locally
Bottom line: Worth settling before approval: ncclDdaIpcCommFini now returns errors into NCCLCHECK at init.cc:584, so a failed barrier or close skips the rest of commFree.
Comment @argus review to re-run, or @argus re-review after pushing.
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the new bootstrapBarrier in ncclDdaIpcCommFini runs from commFree, which commReclaim calls serially per process, so destroying an 8-rank comm with several GPUs per process deadlocks.
Comment @argus review to re-run, or @argus re-review after pushing.
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: gating DDA IPC init on RCCL_DDA_ENABLE also leaves ddaScratch null, which silently turns off the CE-scratch AllGather/AlltoAll path on gfx942/gfx950 when DDA is disabled.
Comment @argus review to re-run, or @argus re-review after pushing.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: commDestroySync unmaps DDA peer IPC memory without waiting for DDA kernels still running on the user stream (init.cc:4436, same at 4637).
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
Author
|
@argus re-review |
Contributor
pr-rocm-systems-13487-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13487-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
…_ipc_hang_fix # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/fakes/bootstrap_stubs.cc
Contributor
pr-rocm-systems-13487-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13487-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
…_ipc_hang_fix # Conflicts: # projects/rccl/test/host/run_host_tests.sh
Contributor
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the doneEvent wait at ipc_init.cu:220 does not cover CE-scratch AllGather/AlltoAll copies into peer scratch, and two lifecycle tests still pass with the fixes they cover reverted.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: a CE launch that fails after queuing peer copies skips the new doneEvent record (ce_coll.cc:3897), so teardown can unmap DDA scratch under them; two lifecycle tests also still pass with their fixes reverted.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-13487-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13487-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
|
@PJAvinash - I assume you have verified there is not perf impact from this PR. I do not expect one since it is happening during finalize, but always good to check. |
nusislam
approved these changes
Nov 3, 2026
Contributor
Author
|
Contributor
pr-rocm-systems-13487-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13487-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
Author
|
Precheckin CI failure is unrelated. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
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
Repeated creation and destruction of 8-rank, one-process-per-GPU communicators can hang during DDA IPC initialization.
The blocked ranks remain inside:
hipIpcOpenMemHandle -> hsa_amd_ipc_memory_attach -> IPCRecvHandleThis was reproduced when one collective test completed and a subsequent test created another MP-8 communicator.
Technical Details
DDA IPC teardown could free a rank's exported scratch or barrier allocation while peer ranks still retained imported IPC mappings to that allocation.
commReclaimrunscommFreeserially per local rank with no synchronization between ranks, so one rank's free could precede a peer's mapping close.The IPC handler also only closed mappings after a completely successful exchange. If
hipIpcOpenMemHandlefailed partway through an exchange, mappings opened before the failure were not closed.This left the HIP/ROCr IPC lifecycle improperly ordered and could cause a later IPC import to hang.
Changes
commDestroySync, which runs concurrently for every local rank, before its existing intranode barrier.commReclaim's revoked-communicator stage, for every local rank, before the serialcommCleanuploop. Revoke stays non-collective.comm->doneEventso DDA kernels still running on the user stream cannot touch an unmapped peer address. A failed wait logs a warning and cleanup continues.commFree/ncclDdaIpcCommFinistay non-collective, so destroying a communicator whose ranks share a process cannot deadlock.ncclDdaIpcCommFiniis best-effort, closes any remaining mappings itself (init-failure and other directcommFreepaths), and always completes the remaining cleanup.rccl-UnitTestsMicroDdaIpcLifecycle) torun_host_tests.sh.Known limit: with one process per GPU, revoke has no cross-process synchronization by design, so the revoke ordering covers ranks within a process.
[internal-env][no-behavior-change]
Issue Tracking
JIRA ID : AICOMRCCL-3064
Test Plan
Added host-only DDA IPC lifecycle tests (
rccl-UnitTestsMicroDdaIpcLifecycle) covering:commFree, no allocation freed while still mapped.doneEventbefore the first unmap, and a failed wait does not stop cleanup.ncclDdaIpcCommFiniwithout a prior close (init-failure path) stays local and releases every mapping and allocation.hipIpcOpenMemHandlefailure cleanup and idempotent close.RCCL_DDA_ENABLE=0still allocates DDA scratch (required by CE-scratch AllGather/AlltoAll).Added init tests covering:
commDestroySyncordering (close mappings -> intranode barrier -> proxy stop), including aborted comms.commReclaimclosing all local ranks' mappings before serial cleanup, for normal and revoked communicators.Test Result
rccl-UnitTestsMicro*target): 80 run, 79 pass. The only failing test,EnqueueMicrotest.TopoGetAlgoInfo_ThreePivotBiRings_PicksTreeLlUpToTheCutoffThenRingSimple, also fails on develop (from test(rccl): cover enqueue.cc rearch, validation, topology and NVLS paths in microtests #14529). Develop's newrccl-UnitTestsMicroHostDispatch(test(rccl): add rccl-UnitTestsHostAll, one front door for the host tests #14121) does not compile with ROCm 7.2.1 (nlohmann/json.hppconversion error); this PR does not touch it.--gtest_repeat=5: 5/5 iterations passed (no hang or fault).RCCL_DDA_ENABLE=0, AllGather and AlltoAll: passed;ddaScratchallocated on all ranks.--gtest_repeat=3: 3/3 passed.doneEventtests were each confirmed to fail with their fix removed.Submission Checklist