Skip to content

fix(rccl): honor abortFlag in DDA waits so revoke can complete - #11245

Merged
alex-breslow-amd merged 31 commits into
developfrom
users/atulkulk/fix/rccl-dda-abort-on-revoke
Oct 21, 2026
Merged

alex-breslow-amd merged 31 commits into
developfrom
users/atulkulk/fix/rccl-dda-abort-on-revoke

Conversation

@atulkulk

@atulkulk atulkulk commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

ncclCommRevoke and ncclCommAbort set abortFlag so in-flight kernels can drain. Generic device primitives already sample that flag, but the standalone DDA kernels did not.

On gfx1250, an incomplete DDA collective (for example, a rank that never launches its AllReduce) left the other ranks spinning on peer flags or barrier mailboxes that would never arrive. Revoke and abort then hung in stream sync (RevokeMPITest.IncompleteCollective_Revoke_Shrink_Collective). DDA IPC on gfx942/gfx950 has the same shape in IpcGpuBarrier.

Technical Details

Every DDA wait now polls comm->abortFlagDev, at the same cadence as checkAbort (kDdaSpinsBeforeCheckAbort, in the new dda_kernel_params.h). This covers the LL and LL128 flag waits in AllReduce (one-shot and two-shot), AllGather, ReduceScatter and AlltoAll, and the fabric and IPC GPU barriers. Launchers pass a dda::common::DdaKernelParams by value to the kernels, wait helpers and barriers. It holds the abort flag today, and later per-launch values go there rather than as extra kernel arguments.

On abort a wait gives up rather than returning early from the kernel where that would skip shared work: polls break so the LL epoch update and __syncthreads still run. The LL two-shot AllReduce uses a sticky abort, skipping the remaining waits once the flag is seen, because an early return after each unrolled wait nearly doubled the 8-rank kernel and slowed the single-node two-shot path. A thread that has aborted does not publish its phase-2 sums, which are partial, so a peer that has not seen the abort cannot accept them as the reduced shard.

IpcGpuBarrier and FabricGpuBarrier keep a sticky aborted_ for the rest of the kernel. Once a signaling lane gives up a put or a wait, the block signals no later barrier, so a late peer cannot pair its earlier flag with this block's next one and read a two-shot shard this rank never finished. The signaling lanes all sit in wave 0, so they share the result through __any; FabricGpuBarrier uses __syncthreads_or when the clique is wider than one wave. This replaces a load of the pinned-host abort flag before every put.

DDA stays enabled when revoke is used, because revoke has to stop kernels that are already running. A collective issued on a revoked comm is rejected: DDA, CE 2-shot and GIN-SDMA return from the collective before ncclEnqueueCheck, so rcclCollectiveMustUseEnqueuePath now sends a revoked comm through enqueue, which returns ncclInvalidUsage as nccl.h documents. Without this, a collective on the revoked parent would run DDA again on mailboxes left with unconsumed flags.

A parent comm shrunk with NCCL_SHRINK_ABORT stops using DDA (comm->ddaDisabledAfterAbort, checked in rcclDdaEnabled). The abort can leave ranks with different LL epochs and unconsumed barrier mailbox slots, so a later DDA collective on that parent could hang or return a peer's stale data. The shrunk child does not share the parent's resources and is not affected.

