Repository navigation
fix(rccl): latch GIN only after a successful connect - #14321
Merged
Merged
Conversation
ncclDevrCommCreateInternal set devr->ginEnabled before ncclGinConnectOnce and never cleared it. A failed connect left ginCommCount at 0, so a retry skipped the reconnect and divided by zero in ginDevCommSetupWithBackend. Set the latch only once connect succeeds, so the next create reconnects. AICOMRCCL-2879 Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
…est fast The suggestion apply left both comment lines in place, and EXPECT_FALSE let a latched ginEnabled fall through into a host-test divide by zero. Co-authored-by: Cursor <cursoragent@cursor.com>
Upstream sets ginEnabled before ncclGinConnectOnce. Tag the comment RCCL: so a future NCCL sync keeps the latch after connect. AICOMRCCL-2879 Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Oct 27, 2026
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Kapil-Shyam-Pawar
approved these changes
Oct 27, 2026
Kapil-Shyam-Pawar
left a comment
Kapil-Shyam-Pawar
left a comment
Contributor
There was a problem hiding this comment.
lgtm
#13231 was reviewed and approved
just switching from fork here
1 task done
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
pr-rocm-systems-14321-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14321-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
abouteiller
approved these changes
Oct 27, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
dlamd1dai
added a commit
that referenced
this pull request
Oct 30, 2026
## Motivation Replaces #14231 with the same commits on an upstream branch, so the RCCL coco PR gate runs without a fork override. The earlier review is on #14231. Follow-up to #14321 (merged), raised by Argus in its review of #13231. #14321 sets `devr->ginEnabled` only after `ncclGinConnectOnce` succeeds. After that, `ncclDevrCommCreateInternal` still registers every existing memory with GIN and then updates every existing window's GIN handles. If anything fails in that stretch (a `symMemoryRegisterGin` call, `ncclGinDevCommSetup`, stream creation, or `symWindowInitGin`), GIN stays latched: - The memory that failed to register has its `ginSegmentInfos` freed, and memories later in the list were never registered. - Windows never got their GIN handles, because the window update runs after `ncclGinDevCommSetup`. - The next create computes `ginActivated` false, skips both the registration and the window update, and still builds a full GIN devcomm. A later `ncclCommWindowRegister` on one of those memories then indexes the `ginSegmentInfos` that was freed. ## Technical Details - `ncclDevrCommCreateInternal` treats connect, registering the existing memories, and updating the existing windows as one activation. It records completion in `ginActivationDone`, set once the window update loop finishes. - On any failure before that point: - `symGinActivationRollbackWindows` (on the `fail_stream` path, while the stream is still alive) frees the segment windows attached to existing windows, clears their GIN handles, and copies the cleared window to the device. - `symGinActivationRollbackMemories` (on the `fail` path) deregisters every memory holding `ginSegmentInfos`, frees them, resets `numGinSegments` to 1 as for memory obtained before GIN, and clears `ginEnabled`. - When the window rollback runs, `fail_stream` synchronizes the stream before destroying it. A retry rebuilds those windows on a new stream, which is not ordered after this one. - Rolling back everything GIN holds is safe because nothing had GIN state before this activation. `ginActivated` is only true while `ginEnabled` was false, and `ginSegmentInfos` and a window's `ginMultiSegmentWins` are only set once GIN is enabled. - `ncclGinConnectOnce` keeps `connected` true, so the retry does not reconnect. It re-registers the memories and updates the windows again. - Failures after the window update are unchanged. GIN is then fully activated for every existing memory and window, and keeping it latched is correct. - As with the connect retry in #14321, the rollback assumes the failure happened on every rank. ## Issue Tracking JIRA ID: AICOMRCCL-2879 ## Test Plan - New `DevrCommCreateInternalTest.GinRegisterFailsAfterConnect_RollsBackActivation`: connect succeeds, and two GIN-capable memories are on `memHead`. The first registers and the second fails. The test checks: - `ginEnabled` is already true while registration runs. - After the failure, the first memory is deregistered, neither memory keeps `ginSegmentInfos`, and `ginEnabled` is false. - A second create connects and registers again, then rolls back the same way. - New `DevrCommCreateInternalTest.StreamCreateFailsAfterGinRegister_RollsBackActivation`: connect succeeds and both memories register, then stream creation fails. The test checks that both memories are deregistered, neither keeps `ginSegmentInfos` or `ginHostWins`, and `ginEnabled` is false. - Full host-test pipeline (`run_host_tests.sh` deps through run) in `rocm/dev-ubuntu-22.04:7.2`. ## Test Result - On `develop` at `060a14eb23`: `rccl-UnitTestsMicro` 2232 of 2232 passed, and every other host binary passed. - With `ginActivationDone = true;` moved right after the register loop, `StreamCreateFailsAfterGinRegister_RollsBackActivation` fails (no deregistrations, `ginEnabled` still true), while `GinRegisterFailsAfterConnect_RollsBackActivation` still passes. - Before the rebase, with the `dev_runtime.cc` rollback reverted, `GinRegisterFailsAfterConnect_RollsBackActivation` failed on the `ginEnabled` check after the first create. With `devr->ginEnabled = true;` removed, it failed on the check inside the register hook. - Not covered by host tests: the window rollback, which needs a window in `winSorted` that this fixture does not build, and a failure inside `ncclGinDevCommSetup`, which the fixture fakes as always succeeding. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Made with [Cursor](https://cursor.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
Replaces #13231 with the same commits on an upstream branch, so the RCCL coco PR gate runs without a fork override. The earlier review, including the Argus threads and Aurelien's and Kapil's approvals, is on #13231.
Found while reviewing #13228. If
ncclGinConnectOncefails, the nextncclDevrCommCreateInternalon the same communicator can divide by zero inginDevCommSetupWithBackend.ncclDevrCommCreateInternalsetdevr->ginEnabled = trueas soon as GIN was requested, before trying to connect, and no failure path cleared it. A connect that fails beforebackends[].ginCommCountis assigned leaves that count at 0, as it was at plugin registration. On retry,ginEnabledis already true, so connect is skipped. Setup then runsROUNDUP(nContextsTotal, backend->ginCommCount)with a count of 0.Two argument checks after the latch (a world GIN barrier on a rail connection, and a custom stride of 0) returned with GIN already latched. That left the communicator in the same state.
Technical Details
ncclDevrCommCreateInternalnow setsdevr->ginEnabledonly afterncclGinConnectOncesucceeds. A failed connect, or an early argument rejection, leaves GIN unlatched, so the next create connects again.ncclGinConnectOncealready closes partial connections and leavesconnectedfalse on failure, so calling it again is safe.RCCL:and notes that upstream latches before connect, so a future NCCL sync keeps it after connect.ginDevCommSetupWithBackendis unchanged. After this change, aginCommCountof 0 means connect never completed, and clamping it there would hide that.dev_runtime_micro_fakesgets ag_devrGinConnectOncehook, modeled ong_devrRmaProxyConnectOnce, so a test can make connect fail.Not changed: if connect succeeds and a later step fails (GIN registration of existing memory,
ncclGinDevCommSetup, or the window GIN-handle update), GIN stays latched as before. #14231 handles that case.Issue Tracking
JIRA ID: AICOMRCCL-2879
Test Plan
DevrCommCreateInternalTest.GinConnectFails_RetryConnectsAgain: connect fails twice. The test checks thatginEnabledstays false and that the second create calls connect again.DevrCommCreateInternalTest.WorldGinBarrierOnRail_RejectedWithoutLatchingGin: the early rejection does not call connect and does not latch GIN.run_host_tests.shinrocm/dev-ubuntu-22.04:7.2, after rebasing ontodevelop.Test Result
develop'sGinSignalRegions_UseLargestRoundedContextCount.dev_runtime.ccchange reverted,GinConnectFails_RetryConnectsAgainfails on theginEnabledcheck.Submission Checklist