Skip to content

fix(gin): size Anvil signal allgather to the world communicator - #12450

Closed
dlamd1dai wants to merge 1 commit into
ROCm:developfrom
dlamd1dai:users/dondai/aicomrccl-2339-signal-allgather
Closed

dlamd1dai wants to merge 1 commit into
ROCm:developfrom
dlamd1dai:users/dondai/aicomrccl-2339-signal-allgather

Conversation

@dlamd1dai

Copy link
Copy Markdown
Contributor

Motivation

Follow-up to #11569 / AICOMRCCL-2339. ginAnvilRegisterLsaSignals allgathers each rank's LSA signal base through bootstrapAllGather, which writes one slot per world rank (bootstrapState::nranks). The buffer was ctx->nRanks long and the local VA was stored at ctx->rank. Under NCCL_GIN_CONNECTION_RAIL those are the GIN team, so the ring writes past the allocation and publishes this rank's VA in the wrong slot.

Technical Details

  • Allocate comm->nRanks slots
  • Publish and check the local VA at comm->rank, which matches the bootstrap rank index
  • On a full (non-RAIL) connection, GIN rank and world rank are the same, so the check is unchanged

#11569 rewrites ginAnvilRegisterLsaSignals and still sizes this buffer from ctx->nRanks. If that PR merges first, rebase this change onto it.

JIRA ID: AICOMRCCL-2339

Test Plan

  • Single-node Anvil (NCCL_GIN_TYPE=7, 1 node / 2 ranks) still binds LSA signals; the world size equals the GIN team, so the gathered slot is the same as before
  • No multi-node / RAIL run planned for this change

Test Result

  • Not run on a SUT in this session

Submission Checklist

Made with Cursor

bootstrapAllGather writes one slot per world rank. Under RAIL a GIN-team buffer is too small and the local VA is published at the wrong index.

Co-authored-by: Cursor <cursoragent@cursor.com>
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rccl/src/gin/gin_plugin_anvil_sdma.cc; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Findings

1 finding(s) that could not be posted as inline comments
  • projects/rccl/src/gin/gin_plugin_rocshmem_gda.cc:303 (Medium) The same team-sized allgather buffer is still live in the sibling rocshmem-gda backend, where the gathered data does reach the device The same overflow is still live in the sibling backend. ginRocshmemGdaCreateContext at gin_plugin_rocshmem_gda.cc:303-306 sizes both allgather buffers from ctx-&gt;nRanks and publishes at ctx-&gt;rank, then bootstrapAllGather at 308-309 writes comm-&gt;nRanks slots, and unlike the block fixed here the result is copied to the device at 311-312. ginRocshmemGdaRegMrSym at gin_plugin_rocshmem_gda.cc:187-193 has the same shape. That file is outside this PR, so this looks like a separate change rather than something to fold in here.

Bottom line: Worth settling before approval: no test can observe this fix, since the GIN plugin suite's allgather stub discards non-int payloads and passes identically with the fix reverted. The sizing change itself checks out against the bootstrap allgather contract.

Comment @argus review to re-run, or @argus re-review after pushing.

