fix(rccl): don't record doneEvent on a possibly-destroyed stream - #10576
Open
pvallem wants to merge 1 commit into
Open
fix(rccl): don't record doneEvent on a possibly-destroyed stream#10576pvallem wants to merge 1 commit into
pvallem wants to merge 1 commit into
Conversation
PR #6130 moved the doneEvent record out of ncclLaunchKernel into ncclLaunchPrepare's stream-change branch, where it runs on comm->lastStream. That turned lastStream from a compare-only value into a dereferenced handle, but RCCL never learns when the application destroys a stream: HIP has no destruction notification and no safe validity probe, and lastStreamValid was only ever set true. An application that destroys the stream a communicator last launched on and then issues a collective on a different stream therefore hits a use-after-destroy. It surfaces either as ncclUnhandledCudaError ('invalid resource handle') or, when the freed handle has been recycled onto an unrelated live stream, as a bogus ordering edge and a segfault. Record doneEvent at launch time again, on launchStream, which is live by construction. lastStream becomes an identity token stored as (uintptr_t)stream + 1, with 0 meaning "no launch yet" so a prior launch on the default stream stays distinguishable. Comparing a stale token is safe: hipStreamDestroy defers handle reuse until the stream's work completes, so a recycled handle implies the prior kernel already finished. Folding the two fields into one word also removes any torn-read question between them. This reopens the ~3us per-launch cost #6130 removed; recovering it by fusing the record into the launch via hipExtModuleLaunchKernel is tracked separately. Refs: ROCM-29677 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
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
PR #6130 moved the
hipEventRecord(comm->doneEvent, ...)out ofncclLaunchKernelandinto
ncclLaunchPrepare's stream-change branch, recording it oncomm->lastStreamrather than on the stream being launched.
That turned
lastStreamfrom a compare-only value into a dereferenced handle. RCCLnever learns when the application destroys a stream — HIP has no destruction notification
and no safe validity probe — and
lastStreamValidwas only ever settrue, nevercleared. So an application that destroys the stream a communicator last launched on, and
then issues a collective on a different stream, hits a use-after-destroy.
It surfaces two ways:
ncclUnhandledCudaErrorfromHIP failure: 'invalid resource handle'producing a bogus ordering edge
This is not theoretical — it reproduces on a real QA workload (see Test Result).
Technical Details
Record
doneEventat launch time again, onlaunchStream, which is live by construction:the application has just handed it to us.
ncclLaunchPreparenow only waits on the event,never records it.
lastStream+lastStreamValidcollapse into a singleuintptr_t lastStreamTag, producedby
ncclStreamTag(s) = (uintptr_t)s + 1so0stays free as the "no launch yet" sentineland a prior launch on the default stream remains distinguishable (the case #5800 fixed).
One word also removes any torn-read question between a handle and a separate flag.
Comparing a stale tag is safe in both directions:
hipStreamDestroydefers handle reuse until the stream's work completes, so a recycled handle proves the
prior kernel already finished.
doneEvent, recorded on thatstream while it was alive. The event is comm-owned and outlives the stream.
Typing it as
uintptr_tmakes the unsafe use unrepresentable rather than relying on acomment to prevent recurrence.
JIRA ID
ROCM-29677
Test Plan
hipStreamDestroy(A)-> create decoy streams toforce handle divergence -> allreduce on a new stream B, same comm.
DoneEventOrdering.StreamDestroyedThenSwitch, run against both a patchedand an unpatched RCCL to confirm it actually catches the defect.
concurrent_collectives weekly(8x MI350X, 1000 iterations, 256 MB),which creates 6 streams per datatype phase and destroys them while the 6 communicators
span all three phases.
librcclswapped viaLD_PRELOAD.rccl-tests all_reduce_perfA/B for the performance impact.Test Result
Targeted repro — same binary, same HIP (
/opt/rocm/lib/libamdhip64.so.7), only librccl swapped:invalid resource handle, exit 3phase2 OK -- NO CRASH, exit 0New unit test — validated in both directions:
StreamDestroyedThenSwitchHIP failure: 'invalid resource handle'Real workload, 4 runs per arm, alternating:
Note the defect is intermittent at the workload level (it only fires when the replacement
stream draws a different handle than the destroyed one), which is why the deterministic
repro above uses decoy streams to force the condition.
Affected versions — the bug is present in every RCCL from 2.28.9 onward:
Consistent with the dates: #6130 merged 2026-05-15; the 2.28.9 bump (
46931872a0) landed2026-05-18. ROCm 10.0.0rc3 ships this defect.
Performance Impact
This restores the per-launch
hipEventRecordthat #6130 removed, and that cost is real.all_reduce_perf -b 8 -e 8K -f 8 -n 1000 -w 200, idle node, median of 5 alternating repsper arm. All 20 runs exited 0 with
#wrong = 0on every size row; none discarded.rccl-tests reports one time per op while issuing
-g NncclAllReducecalls, so the per-opdelta scales with N;
delta/Gis the implied per-launch cost. It lands in the same2.5-3.1 us band at both GPU counts, averaging 2.76 us per launch — matching the figure
#6130 cited.
This is a knowing tradeoff: a crash is being exchanged for a latency regression that
#6130 was created to remove. Recovering it is tracked as an immediate follow-up — fusing
the record into the launch via
hipExtModuleLaunchKernel, which accepts both the packedextrabuffer this code needs and astopEvent, exactly as RCCL did viahipExtLaunchKernel's 8th argument before #3741. That restores the ordering edge at zeroextra host calls. Reviewers who care about the #6130 workloads (ROCM-21756, ROCM-24157)
should track that follow-up.
Submission Checklist
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
🤖 Generated with Claude Code