Repository navigation
Conversation
|
@Kapil-Shyam-Pawar @ROCm/rccl-leads this replaces #11089 after the stack base was deleted. Same AICOMRCCL-2241 memHead-ordering fix, rebased onto current %RCCL_TEST_CONFIG @argus review |
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/docs/contrib/GIN/NCCL_Gin_and_Symmetric_Memory/NCCL_Gin_and_Symmetric_Memory.md:246(Low) The design doc's symMemoryObtain stage list still describes the ordering this PR invertsdocs/contrib/GIN/NCCL_Gin_and_Symmetric_Memory/NCCL_Gin_and_Symmetric_Memory.md:246-247still lists Stage 8 assymMemoryRegisterGin/symMemoryRegisterRmaand Stage 9 as the link intodevr->memHead, which is exactly the order this change inverts. Swapping those two lines keeps the only in-tree description ofsymMemoryObtainin step with the code.
Bottom line: LGTM once the stage order in NCCL_Gin_and_Symmetric_Memory.md:246-247 is swapped to match the new link-before-register ordering. That one is mechanical, so it does not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
|
Follow-up
Kept @argus re-review |
|
Math-CI |
There was a problem hiding this comment.
ARGUS
Bottom line: LGTM once the GIN-register-succeeds-then-RMA-register-fails rollback gets a test; that is the ginSegmentInfos leak this PR fixes and nothing covers it. Test-only, so it does not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
…L-2241) Anvil SDMA resolves user VA via ncclDevrGetLsaSelfAddr during regMrSym, which only walks memHead. Linking after GIN/RMA register hid in-flight windows (legal after ncclDevCommCreate). Unlink and deregister on the failure path. Co-authored-by: Cursor <cursoragent@cursor.com>
…2241) Alltoall_PureReference now registers before DevCommCreate, so it no longer catches the Anvil SDMA memHead lookup. Add a GIN-enabled case that does. Co-authored-by: Cursor <cursoragent@cursor.com>
Name the AICOMRCCL-2241 case in the cluster test-runner configs so proxy device-MPI jobs report it explicitly, not only via the catch-all filter. Co-authored-by: Cursor <cursoragent@cursor.com>
…L-2241 Those gin_device_mpi jobs pin NCCL_GIN_TYPE=2 on two nodes, so they never reach Anvil SDMA memHead lookup. Coverage stays on gin_devmpi_sdma. Co-authored-by: Cursor <cursoragent@cursor.com>
…2241) Route symMemoryRegisterGin failures through NCCLCHECKGOTO with a fail_simple path that deregisters partial MRs, factor GIN/RMA teardown into symMemoryUnregister, and add a DevRuntime unit test that asserts memHead is unlinked when ncclGinRegister fails after the pre-register link. Co-authored-by: Cursor <cursoragent@cursor.com>
…CL-2241 ncclRmaProxyRegister can return after filling only a prefix of rmaHostWins; deregMrSym implementations deref the handle, so skip null slots in ncclRmaProxyDeregister. Record the Anvil SDMA window-register fix in CHANGELOG. Co-authored-by: Cursor <cursoragent@cursor.com>
…2241) gin_rocshmem_backends_mellanox.json also runs GDA device-MPI and alltoall_perf. Add gin_devmpi_sdma.json so this PR can trigger only the Anvil SDMA suite. Co-authored-by: Cursor <cursoragent@cursor.com>
…ng (AICOMRCCL-2241) fail_simple can hand a partly filled ginHostWins to ncclGinDeregister; skip null slots the way ncclRmaProxyDeregister already does. Record memHead at ncclGinRegister so the host test fails if the pre-register link moves. Keep gin_devmpi_sdma.json as a five-test Anvil SDMA subset instead of the full suite that self-skips. Co-authored-by: Cursor <cursoragent@cursor.com>
Unregister GIN/RMA before unlinking memHead on symMemoryObtain failure, null ginHostWins/ginDevWins after fail_simple deregister, and add (void) on symMemoryUnregister GIN teardown. Strengthen WindowRegister_AfterDevCommCreate with a GIN put round-trip and drop the standalone gin_devmpi_sdma.json runner. Co-authored-by: Cursor <cursoragent@cursor.com>
fail_mem_space freed the big-space offset while per-rank cuMemMap slices were still live. Share the destroy unmap helper and cover it from the GIN-register failure host test.
develop now builds segment infos before ncclGinRegister. The no-op stub left numGinSegments at 0, so the GIN-failure host tests never reached registration.
develop now supplies ras_param_fakes.cc, so the extra ras_fakes.cc is redundant. The widened GIN rollback also deregisters the part-way failing segment. Co-authored-by: Cursor <cursoragent@cursor.com>
The Anvil SDMA subset replaces the common GinMPIDeviceTests list, so the AICOMRCCL-2241 case never ran on NCCL_GIN_TYPE=7. Keep the GDA common entry and note the changelog type after the 2.31 renumber. Co-authored-by: Cursor <cursoragent@cursor.com>
Swap the design-doc stages and tighten comments from Argus/Aurelien review on AICOMRCCL-2241 so the in-tree description matches link-before-register. Co-authored-by: Cursor <cursoragent@cursor.com>
…2241) Math-CI ROCm 7.0.2 backwards-compat builds the host DevRuntime stubs without the CUDA-named CU_MEM_* aliases from p2p.h, so CU_MEM_LOCATION_TYPE_DEVICE failed to compile. Match sibling tests that already use hipMemLocationTypeDevice. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Deferred Argus's remaining test gap ( Also rebasing this branch onto current |
|
Rebased onto current %RCCL_TEST_CONFIG |
8f6914a to
cbb71f3
Compare
There was a problem hiding this comment.
ARGUS
Bottom line: LGTM once symMemoryUnregister (dev_runtime.cc:745) gets the same rmaHostWins[0] != nullptr guard the file's two other deregister sites use. That one is mechanical, so it does not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
Match the other deregister sites so a partway ncclRmaProxyConnectOnce failure cannot walk past the rmaHostWins slots. Note the ginWins clear loop as defensive-only per Argus; unlink helper and tests stay in AICOMRCCL-2428. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed Argus's remaining merge-blocking nit in
Unlink-helper extraction and host coverage for the null-skip deregister paths stay in AICOMRCCL-2428. @Kapil-Shyam-Pawar please re-approve on this SHA when CI is green. @argus re-review %RCCL_TEST_CONFIG |
|
@ROCm/rccl-ci @ROCm/rccl-leads runner ping: RCCL cluster CI for this SHA has been queued since ~20:34 UTC with no runner pickup (~3h).
GIN is the functional check for AICOMRCCL-2241 ( HEAD: |
|
Follow-up draft for the deferred Argus coverage (AICOMRCCL-2428): stacked host micro-test PR opened as a draft; will rebase onto |
|
Second runner ping @ROCm/rccl-ci @ROCm/rccl-leads: RCCL cluster CI for this SHA still has not started. Same run as yesterday, still queued, no job pickup.
GIN remains the Anvil SDMA functional check for AICOMRCCL-2241 ( |
…L-2241) (#11863) ## Motivation `ncclCommWindowRegister` fails with `could not resolve LSA flat addr` when it is called after `ncclDevCommCreate` on the GIN Anvil SDMA backend (`NCCL_GIN_TYPE=7`). Registering a symmetric window after creating a device communicator is legal, but on this backend it aborted registration, which made the GIN device-API alltoall tests unable to reach a kernel launch. Replaces #11089, which GitHub auto-closed when #11765 merged and deleted `users/kpawar/gin-sdma-build-fix`. This branch is rebased onto current `develop` (includes #11765). Review history and Argus LGTM live on #11089. ## Technical Details `symMemoryObtain` registered the window with the GIN/RMA plugins *before* linking the in-flight `ncclDevrMemory` onto `devr->memHead`. The Anvil SDMA plugin resolves the caller's user VA inside `ginAnvilRegMrSym` via `ncclDevrGetLsaSelfAddr`, and that helper only walks `devr->memHead` (plus the LSA flat range). With the link happening after registration, the memory being registered was invisible to the lookup, so it returned `nullptr` and registration failed. This changes the ordering so `mem` is on `memHead` before `symMemoryRegisterGin`/`symMemoryRegisterRma` run, which restores the invariant those plugins depend on. Since the window is now published earlier, the `fail_mem_space_teams` path also has to undo more work: - unlink `mem` from `memHead`, - `symMemoryUnregister` for GIN/RMA teardown (shared with `symMemoryDestroy`), - unbind team memory. The single-segment `symMemoryRegisterGin` path uses `NCCLCHECKGOTO` so a partway `ncclGinRegister` failure deregisters MRs already stored in `ginHostWins` even when `ginSegmentInfos` was never allocated. `ncclRmaProxyDeregister` skips null handles so a partial `ncclRmaProxyRegister` cannot walk into `deregMrSym` implementations that deref unchecked. `fail_mem` additionally frees `mem->ginSegmentInfos`, which leaked when GIN registration succeeded but a later step failed. `mem` comes from `ncclCalloc`, so both guards read as null on the early failure paths. ## Issue Tracking JIRA ID: AICOMRCCL-2241 ## Test Plan MI355X (gfx950, 8 XCC), 2 ranks on one node, GIN Anvil SDMA backend: ``` NCCL_GIN_TYPE=7 NCCL_CUMEM_ENABLE=1 RCCL_ENABLE_INTRANET=1 \ mpirun -np 2 rccl-UnitTestsMPI --gtest_filter=\ GinMPIDeviceTests.Alltoall_PureReference:\ GinMPIDeviceTests.AlltoallHybrid_Reference:\ GinMPIDeviceTests.BarrierSession_LsaOnly:\ GinMPIDeviceTests.SignalAdd_AndShadow:\ GinMPIDeviceTests.WindowRegister_AfterDevCommCreate ``` `WindowRegister_AfterDevCommCreate` is the dedicated regression for post-`ncclDevCommCreate` window register. It stays on `gin_devmpi_common` (GDA extra API coverage) and is also listed in the `gin_devmpi_sdma` subset (Anvil SDMA is the backend that actually failed). Host micro-test: `rccl-UnitTestsDevRuntime --gtest_filter=SymMemoryObtainTest.*` covers destroy plus GIN-register failure unlinking `memHead`. Cluster: `%RCCL_TEST_CONFIG gin_rocshmem_backends_mellanox.json` (`gin_devmpi_sdma` plus GDA `gin_devmpi_common`). ## Test Result All five tests pass with the fix. Before it, both alltoall tests failed in `ncclCommWindowRegister` with `could not resolve LSA flat addr`. ``` [ RUN ] GinMPIDeviceTests.BarrierSession_LsaOnly [ OK ] (7745 ms) [ RUN ] GinMPIDeviceTests.SignalAdd_AndShadow [ OK ] (2273 ms) [ RUN ] GinMPIDeviceTests.Alltoall_PureReference [ OK ] (2267 ms) [ RUN ] GinMPIDeviceTests.AlltoallHybrid_Reference [ OK ] (2257 ms) [ PASSED ] 4 tests. ``` Note for anyone reproducing this on a `Debug` build of the tests: run with `HSA_NO_SCRATCH_RECLAIM=0`, or build the test TU optimized. `Debug` compiles these kernels with no `-O`, so the GIN device structs spill to scratch (`alltoallPureKernel` needs 28,720 bytes/lane, `alltoallHybridKernel` 47,424, versus 1,136 at `-O3`). `HSA_NO_SCRATCH_RECLAIM=1` makes ROCr reserve scratch for full device occupancy and forbids its occupancy-reduction fallback, so on a 256-CU part those kernels each demand 14-23 GiB against a 32 GiB scratch pool. The suite then aborts with `HSA_STATUS_ERROR_OUT_OF_RESOURCES` on whichever kernel exhausts the pool. That is a build-configuration and scratch-policy interaction, unrelated to this change. ## 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>
…L-2241) (#11863) ## Motivation `ncclCommWindowRegister` fails with `could not resolve LSA flat addr` when it is called after `ncclDevCommCreate` on the GIN Anvil SDMA backend (`NCCL_GIN_TYPE=7`). Registering a symmetric window after creating a device communicator is legal, but on this backend it aborted registration, which made the GIN device-API alltoall tests unable to reach a kernel launch. Replaces #11089, which GitHub auto-closed when #11765 merged and deleted `users/kpawar/gin-sdma-build-fix`. This branch is rebased onto current `develop` (includes #11765). Review history and Argus LGTM live on #11089. ## Technical Details `symMemoryObtain` registered the window with the GIN/RMA plugins *before* linking the in-flight `ncclDevrMemory` onto `devr->memHead`. The Anvil SDMA plugin resolves the caller's user VA inside `ginAnvilRegMrSym` via `ncclDevrGetLsaSelfAddr`, and that helper only walks `devr->memHead` (plus the LSA flat range). With the link happening after registration, the memory being registered was invisible to the lookup, so it returned `nullptr` and registration failed. This changes the ordering so `mem` is on `memHead` before `symMemoryRegisterGin`/`symMemoryRegisterRma` run, which restores the invariant those plugins depend on. Since the window is now published earlier, the `fail_mem_space_teams` path also has to undo more work: - unlink `mem` from `memHead`, - `symMemoryUnregister` for GIN/RMA teardown (shared with `symMemoryDestroy`), - unbind team memory. The single-segment `symMemoryRegisterGin` path uses `NCCLCHECKGOTO` so a partway `ncclGinRegister` failure deregisters MRs already stored in `ginHostWins` even when `ginSegmentInfos` was never allocated. `ncclRmaProxyDeregister` skips null handles so a partial `ncclRmaProxyRegister` cannot walk into `deregMrSym` implementations that deref unchecked. `fail_mem` additionally frees `mem->ginSegmentInfos`, which leaked when GIN registration succeeded but a later step failed. `mem` comes from `ncclCalloc`, so both guards read as null on the early failure paths. ## Issue Tracking JIRA ID: AICOMRCCL-2241 ## Test Plan MI355X (gfx950, 8 XCC), 2 ranks on one node, GIN Anvil SDMA backend: ``` NCCL_GIN_TYPE=7 NCCL_CUMEM_ENABLE=1 RCCL_ENABLE_INTRANET=1 \ mpirun -np 2 rccl-UnitTestsMPI --gtest_filter=\ GinMPIDeviceTests.Alltoall_PureReference:\ GinMPIDeviceTests.AlltoallHybrid_Reference:\ GinMPIDeviceTests.BarrierSession_LsaOnly:\ GinMPIDeviceTests.SignalAdd_AndShadow:\ GinMPIDeviceTests.WindowRegister_AfterDevCommCreate ``` `WindowRegister_AfterDevCommCreate` is the dedicated regression for post-`ncclDevCommCreate` window register. It stays on `gin_devmpi_common` (GDA extra API coverage) and is also listed in the `gin_devmpi_sdma` subset (Anvil SDMA is the backend that actually failed). Host micro-test: `rccl-UnitTestsDevRuntime --gtest_filter=SymMemoryObtainTest.*` covers destroy plus GIN-register failure unlinking `memHead`. Cluster: `%RCCL_TEST_CONFIG gin_rocshmem_backends_mellanox.json` (`gin_devmpi_sdma` plus GDA `gin_devmpi_common`). ## Test Result All five tests pass with the fix. Before it, both alltoall tests failed in `ncclCommWindowRegister` with `could not resolve LSA flat addr`. ``` [ RUN ] GinMPIDeviceTests.BarrierSession_LsaOnly [ OK ] (7745 ms) [ RUN ] GinMPIDeviceTests.SignalAdd_AndShadow [ OK ] (2273 ms) [ RUN ] GinMPIDeviceTests.Alltoall_PureReference [ OK ] (2267 ms) [ RUN ] GinMPIDeviceTests.AlltoallHybrid_Reference [ OK ] (2257 ms) [ PASSED ] 4 tests. ``` Note for anyone reproducing this on a `Debug` build of the tests: run with `HSA_NO_SCRATCH_RECLAIM=0`, or build the test TU optimized. `Debug` compiles these kernels with no `-O`, so the GIN device structs spill to scratch (`alltoallPureKernel` needs 28,720 bytes/lane, `alltoallHybridKernel` 47,424, versus 1,136 at `-O3`). `HSA_NO_SCRATCH_RECLAIM=1` makes ROCr reserve scratch for full device occupancy and forbids its occupancy-reduction fallback, so on a 256-CU part those kernels each demand 14-23 GiB against a 32 GiB scratch pool. The suite then aborts with `HSA_STATUS_ERROR_OUT_OF_RESOURCES` on whichever kernel exhausts the pool. That is a build-configuration and scratch-policy interaction, unrelated to this change. ## 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>
…L-2428) Exercise fail_mem_space_teams after a successful GIN register so symMemoryUnregister's GIN arm and free(ginSegmentInfos) run, then assert memHead is unlinked. Kept out of ROCm#11863 so AICOMRCCL-2241 can land first.
…L-2428) (#12705) ## Motivation Follow-up to #11863 / AICOMRCCL-2241, which has merged. Argus noted that `free(mem->ginSegmentInfos)` on the `fail_mem` path is never exercised: reaching it needs GIN register to succeed and RMA register to then fail. Existing host coverage either fails GIN first (`GinRegisterFails_UnbindsTeamsAndReturnsSpace`) or fails RMA with `ginEnabled` false (`RmaRegisterFails_ReturnsSpaceWithoutLinking`). Supersedes #12082, which was opened from a fork branch and so could not pass the `RCCL PR gate` coco smoke check. Same single commit, same content, now on a branch in this repo. ## Technical Details Adds `SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks` in `rccl-UnitTestsMicro` (`test/host/dev-runtime-test.cc`): - `ginEnabled` true, GIN register succeeds, RMA register fails - Asserts `symMemoryUnregister`'s GIN arm ran (`ncclGinDeregister` once) - Asserts RMA deregister is skipped (`rmaHostWins[0]` never set) - Asserts `memHead` is unlinked and reserved space is returned `free(ginSegmentInfos)` sits on the same fail path after unregister; the deregister call is the observable witness because `mem` is freed before `Obtain` returns. Remaining AICOMRCCL-2428 items (shared unlink helper, `rccl-UnitTestsDevRuntime` CI wiring, real-TU null-skip deregister coverage) stay for later PRs. JIRA ID: AICOMRCCL-2428 ## Test Plan ``` rccl-UnitTestsMicro --gtest_filter=SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks ``` Host CPU-only CI (`RCCL host unit tests`) also runs the suite via `rccl-UnitTestsMicro`. ## Test Result `Host unit tests (CPU-only)` passed on this commit in #12082 (run 37050708055): all five `SymMemoryObtainRollbackTest` cases ran green, including the new one. ``` [ RUN ] SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks [ OK ] SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks (0 ms) ``` ## 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>
…L-2428) (#12705) ## Motivation Follow-up to #11863 / AICOMRCCL-2241, which has merged. Argus noted that `free(mem->ginSegmentInfos)` on the `fail_mem` path is never exercised: reaching it needs GIN register to succeed and RMA register to then fail. Existing host coverage either fails GIN first (`GinRegisterFails_UnbindsTeamsAndReturnsSpace`) or fails RMA with `ginEnabled` false (`RmaRegisterFails_ReturnsSpaceWithoutLinking`). Supersedes #12082, which was opened from a fork branch and so could not pass the `RCCL PR gate` coco smoke check. Same single commit, same content, now on a branch in this repo. ## Technical Details Adds `SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks` in `rccl-UnitTestsMicro` (`test/host/dev-runtime-test.cc`): - `ginEnabled` true, GIN register succeeds, RMA register fails - Asserts `symMemoryUnregister`'s GIN arm ran (`ncclGinDeregister` once) - Asserts RMA deregister is skipped (`rmaHostWins[0]` never set) - Asserts `memHead` is unlinked and reserved space is returned `free(ginSegmentInfos)` sits on the same fail path after unregister; the deregister call is the observable witness because `mem` is freed before `Obtain` returns. Remaining AICOMRCCL-2428 items (shared unlink helper, `rccl-UnitTestsDevRuntime` CI wiring, real-TU null-skip deregister coverage) stay for later PRs. JIRA ID: AICOMRCCL-2428 ## Test Plan ``` rccl-UnitTestsMicro --gtest_filter=SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks ``` Host CPU-only CI (`RCCL host unit tests`) also runs the suite via `rccl-UnitTestsMicro`. ## Test Result `Host unit tests (CPU-only)` passed on this commit in #12082 (run 37050708055): all five `SymMemoryObtainRollbackTest` cases ran green, including the new one. ``` [ RUN ] SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks [ OK ] SymMemoryObtainRollbackTest.GinSucceedsThenRmaFails_UnregistersGinAndUnlinks (0 ms) ``` ## 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>
## Motivation The code behind these changelog entries merged after the ROCm 10.1 snap, so they did not ship in 10.1. They belong in the next release section, not in the released section. This moves them and changes no text. ## Technical Details Moves 3 entries to `RCCL 2.32.3 for ROCm 10.2.0 (Unreleased)`, each under its matching category heading: - [#11475](#11475): Vendored `{fmt}` 10.2.1 as a header-only tree under `external/fmt`. RCCL, its tests, and the ... - Merge commit `de0286a0ea` is not in the snap (GitHub compare status `diverged`). - `projects/rccl/external/fmt/CMakeLists.txt` does not exist at the pin. - [#11863](#11863): Fixed `ncclCommWindowRegister` after `ncclDevCommCreate` aborting on the GIN Anvil SDMA backend ... - Merge commit `a021a314d1` is not in the snap (GitHub compare status `diverged`). - 3 line(s) the PR added to `projects/rccl/src/dev_runtime.cc` are missing from it at the pin, e.g. `for (int i = 0; mem->ginSegmentInfos != nullptr && i <= numSegmentsRegistered && i < mem->numGinSegm`. - [#11368](#11368): Fixed the DDA, Copy-Engine 2-shot, GIN SDMA and hierarchical AllGather/ReduceScatter backends not ... - Merge commit `586c5a3d64` is not in the snap (GitHub compare status `diverged`). - 3 line(s) the PR added to `projects/rccl/src/algorithms/dda/all_gather/dda_all_gather_fabric.cu` are missing from it at the pin, e.g. `hipExtLaunchKernelGGL((dda::common::ddaAllGatherFabric<T, 4>), grid, block, 0, stream, /*startEvent=`. If an entry should stay where it is, say so in a comment and it will be dropped from this PR. ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ## Test Plan Check each merge commit against the ROCm 10.1 snap (`release/therock-10.1`, `rocm-systems` pin `7da43026e4f2ef7600433c38c820732f7a205d22`): ``` git fetch origin 7da4302 git merge-base --is-ancestor de0286a 7da4302; echo $? # 1 means not in the snap git merge-base --is-ancestor a021a31 7da4302; echo $? # 1 means not in the snap git merge-base --is-ancestor 586c5a3 7da4302; echo $? # 1 means not in the snap ``` ## Test Result - Each merge commit is not an ancestor of the pin (compare status `diverged` or `ahead`). - The code each PR added is absent from the files at the pin (see Technical Details). - The patch applies cleanly to develop, and the non-blank lines of the changelog are identical before and after, so this is a pure move. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Corey Derochie <corey.derochie@amd.com>
Motivation
ncclCommWindowRegisterfails withcould not resolve LSA flat addrwhen it is called afterncclDevCommCreateon the GIN Anvil SDMA backend (NCCL_GIN_TYPE=7). Registering a symmetric window after creating a device communicator is legal, but on this backend it aborted registration, which made the GIN device-API alltoall tests unable to reach a kernel launch.Replaces #11089, which GitHub auto-closed when #11765 merged and deleted
users/kpawar/gin-sdma-build-fix. This branch is rebased onto currentdevelop(includes #11765). Review history and Argus LGTM live on #11089.Technical Details
symMemoryObtainregistered the window with the GIN/RMA plugins before linking the in-flightncclDevrMemoryontodevr->memHead.The Anvil SDMA plugin resolves the caller's user VA inside
ginAnvilRegMrSymviancclDevrGetLsaSelfAddr, and that helper only walksdevr->memHead(plus the LSA flat range). With the link happening after registration, the memory being registered was invisible to the lookup, so it returnednullptrand registration failed.This changes the ordering so
memis onmemHeadbeforesymMemoryRegisterGin/symMemoryRegisterRmarun, which restores the invariant those plugins depend on. Since the window is now published earlier, thefail_mem_space_teamspath also has to undo more work:memfrommemHead,symMemoryUnregisterfor GIN/RMA teardown (shared withsymMemoryDestroy),The single-segment
symMemoryRegisterGinpath usesNCCLCHECKGOTOso a partwayncclGinRegisterfailure deregisters MRs already stored inginHostWinseven whenginSegmentInfoswas never allocated.ncclRmaProxyDeregisterskips null handles so a partialncclRmaProxyRegistercannot walk intoderegMrSymimplementations that deref unchecked.fail_memadditionally freesmem->ginSegmentInfos, which leaked when GIN registration succeeded but a later step failed.memcomes fromncclCalloc, so both guards read as null on the early failure paths.Issue Tracking
JIRA ID: AICOMRCCL-2241
Test Plan
MI355X (gfx950, 8 XCC), 2 ranks on one node, GIN Anvil SDMA backend:
WindowRegister_AfterDevCommCreateis the dedicated regression for post-ncclDevCommCreatewindow register. It stays ongin_devmpi_common(GDA extra API coverage) and is also listed in thegin_devmpi_sdmasubset (Anvil SDMA is the backend that actually failed).Host micro-test:
rccl-UnitTestsDevRuntime --gtest_filter=SymMemoryObtainTest.*covers destroy plus GIN-register failure unlinkingmemHead.Cluster:
%RCCL_TEST_CONFIG gin_rocshmem_backends_mellanox.json(gin_devmpi_sdmaplus GDAgin_devmpi_common).Test Result
All five tests pass with the fix. Before it, both alltoall tests failed in
ncclCommWindowRegisterwithcould not resolve LSA flat addr.Note for anyone reproducing this on a
Debugbuild of the tests: run withHSA_NO_SCRATCH_RECLAIM=0, or build the test TU optimized.Debugcompiles these kernels with no-O, so the GIN device structs spill to scratch (alltoallPureKernelneeds 28,720 bytes/lane,alltoallHybridKernel47,424, versus 1,136 at-O3).HSA_NO_SCRATCH_RECLAIM=1makes ROCr reserve scratch for full device occupancy and forbids its occupancy-reduction fallback, so on a 256-CU part those kernels each demand 14-23 GiB against a 32 GiB scratch pool. The suite then aborts withHSA_STATUS_ERROR_OUT_OF_RESOURCESon whichever kernel exhausts the pool. That is a build-configuration and scratch-policy interaction, unrelated to this change.Submission Checklist
Made with Cursor