Tests (the MPI cases are in RevokeMPITests.cpp):

  • RevokeDdaMPITest.IncompleteCollective_* (11 cases, gfx1250): one rank skips a DDA collective, the rest revoke and shrink. They cover LL and LL128 one-shot and two-shot AllReduce, the VMM barrier, and LL/LL128 AllGather, ReduceScatter and AlltoAll. Each case first confirms through rcclGetCollImplInfo that the intended DDA path is selected, and after the revoke checks that the same collective on the revoked parent returns ncclInvalidUsage.
  • RevokeDdaIpcMPITest.IncompleteCollective_DdaIpc* (5 cases, gfx942/gfx950, 8 ranks): the same pattern for DDA IPC AllReduce (flat and the three-barrier tree kernel), AllGather, ReduceScatter and AlltoAll.
  • AbortDdaMPITest.IncompleteCollective_* (5 cases): the same pattern with ncclCommAbort, for LL one-shot and two-shot AllReduce, and AllGather, ReduceScatter and AlltoAll LL.
  • ShrinkAbort_DisablesDdaOnParent in both RevokeDdaMPITest and RevokeDdaIpcMPITest: after NCCL_SHRINK_ABORT, the parent no longer selects DDA and the child's AllReduce succeeds.
  • DdaIpcGpuBarrierOrdering.AbortedBarrierStaysSilent (one GPU): a barrier that gave up on abort must not signal the next one after late peers drain the first. It fails with the sticky aborted_ removed.
  • Rcclwrap.EnqueueGuards_RevokedForcesEnqueue: a revoked comm takes the enqueue path.
  • The DDA fabric launcher microtests check that comm->abortFlagDev reaches the kernel; the mock comm now carries a non-null flag so the check can fail.

Launch, abort and stream-drain results are voted across ranks, and post-revoke waits are bounded, so a failure on one rank fails every rank instead of hanging. The test runner configs for MI300, MI355X and MI455 include the new cases.

Known gaps

These are out of scope for this PR and tracked as follow-ups:

  • ncclCommRevoke and NCCL_SHRINK_ABORT with a stuck DDA kernel. Both set abortFlag, sync only hostStream and deviceStream, then clear the flag (commRevokeAsync and ncclCommShrink). DDA kernels run on the user stream, which deviceStream is not ordered after outside graph capture, so a stuck kernel that does not poll the flag inside that window keeps spinning. In practice the window is long enough for the poll (every kDdaSpinsBeforeCheckAbort spins), which is why the revoke tests pass, but it is not guaranteed. Native single-stream kernels and NCCL upstream have the same window. ncclCommAbort leaves the flag set and is not affected.
  • DDA stays off on a parent shrunk with NCCL_SHRINK_ABORT. Turning it back on needs a cross-rank resync of the LL epochs and barrier mailboxes.
  • Graphs captured before the abort. The DDA gate applies at enqueue, so a graph that captured a DDA kernel before the abort still replays it.
  • Kernels that start after the flag is cleared. A DDA kernel that is queued behind other user-stream work and only starts after ncclCommRevoke or NCCL_SHRINK_ABORT clears abortFlag never sees the abort, and hangs if a peer is missing.

Issue Tracking

JIRA ID: ROCM-33096 (gfx1250, DDA fabric), ROCM-32381 (gfx942, DDA IPC).

Test Plan

Run each RevokeDdaMPITest case in its own process, as the runner configs do. In one process the first case caches RCCL_DDA_LL and the LL sub-thresholds, so later MpiEnvGuards have no effect and some cases skip. AbortDdaMPITest.* sets no overrides and can run in one process.

  • mpirun -np 4 ./rccl-UnitTestsMPI --gtest_filter=<case> for each RevokeDdaMPITest, RevokeMPITest and RevokeNonBlockingMPITest case in the revoke_mpi_tests_4rank group (for example --gtest_filter=RevokeDdaMPITest.IncompleteCollective_DdaVmmBarrier), and --gtest_filter='AbortDdaMPITest.*'.
  • The same MPI cases at 8 ranks across two nodes (mpirun -np 8 --host <nodeA>:4,<nodeB>:4 ...).
  • mpirun -np 8 ./rccl-UnitTestsMPI --gtest_filter=RevokeDdaIpcMPITest.<case>, one per process, on 8 gfx942/gfx950 GPUs, plus the revoke, shrink, abort and grow suites of mi300x_mellanox_ib.json.
  • ./rccl-UnitTestsFixtures --gtest_filter='DdaIpcGpuBarrierOrdering.*' and ./rccl-UnitTestsFixturesDebug --gtest_filter='Rcclwrap.EnqueueGuards_*'.
  • Host microtests: rccl-UnitTestsMicroDdaColl and rccl-UnitTestsMicroDda.
  • rccl-tests with data checking (-c 1) for AllReduce, AllGather, ReduceScatter and AlltoAll on one and two nodes, plus two-node AlltoAll and ReduceScatter with RCCL_DDA_LL128_THRESHOLD=67108864. NCCL_DEBUG_SUBSYS=COLL logs confirm the DDA path at each size.
  • rccl-tests performance against the develop base on one and two nodes: the four collectives from 1 KB to 64 MB (32 MB on two nodes), plus AllReduce forced onto the LL two-shot kernel (RCCL_DDA_LL_ONESHOT_THRESHOLD=0).

