Repository navigation
fix(rccl/gin): resolve VA signals on the rocSHMEM GDA backend - #14203
Merged
Merged
Conversation
![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
2 more finding(s), not posted inline
projects/rccl/test/device/GinRocshmemGdaTemplate_test.cpp:756(Low) VaSignal_PutSignalsWindowCell cannot detect a wrong VA rkey, because GdaEnv zeroes every rkey (lines 115-116) and the rocSHMEM mock records only counts. Seed distinct per-peer rkeys once the mock records them.projects/rccl/test/device/GinRocshmemGdaTemplate_test.cpp:587(Nit) Trailing comment at line 587 says 'non-indexed reset', but a VA reset now clears its cell. Say 'NONE reset'. The trailing comment at line 587 still says "untouched by non-indexed reset". A VA reset now clears its cell, so could it say "untouched by NONE reset"?
Bottom line: Worth settling before approval: the four newly un-skipped VASignal_* MPI cases, including VASignal_ReadAndReset, are not scheduled on NCCL_GIN_TYPE=6, so the VA ResetSignal path gets no end-to-end GDA run.
Comment @argus review to re-run, or @argus re-review after pushing.
dlamd1dai
force-pushed
the
users/dondai/aicomrccl-3180-gda-va-signals
branch
from
October 24, 2026 21:32
f0e2cd4 to
e47453b
Compare
Base automatically changed from
users/dondai/gin-anvil-sdma-strong-signal
to
develop
October 25, 2026 21:13
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
The GDA plugin advertises supportsVASignals, but Put and PutValue built the signal address from indexedSignal.signalId for every descriptor. For a VA descriptor that field aliases vaSignal.signalWindow, so the atomic landed at an unrelated offset past the indexed signal buffer. ResetSignal ignored VA descriptors. - Put/PutValue resolve a VA signal through its window's per-peer remote VA and rkey, the same way a put destination is resolved. - ResetSignal clears a VA cell through the window's local VA. - regMrSym registers windows with remote atomics, which the NIC requires for an RDMA atomic into a window cell. - GinMPIDeviceTests.VASignal_* no longer skip GDA. JIRA ID: AICOMRCCL-3180 Co-authored-by: Cursor <cursoragent@cursor.com>
The GDA VA-signal fix un-skipped VASignal_NoPayload_IncAndAdd, VASignal_ReadAndReset, VASignal_StrictOrderingFlag and VASignal_Put_MultiContext, but no job ran them with NCCL_GIN_TYPE=6. Add them to gin_devmpi_gda and to gin-tests.json, with NCCL_GIN_NCONTEXTS=2 for the multi-context case. JIRA ID: AICOMRCCL-3180 Co-authored-by: Cursor <cursoragent@cursor.com>
dlamd1dai
force-pushed
the
users/dondai/aicomrccl-3180-gda-va-signals
branch
from
October 27, 2026 02:58
15a7e10 to
0b76b5e
Compare
Contributor
pr-rocm-systems-14203-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14203-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
ywang103-amd
pushed a commit
that referenced
this pull request
Oct 28, 2026
## Motivation The rocSHMEM GDA backend (`NCCL_GIN_TYPE=6`) advertises `supportsVASignals = true`, but its device path never handled VA signals. `Put` and `PutValue` built the signal address from `indexedSignal.signalId` for every descriptor. For a VA descriptor, that field aliases `vaSignal.signalWindow` in the same union, so the atomic landed at an unrelated offset past the indexed signal buffer and never reached the VA cell. `ResetSignal` ignored VA descriptors. The MPI tests already skipped `VASignal_*` on GDA, while the plugin claimed support. This is the VA-signal half of AICOMRCCL-3180. Strong-signal ordering on GDA comes in a follow-up PR. ## Technical Details **Device (`gin_rocshmem_gda.h`)** - New `detail::gdaSignalTarget()` returns the remote address and rkey of the cell a signal targets. A VA signal resolves through its window's per-peer `remote_vas` and `rkeys`, the same way a put destination is resolved. An indexed signal keeps today's per-context stripe lookup. - `Put` and `PutValue` both call it, so neither one reads `indexedSignal` off a VA descriptor any more. - `ResetSignal` clears a VA cell through the window's `local_va`, the same way Anvil does since #12748. **Host (`gin_plugin_rocshmem_gda.cc`)** - `regMrSym` registers windows with `atomic=1` (`IBV_ACCESS_REMOTE_ATOMIC`). A VA signal is an RDMA atomic into a window cell, and without the flag the NIC rejects it and moves the queue pair to an error state. GDAKI already registers windows this way. **Unchanged** - `supportsStrongSignals` and `ncclGinApi_SupportsStrongSignal` stay as they are. The follow-up handles them. - Weak and strong signals still post a single `atomic_add` after the put. ## Issue Tracking JIRA ID: AICOMRCCL-3180 ## Test Plan - Device fixtures, mocked queue pair, no NIC (`rccl-UnitTestsFixtures --gtest_filter=GinRocshmemGdaTemplateTest.*`): - `VaSignal_PutSignalsWindowCell`: for both `Put` and `PutValue`, the payload lands, the VA cell takes the signal argument, and no indexed signal changes. - `ResetSignal_VaClearsWindowCell`: the VA cell is cleared and indexed signals are untouched. - Host plugin test (`rccl-UnitTestsGinRocshmemGdaPlugin`): - `RegMrSym_AllowsRemoteAtomicsForVaSignals`: the window MR is registered with `atomic=1`, and the device memory handle carries the local VA plus the per-peer VAs and rkeys. - MPI on hardware (`NCCL_GIN_TYPE=6`, np 2): `GinMPIDeviceTests.VASignal_*` no longer skip GDA. This run is the one that matters, because the mocked queue pair ignores rkeys and only a real NIC shows whether the atomic is accepted. ## Test Result - `rccl-UnitTestsGinRocshmemGdaPlugin --gtest_filter=GinRocshmemGdaPluginTest.*`: 6 passed. - `rccl-UnitTestsFixtures --gtest_filter=GinRocshmemGdaTemplateTest.*`: 18 passed, including both VA-signal cases. - MPI, np 2 on one node, `NCCL_GIN_TYPE=6`, rebased build: `VASignal_Put`, `VASignal_NoPayload_IncAndAdd`, `VASignal_ReadAndReset` and `VASignal_StrictOrderingFlag` passed, and `VASignal_Put_MultiContext` passed with `NCCL_GIN_NCONTEXTS=2`, on both NICs: - MI300X with ConnectX-7 (mlx5). - MI300X with Broadcom BCM57608 (bnxt_re, firmware 233.2.104.0). GIN picked `rocshmem-gda` and registered the windows with real rkeys. - Both runs keep the two ranks on one node, so the RDMA traffic goes through NIC loopback, not across nodes. ## 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>
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
The rocSHMEM GDA backend (
NCCL_GIN_TYPE=6) advertisessupportsVASignals = true, but its device path never handled VA signals.PutandPutValuebuilt the signal address fromindexedSignal.signalIdfor every descriptor. For a VA descriptor, that field aliasesvaSignal.signalWindowin the same union, so the atomic landed at an unrelated offset past the indexed signal buffer and never reached the VA cell.ResetSignalignored VA descriptors. The MPI tests already skippedVASignal_*on GDA, while the plugin claimed support.This is the VA-signal half of AICOMRCCL-3180. Strong-signal ordering on GDA comes in a follow-up PR.
Technical Details
Device (
gin_rocshmem_gda.h)detail::gdaSignalTarget()returns the remote address and rkey of the cell a signal targets. A VA signal resolves through its window's per-peerremote_vasandrkeys, the same way a put destination is resolved. An indexed signal keeps today's per-context stripe lookup.PutandPutValueboth call it, so neither one readsindexedSignaloff a VA descriptor any more.ResetSignalclears a VA cell through the window'slocal_va, the same way Anvil does since feat(gin/sdma): honour strong signals on the Anvil SDMA backend #12748.Host (
gin_plugin_rocshmem_gda.cc)regMrSymregisters windows withatomic=1(IBV_ACCESS_REMOTE_ATOMIC). A VA signal is an RDMA atomic into a window cell, and without the flag the NIC rejects it and moves the queue pair to an error state. GDAKI already registers windows this way.Unchanged
supportsStrongSignalsandncclGinApi_SupportsStrongSignalstay as they are. The follow-up handles them.atomic_addafter the put.Issue Tracking
JIRA ID: AICOMRCCL-3180
Test Plan
rccl-UnitTestsFixtures --gtest_filter=GinRocshmemGdaTemplateTest.*):VaSignal_PutSignalsWindowCell: for bothPutandPutValue, the payload lands, the VA cell takes the signal argument, and no indexed signal changes.ResetSignal_VaClearsWindowCell: the VA cell is cleared and indexed signals are untouched.rccl-UnitTestsGinRocshmemGdaPlugin):RegMrSym_AllowsRemoteAtomicsForVaSignals: the window MR is registered withatomic=1, and the device memory handle carries the local VA plus the per-peer VAs and rkeys.NCCL_GIN_TYPE=6, np 2):GinMPIDeviceTests.VASignal_*no longer skip GDA. This run is the one that matters, because the mocked queue pair ignores rkeys and only a real NIC shows whether the atomic is accepted.Test Result
rccl-UnitTestsGinRocshmemGdaPlugin --gtest_filter=GinRocshmemGdaPluginTest.*: 6 passed.rccl-UnitTestsFixtures --gtest_filter=GinRocshmemGdaTemplateTest.*: 18 passed, including both VA-signal cases.NCCL_GIN_TYPE=6, rebased build:VASignal_Put,VASignal_NoPayload_IncAndAdd,VASignal_ReadAndResetandVASignal_StrictOrderingFlagpassed, andVASignal_Put_MultiContextpassed withNCCL_GIN_NCONTEXTS=2, on both NICs:rocshmem-gdaand registered the windows with real rkeys.Submission Checklist