Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: The latch move itself is correct. Worth settling before approval: the comment added at dev_runtime.cc:1930 overstates what a failed connect leaves behind, and the test comment at dev-runtime-test.cc:5585 repeats it.
Comment @argus review to re-run, or @argus re-review after pushing.
abouteiller
approved these changes
Oct 23, 2026
1 task done
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>
Kapil-Shyam-Pawar
approved these changes
Oct 26, 2026
dlamd1dai
force-pushed
the
users/dondai/rccl-gin-connect-retry
branch
from
October 26, 2026 19:02
5527192 to
632498e
Compare
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>
1 task done
Contributor
Author
|
Closing in favor of #14321. It has the same commits (head f6f5cb8) on an upstream branch, so the RCCL coco PR gate can run without a fork override. @abouteiller @Kapil-Shyam-Pawar, could you approve #14321 again? Nothing changed since your reviews except the rebase onto develop and the |
1 task done
dlamd1dai
added a commit
that referenced
this pull request
Oct 27, 2026
## 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 `ncclGinConnectOnce` fails, the next `ncclDevrCommCreateInternal` on the same communicator can divide by zero in `ginDevCommSetupWithBackend`. `ncclDevrCommCreateInternal` set `devr->ginEnabled = true` as soon as GIN was requested, before trying to connect, and no failure path cleared it. A connect that fails before `backends[].ginCommCount` is assigned leaves that count at 0, as it was at plugin registration. On retry, `ginEnabled` is already true, so connect is skipped. Setup then runs `ROUNDUP(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 - `ncclDevrCommCreateInternal` now sets `devr->ginEnabled` only after `ncclGinConnectOnce` succeeds. A failed connect, or an early argument rejection, leaves GIN unlatched, so the next create connects again. `ncclGinConnectOnce` already closes partial connections and leaves `connected` false on failure, so calling it again is safe. - The comment on the latch starts with `RCCL:` and notes that upstream latches before connect, so a future NCCL sync keeps it after connect. - `ginDevCommSetupWithBackend` is unchanged. After this change, a `ginCommCount` of 0 means connect never completed, and clamping it there would hide that. - Test fakes: `dev_runtime_micro_fakes` gets a `g_devrGinConnectOnce` hook, modeled on `g_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 - New `DevrCommCreateInternalTest.GinConnectFails_RetryConnectsAgain`: connect fails twice. The test checks that `ginEnabled` stays false and that the second create calls connect again. - New `DevrCommCreateInternalTest.WorldGinBarrierOnRail_RejectedWithoutLatchingGin`: the early rejection does not call connect and does not latch GIN. - Full host-test suite, built host-only with `run_host_tests.sh` in `rocm/dev-ubuntu-22.04:7.2`, after rebasing onto `develop`. ## Test Result - After the rebase: 7909 of 7909 host tests passed across 32 binaries, including both new tests and `develop`'s `GinSignalRegions_UseLargestRoundedContextCount`. - With the `dev_runtime.cc` change reverted, `GinConnectFails_RetryConnectsAgain` fails on the `ginEnabled` check. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Aurelien Bouteiller <aurelien.bouteiller@amd.com> Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
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
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.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.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.rccl-UnitTestsMicro, built host-only withrun_host_tests.shinrocm/dev-ubuntu-22.04:7.2.Test Result
rccl-UnitTestsMicro: 1663 of 1663 passed, including both new tests.dev_runtime.ccchange reverted,GinConnectFails_RetryConnectsAgainfails on theginEnabledcheck.Submission Checklist