Test Result

gfx942, DDA IPC (8x MI300X, one node, ROCm 7.13.0 rc2, DebugFast, 7bb78ac42da):

  • The revoke, shrink, abort and grow suites of mi300x_mellanox_ib.json: 62 pass, 2 fail and 19 skip on this hardware, including every RevokeDdaIpcMPITest.IncompleteCollective_* case and DdaIpcGpuBarrierOrdering.*. RevokeDdaIpcMPITest.ShrinkAbort_DisablesDdaOnParent, added after that run, passes on its own. The 2 failures are SymAbort_Kernel.LLAllToAll_AbortReleasesKernel and SymAbort_Kernel.LsaBarrier_AbortReleasesKernel, which time out in communicator setup on develop as well.
  • Rcclwrap.EnqueueGuards_* passes. With the sticky barrier abort or the revoked-comm divert reverted, AbortedBarrierStaysSilent, EnqueueGuards_RevokedForcesEnqueue and the IPC AllReduce revoke cases fail.
  • Host microtests: rccl-UnitTestsMicroDda passes 51 of 51. rccl-UnitTestsMicroDdaColl passes 266; 5 LL128 eligibility tests fail on develop as well, after Tuning Infra Improvement #12386 removed the RCCL_DDA_LL gate from LL128 eligibility.
  • With the barrier fix applied on cc2700e596d, rccl-tests -c 1 reports 0 wrong values for all four collectives on DDA IPC, and performance against cc2700e596d is within noise from 1 KB to 8 MB (3 reps).

gfx1250, DDA fabric (4 GPUs per node, ctheliosp-rck-g02-j11-12 alone and with j11-11, one MNNVL fabric clique):

  • At 65410fcff30 (the barrier fix, before the revoked-comm divert and the new tests), 80 of 86 cases in the revoke, shrink and abort suites pass on one and two nodes. Of the 6 failures, 3 two-node cases did not start (ORTE was unable to reliably start one or more daemons), 2 are the SymAbort_Kernel timeouts above, and 1 is a stale SymCheckMode_Registration filter name in the mi455 config on develop. rccl-tests -c 1 reports 0 wrong values in every run that completed.
  • 7bb78ac42da is not yet tested on gfx1250: the nodes moved to a new ROCm build mid-run and are now in use. Results to follow.
  • Earlier measurements against develop dfb3f8c9e28 (median latency, 2% threshold with non-overlapping ranges):
    • At 134defe6cc6, AllReduce forced onto the LL two-shot kernel is 3–6% slower (about 1–1.4 µs) from 64 KB to 2 MB, on one and two nodes. At default thresholds this kernel is used at 2 MB only.
    • Two-node ReduceScatter on DDA fabric LL is 2–3.4% slower from 32 KB to 2 MB; on DDA fabric LL128 it is 5–7% faster at 4–8 MB and about 4% faster at 16–32 MB.
    • Everything else is within noise, apart from single-point 3% differences: one-node AllReduce 2 KB slower, two-node AllGather 32 KB faster and AlltoAll 2 KB slower.
    • With the two-shot review fixes (8fecb7a64bb), the LL two-shot AllReduce is within -1.5% to +0.2% of 134defe6cc6 at every size from 64 KB to 2 MB, on one node (4 ranks, 5 reps) and two nodes (8 ranks, 3 reps).

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Sep 3, 2026 •

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/algorithms/dda/all_gather/all_gather_dda_fabric_ll.h, projects/rccl/src/algorithms/dda/all_gather/all_gather_dda_fabric_ll128.h, projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_fabric_ll128.h, projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll.h, projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll128.h (+33 more); 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?

@therock-pr-bot

therock-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@atulkulk
atulkulk force-pushed the users/atulkulk/fix/rccl-dda-abort-on-revoke branch 2 times, most recently from e6a2877 to eb8e08a Compare September 10, 2026 16:27
@atulkulk
atulkulk marked this pull request as ready for review September 10, 2026 16:57
@atulkulk
atulkulk requested review from a team and a lite review from Copilot September 10, 2026 16:57

Copilot AI 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.

🟡 Changes recommended

The new DDA revoke MPI test currently queries HIP device properties for device 0 instead of the rank’s assigned/current device, which can cause incorrect skip/pass behavior under non-trivial device assignment.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR ensures ncclCommRevoke can reliably unblock in-flight DDA collectives by sampling comm->abortFlagDev inside DDA LL/LL128 wait loops and inside the Fabric/IPC GPU barriers, then adds MPI tests/config entries to specifically exercise the DDA LL one-shot and two-shot paths on gfx1250.

Changes:

  • Add a shared device helper (dda_abort.h) and thread comm->abortFlagDev through DDA LL/LL128 kernels and wait primitives to break out of peer-flag spins when revoked.
  • Update DDA Fabric/IPC GPU barrier implementations to poll abortFlagDev during CAS-based waits.
  • Add new MPI revoke+shrink tests that assert DDA LL one-shot/two-shot path selection via log “needles”, and register them in MI455 test-runner configs.
File summaries
File Description
projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce.json Register new DDA revoke MPI test cases in the MI455 RoCE suite config.
projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce_func.json Register new DDA revoke MPI test cases in the MI455 RoCE functional suite config.
projects/rccl/test/RevokeMPITests.cpp Add RevokeDdaMPITest cases that force DDA LL one-shot/two-shot selection and then revoke+shrink.
projects/rccl/src/CMakeLists.txt Add new DDA abort header to the source list.
projects/rccl/src/algorithms/dda/device/dda_abort.h Introduce device-side abort-flag polling helpers for DDA kernels/barriers.
projects/rccl/src/algorithms/dda/device/CollCommon.h Add LL line wait helper that aborts based on abortFlagDev.
projects/rccl/src/algorithms/dda/device/CollCommon_ll128.h Add LL128 flag/poll helpers that abort based on abortFlagDev.
projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll.h Thread abort flag into LL reduce-scatter waits and exit cleanly on revoke.
projects/rccl/src/algorithms/dda/reduce_scatter/reduce_scatter_dda_fabric_ll128.h Thread abort flag into LL128 reduce-scatter waits and exit cleanly on revoke.
projects/rccl/src/algorithms/dda/reduce_scatter/dda_reduce_scatter_fabric_ll.cu Pass comm->abortFlagDev into LL reduce-scatter kernel launches.
projects/rccl/src/algorithms/dda/reduce_scatter/dda_reduce_scatter_fabric_ll128.cu Pass comm->abortFlagDev into LL128 reduce-scatter kernel launches.
projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll.h Thread abort flag into LL all-reduce waits and exit cleanly on revoke.
projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_ll_twoshot.h Thread abort flag into LL two-shot all-reduce waits and exit cleanly on revoke.
projects/rccl/src/algorithms/dda/all_reduce/all_reduce_dda_fabric_ll128.h Thread abort flag into LL128 all-reduce waits and exit cleanly on revoke.
projects/rccl/src/algorithms/dda/all_reduce/dda_all_reduce_fabric_ll.cu Pass comm->abortFlagDev into LL all-reduce (one-shot + two-shot) launches.
projects/rccl/src/algorithms/dda/all_reduce/dda_all_reduce_fabric_ll128.cu Pass comm->abortFlagDev into LL128 all-reduce launches.
projects/rccl/src/algorithms/dda/all_gather/all_gather_dda_fabric_ll.h Thread abort flag into LL all-gather waits to avoid hangs on revoke.
projects/rccl/src/algorithms/dda/all_gather/all_gather_dda_fabric_ll128.h Thread abort flag into LL128 all-gather waits/polls to avoid hangs on revoke.
projects/rccl/src/algorithms/dda/all_gather/dda_all_gather_fabric_ll.cu Pass comm->abortFlagDev into LL all-gather launches.
projects/rccl/src/algorithms/dda/all_gather/dda_all_gather_fabric_ll128.cu Pass comm->abortFlagDev into LL128 all-gather launches.
projects/rccl/src/algorithms/dda/alltoall/alltoall_dda_fabric_ll.h Thread abort flag into LL all-to-all waits to avoid hangs on revoke.
projects/rccl/src/algorithms/dda/alltoall/alltoall_dda_fabric_ll128.h Thread abort flag into LL128 all-to-all waits to avoid hangs on revoke.
projects/rccl/src/algorithms/dda/alltoall/dda_alltoall_fabric_ll.cu Pass comm->abortFlagDev into LL all-to-all launches.
projects/rccl/src/algorithms/dda/alltoall/dda_alltoall_fabric_ll128.cu Pass comm->abortFlagDev into LL128 all-to-all launches.
projects/rccl/src/algorithms/dda/ipc/ipc_gpu_barrier.h Add abort-aware spinning to IPC GPU barrier flag CAS loops.
projects/rccl/src/algorithms/dda/ipc/ipc_gpu_barrier.cu Plumb abort flag through IPC barrier construction.
projects/rccl/src/algorithms/dda/ipc/ipc_init.cu Pass comm->abortFlagDev into IPC barrier init.
projects/rccl/src/algorithms/dda/fabric/fabric_gpu_barrier.h Add abort-aware spinning to fabric GPU barrier flag operations.
projects/rccl/src/algorithms/dda/fabric/fabric_gpu_barrier.cu Plumb abort flag through fabric barrier construction.
projects/rccl/src/algorithms/dda/fabric/fabric_init.cu Pass comm->abortFlagDev into fabric barrier init.
Review details
  • Files reviewed: 30/30 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved

@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

Bottom line: Worth settling before approval: new abortFlag wait-loop coverage is AllReduce-LL only, leaving LL128/AllGather/AllToAll/ReduceScatter/barrier abort paths untested.

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

@atulkulk
atulkulk force-pushed the users/atulkulk/fix/rccl-dda-abort-on-revoke branch from b4b375d to dc248f8 Compare September 13, 2026 07:14

@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 fall outside the diff and could not be anchored to a line
  • projects/rccl/CHANGELOG.md:23 (Low) No CHANGELOG entry for a user-visible revoke hang fix

Bottom line: The device-side abort plumbing traces clean on every path checked. Worth settling before approval: the two new tests still cover only AllReduce LL, so every other converted wait loop stays unexercised.

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

Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

  • Add new commits
  • Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

@github-actions github-actions Bot added the Stale PR has no activity for 25+ days label Oct 9, 2026
@atulkulk
atulkulk force-pushed the users/atulkulk/fix/rccl-dda-abort-on-revoke branch from dc248f8 to bf55109 Compare October 12, 2026 21:30
@github-actions github-actions Bot removed the Stale PR has no activity for 25+ days label Oct 13, 2026
@amd-argus

amd-argus Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@cppccppccppc @sunguobao PTAL

@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

Bottom line: Worth settling before approval: the DDA barrier's abort exit leaves CAS mailbox flags set and nothing re-zeroes them (fabric_gpu_barrier.h:94). All seven earlier threads are still open.

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

Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
@atulkulk

Copy link
Copy Markdown
Contributor Author

Re the Argus review summary (CHANGELOG): added a "Resolved issues" entry under RCCL 2.32.3 for the abort/revoke hang in DDA collectives, in 3166b4c.

@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

This PR changes 1,067 lines across 39 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: Worth settling before approval: kDisableLL also turns off the LL128 tier, so the three new LL128 cases always skip and those kernels get no coverage.

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

Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved
Comment thread projects/rccl/test/RevokeMPITests.cpp Resolved

@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

This PR changes 1,139 lines across 38 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.

Bottom line: Worth settling before approval: the new abortFlag_ member and 3-argument mailbox calls in ipc_gpu_barrier.h break the rccl-UnitTestsFixtures build (DdaIpcGpuBarrierOrderingTests.cpp:52 static_assert, 2-argument calls at :93-106).

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

@atulkulk
atulkulk force-pushed the users/atulkulk/fix/rccl-dda-abort-on-revoke branch from 44ef618 to 134defe Compare October 19, 2026 20:57
rcclGetCollImplInfo reports the LL128 tier, not the one-/two-shot
variant, so lowering RCCL_DDA_LL128_ONESHOT_THRESHOLD below the test
size made IncompleteCollective_DdaLl128OneShot rerun the two-shot kernel
and left the one-shot kernel's abort wait without a case. Skip unless
the threshold is at its default.
The ddaLaunch checks in the DDA revoke/shrink and abort helpers were
rank-local ASSERT_EQs. A launch failure on one rank returned from the
test on that rank alone while its peers carried on into the next
MPI_Barrier or hipStreamSynchronize and hung. Collect each launch
result, default it to success on ranks that do not launch, and check it
with ASSERT_MPI_EQ so every rank fails together. For the child launch,
vote before the stream sync so the surviving ranks do not block waiting
on a peer that never launched.
…tests

After ncclCommRevoke or ncclCommAbort the DDA revoke and abort helpers
called hipStreamSynchronize directly. If abortFlag failed to break a DDA
wait, the kernel spun forever and the job hung with no diagnostic. Use
DdaFabricMPI::waitStreamBounded with abortJobUnlessAllRanksDrained so a
stuck kernel or a sticky stream error is reported and the MPI job is
aborted on every rank.
ncclCommAbort was checked with a rank-local ASSERT_EQ. A failure on one
rank returned from the test while its peers blocked in the all-rank
drain vote that follows. Check it with ASSERT_MPI_EQ, as the other
revoke and grow abort tests do, so every rank fails together.
…All polls

The abort exit in the fabric LL/LL128 AllGather and AlltoAll gather
loops is a break rather than the ddaSetLLEpoch-and-return used
elsewhere. Note why at each site: the poll is the outermost loop, so
the epoch bump below still runs, and in the LL128 AllGather every thread
must reach the trailing __syncthreads.
An early return per unrolled wait bloated the kernel; skip the remaining waits instead and still bump the LL epoch.
An abort can leave ranks with different DDA LL epochs and unconsumed
barrier mailbox slots, so a later DDA collective on the parent could hang
or return a peer's stale data. Disqualify DDA on that comm in
rcclDdaEnabled; restoring DDA there needs a cross-rank resync.
DDA kernels, wait helpers and the Fabric/IPC GPU barriers took
comm->abortFlagDev as a bare const uint32_t*. Wrap it in DdaAbortSignal,
passed by value, with isSet() and spinTick() replacing ddaAbortFlagSet and
ddaAbortSpinTick, so the abort state has its own type at every launch and
wait site.

The struct holds one pointer, so the kernel ABI is unchanged.
With the sticky abort, a thread that gave up waiting in phase 2 still
published acc0/acc1 to every peer under the current epoch flag, with zeros
in place of the missing peers. A peer that had not aborted yet and already
held its phase-1 inputs accepted those partial sums in phase 3 and finished
with wrong data. Skip the publish once aborted; peers then either read a
correct reduced shard or keep waiting until they abort too.
DdaFabricMPITestFixture.hpp, DdaReduceScatterLL128MPITests.cpp and
DdaFabricSimpleMPITests.cpp each had their own copy, two of them still
querying device 0. Use DdaMPITestHelpers::isGfx1250Device(), which checks
the current device.
Every lane of ddaLL128PollWire loads the same abort flag in the same
instruction, so the lanes already leave the loop together without
__any().
Equivalent to the early return inside the loop, and reads like the other
DDA wait helpers.
A barrier wait that gave up on abort let the kernel signal its next
barrier anyway. A late peer that was not aborted could then pair its
earlier flag with that signal, pass the barrier, and read a shard this
rank never finished, returning ncclSuccess with wrong data.

Keep a sticky aborted_ flag in IpcGpuBarrier and FabricGpuBarrier,
shared across the signaling lanes with a wave vote, and skip every later
signal in the kernel once it is set. Fabric cliques larger than a wave
fall back to __syncthreads_or.
Per review, make the per-launch DDA kernel argument a general parameter
struct so later values can be added without another kernel argument.
dda_abort.h becomes dda_kernel_params.h, the kernel and barrier members
become params / params_, and the methods become aborted() and
abortSpinTick(). No behavior change.
…cannot run

DDA, CE 2-shot and GIN-SDMA return from the collective impl before
ncclEnqueueCheck, which is where a revoked comm is rejected. Once revoke
can complete with DDA barrier flags left unconsumed, a collective issued
on the revoked parent would run DDA again and could pass a barrier early.
Divert revoked comms through enqueue so they get ncclInvalidUsage.
…nd the revoked parent

- DdaIpcGpuBarrierOrdering.AbortedBarrierStaysSilent fails if a barrier
  that gave up on abort still signals the next one.
- RevokeDdaIpcMPITest.IncompleteCollective_DdaIpcAllReduceTree reaches the
  three-barrier tree kernel; ShrinkAbort_DisablesDdaOnParent now also runs
  on the 8-rank DDA IPC parent.
- The DDA revoke helper checks that the revoked parent rejects the same
  DDA collective with ncclInvalidUsage.
- The DDA fabric mock comm carries a non-null abortFlagDev, so the
  launcher tests can tell the flag was forwarded.
@atulkulk
atulkulk force-pushed the users/atulkulk/fix/rccl-dda-abort-on-revoke branch from 7bb78ac to 43304ae Compare October 20, 2026 21:38
@alex-breslow-amd
alex-breslow-amd merged commit ced131f into develop Oct 21, 2026
67 of 73 checks passed
@alex-breslow-amd
alex-breslow-amd deleted the users/atulkulk/fix/rccl-dda-abort-on-revoke branch October 21, 2026 17:40
pvallem added a commit that referenced this pull request Oct 22, 2026
Resolve against #11245 (abortFlag in DDA waits): both LL128 one-shot poll
sites use the abort-aware ddaLL128PollWire and update the LL epoch before
returning, the word-per-lane LL128 kernel uses ddaLL128WaitFlag, and the
test drops its copies of the two-shot helpers now in DdaMPITestHelpers.hpp.
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

pragupta pushed a commit to ROCm/pytorch that referenced this pull request Nov 4, 2026
Skip due to NCCL defect. Fix:
ROCm/rocm-systems#11245

(cherry picked from commit 64843f1)
Signed-off-by: pablo-garay <pablo-garay@users.noreply.github.com>
pragupta pushed a commit to ROCm/pytorch that referenced this pull request Nov 4, 2026
Skip due to NCCL defect. Fix:
ROCm/rocm-systems#11245

(cherry picked from commit 64843f1)
Signed-off-by: pablo-garay <pablo-garay@users.noreply.github.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.

6 participants