(unsigned long)gatheredLocalBases[ctx->rank], (unsigned long)lsaSelf);
if (comm->rank >= 0 && comm->rank < worldRanks) {
gatheredLocalBases[comm->rank] = (uintptr_t)lsaSelf;
if (ginAnvilBootstrapAllgather(comm->bootstrap, gatheredLocalBases, sizeof(uintptr_t)) != 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium

No test can observe this fix: the stub bootstrapAllGather at gin_anvil_plugin_test_stubs.cc:103 returns early for any element size other than sizeof(int), and this call passes sizeof(uintptr_t), so rccl-UnitTestsGinAnvilPlugin passes bit-identically with the fix reverted. ConnCheck_SkippedOnRailConnection at GinAnvilPlugin_test.cpp:526 already builds comm->rank=1 against ctx->rank=0, so the divergence is one fixture away. Could the stub record the buffer and size, the way bootstrapIntraNodeAllGather already records ranks at gin_anvil_plugin_test_stubs.cc:144, so the published slot becomes assertable?

WARN("GIN anvil-sdma: signal local-base allgather mismatch rank=%d got=%#lx expect=%#lx", comm->rank,
(unsigned long)gatheredLocalBases[comm->rank], (unsigned long)lsaSelf);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
}
uintptr_t* gatheredLocalBases = (uintptr_t*)calloc((size_t)comm->nRanks, sizeof(uintptr_t));
if (gatheredLocalBases) {
gatheredLocalBases[comm->rank] = (uintptr_t)lsaSelf;
if (ginAnvilBootstrapAllgather(comm->bootstrap, gatheredLocalBases, sizeof(uintptr_t)) != 0) {
WARN("GIN anvil-sdma: signal local-base allgather failed");
} else if (gatheredLocalBases[comm->rank] != (uintptr_t)lsaSelf) {
WARN("GIN anvil-sdma: signal local-base allgather mismatch rank=%d got=%#lx expect=%#lx", comm->rank,
(unsigned long)gatheredLocalBases[comm->rank], (unsigned long)lsaSelf);
}

@dlamd1dai dlamd1dai self-assigned this Sep 30, 2026
@dlamd1dai

Copy link
Copy Markdown
Contributor Author

Superseded by #11569 (AICOMRCCL-2339).

#11569 was rebased onto current develop and now carries this fix directly. It deletes ginAnvilRegisterLsaSignals and replaces it with ginAnvilRegisterSignalSpan, which registers one LSA signal stripe per logical context, so the standalone patch here no longer applies to a function that exists. The world-sizing change was folded into the new span probe: the buffer is sized with comm->nRanks and the local VA is published at comm->rank, matching what bootstrapAllGather actually writes.

Closing in favour of #11569.

@dlamd1dai dlamd1dai closed this Oct 1, 2026
dlamd1dai added a commit that referenced this pull request Oct 24, 2026
> **Replaces #11569.** Identical commits and content; head is
`cf8cf9720f16391c76a19534b2af34eaea0c67cd` on both.
>
> #11569 was opened from a fork, where the required `RCCL PR gate` check
can never pass. Fork pull requests receive no secrets, so the `RCCL coco
PR` workflow skips its `Setup` job, the coco smoke never runs, and the
gate fails closed within seconds. Clearing it there needs a maintainer
to dispatch the workflow by hand and override the check, on every push.
This branch lives in `ROCm/rocm-systems`, so the gate dispatches coco to
the Ruby and OCI clusters on its own.
>
> Review history stays on #11569: 27 threads, all resolved. No approval
was lost; it had no approval or changes-requested decision.

## Motivation

Anvil SDMA currently allocates one GPU context per connection and
ignores `nContexts`. When `ncclGinAllContexts` barriers run on a shared
connection, every logical context writes and waits on the same signal
cells, so a later barrier can complete on a stale count from an earlier
one.

## Technical Details

Give each logical context its own `ncclGinAnvilSdmaGPUContext` and LSA
signal stripe while keeping SDMA queues, dirty bits, and counters shared
at the connection. Device APIs now index `ctx.handle` by
`ctx.contextId`. Resource-window bind walks `nContexts` slots instead of
one slot per plugin context.

Signal peer addresses are indexed in GIN-team space (`ctx->rank`),
matching `resolveRemotePeerVa`; `regMrSym` keeps `devr->lsaSelf` because
that table is LSA-indexed. The signal span is registered in the IPC
table once per bind rather than once per context, and the comm-wide
collectives (the local-base allgather and the LSA conn-check) run at
span scope so a comm with N logical contexts still costs one bootstrap
round trip, not N.

Two fixes to the surrounding code are folded in, previously proposed
standalone as #12450 and #12451 and closed in favour of this PR once the
restructuring above made them inapplicable as separate patches:

- The span base probe is sized with `comm->nRanks` and publishes the
local VA at `comm->rank`. `bootstrapAllGather` writes one slot per world
rank, so a GIN-team-sized buffer was overrun and the VA landed in the
wrong slot under `NCCL_GIN_CONNECTION_RAIL`.
- The bind path snapshots `g_pendingByComm` into a vector under
`pluginMutex` and walks the copy. Its three reads (span geometry, slot
binding, and `fail:` rollback) were unlocked while
`ginAnvilPendingAdd`/`Remove`/`Clear` all hold that non-recursive mutex;
`find()` also avoids default-inserting an empty list for a comm with
nothing pending.

JIRA ID: AICOMRCCL-2339
AICOMRCCL-3072

## Test Plan

- Host plugin unit tests, including
`BindSignals_LogicalContextsUseDistinctSignalStripes` and the
`ConnCheck_*` suite
- Device template tests, including `SignalApis_SelectLogicalContext` and
`PutValue_SelectLogicalContext`
- MPI regression
`Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode` (Anvil, 1
node / 2 ranks, `NCCL_GIN_TYPE=7`, delayed second barrier)

## Test Result

Anvil SDMA is `NCCL_GIN_TYPE=7` (`NCCL_NET_DEVICE_GIN_ANVIL_SDMA`).
Earlier runs in this section were recorded when the enum was 6.

Pre-rebase (commit `970a014315`):

- `rccl-UnitTestsGinAnvilPlugin`: 20/20 passed on MI355
- `GinAnvilSdmaTemplateTest.*`: 13/13 passed on MI355 (before
`PutValue_SelectLogicalContext` was added)
-
`GinMPIDeviceTests.Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode`
on MI355: **PASSED** (7779 ms) on `smci355-ccs-aus-m03-17`, 1 node / 2
ranks, `NCCL_GIN_ENABLE=1`, `NCCL_CUMEM_ENABLE=1`. Ran
`rccl-UnitTestsMPI` inside `rccl-gin-pr11556-rs:latest` with
`--ipc=host`. Host `mpirun` on the same node segfaulted in HIP IPC
during communicator bring-up (`ncclP2pImportShareableBuffer` /
`ncclDevrCommCreateInternal`) while other GPU docker jobs held
`/dev/kfd` and `/dev/dri`; that path never reached the kernel.
- Same MPI filter on MI350: **PASSED** (8834 ms) on
`cv350-rck-g03-e23-08`, 1 node / 2 ranks, same GIN env. Host `mpirun -np
2`; GPUs were idle. Used the gfx950 `rccl-UnitTestsMPI`/`librccl.so.1`
from the MI355 PR build (native CentOS 9 link of `librccl.so` failed on
`librocshmem_gin_host.a` bitcode encoding; Docker was unavailable on
that node).

Current head `cf8cf9720f`, rebased onto `develop` at `482e541856`,
retested from a clean worktree on MI355X (`smci355-ccs-aus-m03-17`), 1
node / 2 ranks, `NCCL_GIN_TYPE=7`, `NCCL_GIN_ENABLE=1`,
`NCCL_CUMEM_ENABLE=1`, `NCCL_GIN_NCONTEXTS=4`:

- `rccl-UnitTestsGinAnvilPlugin`: **37/37 passed** (304 ms), including
the full `ConnCheck_*` suite and the two span-scope cases added for the
ARGUS finding. Both were confirmed to fail against `8eafd3d752` (3
allgathers at `nContexts=3`) and pass here (1).
- Whole `GinMPIDeviceTests.*` suite on the Anvil backend: **44 passed,
20 skipped, 0 failed** (40.1 s). Covers
`Barrier_AllContextsConsecutiveSignalsDoNotAlias_SingleNode` (680 ms),
all seven `BarrierFence_*_SingleNode` cases including
`AllContextsPut`/`AllContextsGet`,
`MultiContext_AllFourRoute`/`NonPowerOf2`/`Exclusive`, and
`Put_BasicAndOffsets_MultiContext`/`PutValue_Inline_MultiContext`/`WaitCounterAndSignal_MultiContext`.
- The 20 skips are topology or opt-in gated, not regressions: cross-node
and 4-rank cases, `RailConnection_Create`, the `VASignal_*` cases (VA
signals are unsupported on SDMA by design), and the pool tests that
require `NCCL_GIN_SIGNAL_POOL_SIZE>=0x40000000`.

An earlier attempt at this run skipped all 64 cases because
`NCCL_CUMEM_ENABLE` was unset, which trips the `Symmetric memory
required` gate; the numbers above are from the corrected invocation.

Earlier post-rebase run on `f32754f6c1`, same node and env, before
#12450/#12451 were folded in: plugin **37/37**, `GinMPIDeviceTests.*`
**39 passed, 23 skipped, 0 failed** (34.5 s).

Environment note: the host ROCm on that node is 7.0.2.1 while its KFD
driver is newer, and `hipIpcOpenMemHandle` segfaults during plain P2P
transport setup (`ncclP2pImportShareableBuffer`) before any GIN code
runs. The runs above use the node's ROCm 7.13 runtime in a container
with `--ipc=host`, where the same binaries pass. Not related to this
change; the same workaround was needed for the pre-rebase runs.

## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant