test(rccl): cover KernelCh profiling on the per-communicator profiler thread by i-kosarev · Pull Request #12715 · ROCm/rocm-systems

i-kosarev · 2026-10-02T20:50:54Z

Motivation

NCCL 2.31.2 moved KernelCh profiling to a dedicated per-communicator thread, extending kernel timing to proxy-less NVLink/SHM and graph-captured collectives. The port is in develop, but nothing exercised it beyond a smoke check that some KernelCh events appear. This PR adds the tests; no library code changes.

Technical Details

Test-only profiler plugin test/plugin/profiler_kernelch_recorder.{h,cpp}, built against the standalone v7 headers and installed beside rccl-UnitTestsMPI. It records Coll, P2p, KernelCh and KernelPhase events with the threads that delivered them.
ProfilerKernelChMPITests.cpp, 13 tests. Each checks that every Coll/P2p gets exactly nChannels KernelCh children, each stopped once with a timer, all on the dedicated profiler thread and never on the caller or a proxy thread. Covered: proxy-less intra-node, SHM, the proxy path, send/recv, graph replays, eager/graph mixing, a burst past the 64-slot ring, split communicators, abort, timer units, and KernelPhase from the symmetric kernels.
Four categories in test_categories_mpi.yaml, since the transport and cuMem variables are cached per process. All set RCCL_DDA_ENABLE=0.

Known gaps, not fixed here: DDA collectives raise no profiler events at all (to be tracked as an enablement task), and graphUsageMode defaults to 0 in RCCL, which misattributes KernelCh timings under eager/graph mixing (docs fixed in #13033).

Issue Tracking

JIRA ID: AICOMRCCL-2011

Test Plan

Debug builds, the four quick_env_profiler_kernelch* categories on one node, plus 2-node runs.

Test Result

GPU	ROCm	Runs	Result
gfx942 (MI300X)	7.0.1	8 ranks, SHM, network on one node, 2 nodes	pass; KernelPhase test skips (cuMem init crashes on that host, as for `SymWin_AllReduce`)
gfx950 (MI355X)	10.1.0	8 ranks x3, SHM, network on one node, 2 ranks; KernelPhase test at 8 and 2 ranks	pass

The suite fails on 8-rank gfx950 without RCCL_DDA_ENABLE=0. The mixing test skips, with the reason, when NCCL_GRAPH_MIXING_SUPPORT=0 in the environment turns mixing off.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

therock-pr-bot · 2026-10-02T20:56:51Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 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?

Copilot

Copilot review overview

🟢 Approval recommended

The test implementation, plugin ABI integration, resource lifecycle, and category configuration are internally consistent.

Review effort: Balanced
Findings: None

What changed in this PR

Adds comprehensive MPI coverage for KernelCh profiling on dedicated per-communicator threads.

Changes:

Adds a standalone recorder profiler plugin.
Adds 12 tests covering transports, graphs, bursts, communicator splits, aborts, and timers.
Adds dedicated test categories and build/install integration.

File	Description
`projects/rccl/test/ProfilerKernelChMPITests.cpp`	Adds KernelCh MPI tests and shared validation.
`projects/rccl/test/plugin/profiler_kernelch_recorder.h`	Defines the recorder query interface.
`projects/rccl/test/plugin/profiler_kernelch_recorder.cpp`	Implements the thread-safe profiler recorder.
`projects/rccl/test/test_categories_mpi.yaml`	Adds isolated KernelCh test environments.
`projects/rccl/test/CMakeLists.txt`	Builds, installs, and wires in the recorder and tests.

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

amd-argus

ARGUS

Findings

3 finding(s) that could not be posted as inline comments

projects/rccl/test/plugin/profiler_kernelch_recorder.cpp:203 (Low) Snapshot builds the full parent index map before honouring cap == 0, so the 1 ms size probe rehashes everything under the lock rcclKchRecorderSnapshot builds the parent index map before the cap == 0 break at line 209, so waitForDrain's 1 ms size probe (test/ProfilerKernelChMPITests.cpp:198) rehashes every record while holding gMutex, throttling the profiler thread it is waiting on. Could we return early when cap is 0?
projects/rccl/test/ProfilerKernelChMPITests.cpp:135 (Low) TearDown releases neither the device buffers nor the streams, so any fatal assertion mid-body leaks them TearDown() releases neither the device buffers nor the streams the bodies create, and freeBuffers() is only ever a body's last statement, so a fatal ASSERT after allocBuffers() leaks both allocations for the rest of the process. Could we free them here?
projects/rccl/test/ProfilerKernelChMPITests.cpp:410 (Low) checkBalanced's KernelChStats is discarded in three tests, dropping the task-count assertion their siblings make checkBalanced's KernelChStats is discarded here and at line 474 and line 747, so those three never assert the task count that lines 385, 437, 582 and 618 do, and a run recording one of eight collectives still passes. Was that deliberate?

This PR changes 1,135 lines across 5 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: a broken recorder .so turns all 12 tests into green skips (ProfilerKernelChMPITests.cpp:122), and the rank-local skip at line 153 hangs the ranks that succeeded.

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

Copilot

Copilot review overview

🟡 Changes recommended

Split-communicator invariants and the abort test’s in-flight-work premise are not fully validated.

Review effort: Balanced
Findings: 2

Open (2)

Initial parent/child batch skips channel event balance validation · New
Abort timing test does not ensure operations are still pending · New

amd-argus

ARGUS

This PR changes 1,135 lines across 5 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: both Medium findings from the last round are unchanged, a broken recorder .so still yields 12 green skips (ProfilerKernelChMPITests.cpp:122), and the rank-local skip at line 153 still blocks the ranks that succeeded.

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

i-kosarev · 2026-10-08T01:20:03Z

The three findings from the first review that couldn't be posted inline are addressed in 545f1e8: rcclKchRecorderSnapshot returns before building the index when cap is 0; TearDown frees the buffers and the owned stream; and the proxy-less, SHM and timer tests now assert the task count like the others.

amd-argus

ARGUS

This PR changes 1,253 lines across 5 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: checkBalanced's stopTimer/startTimer check at ProfilerKernelChMPITests.cpp:325 contradicts the burst test's own slot-reuse disclaimer, so that test can fail with no regression present.

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

amd-argus

ARGUS

This PR changes 1,287 lines across 5 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: LGTM.

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

Copilot

🟡 Changes recommended

Setup failures can deadlock ranks, and RCCL recorder-load failures are incorrectly reported as skips.

1 open finding

Communicator setup can deadlock before failure coordination · New

2 resolved since last review

Abort timing test does not ensure operations are still pending
Initial parent/child batch skips channel event balance validation

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

amd-argus

ARGUS

This PR changes 1,504 lines across 5 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: LGTM.

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

ncclCommWindowRegister is collective. Short-circuiting the second call after a failed first one let that rank skip a registration its peers were blocked in, before the failure was agreed on. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

This PR changes 1,505 lines across 5 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: LGTM.

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

Copilot

🟡 Changes recommended

The burst test can miss absent stop timestamps, and inherited graph-mixing configuration can deterministically invalidate the mixing test.

1 open finding

Pin NCCL_GRAPH_MIXING_SUPPORT for graph mixing tests · New

1 resolved since last review

Avoid short-circuiting collective window registrations

Previously missed (1)

In code that hasn't changed since last review

Require nonzero stop timer even with slot reuse

projects/rccl/test/ProfilerKernelChMPITests.cpp:365

slotReuse disables the only assertion that observes stopTimer, so the burst test can pass even if every KernelCh stop state carries a missing/zero timestamp. Slot reuse can invert start/stop ordering, but it should not remove the timer itself; require a nonzero stop timer unconditionally and relax only the ordering comparison.

🧠 Review effort: Balanced

…f; require a stop timer NCCL_GRAPH_MIXING_SUPPORT overrides the config's graphUsageMode, so an inherited 0 ran the unsupported eager/graph mix and failed. The test now checks the effective mode after init and skips on every rank, saying why. Pinning the variable in the category instead would move every other test in that process off the default mode. checkBalanced only looked at stopTimer through the ordering check, which slot reuse disables, so a stop state without a timestamp went unnoticed in the burst test. A nonzero stop timer is now required in every case. Co-authored-by: Cursor <cursoragent@cursor.com>

i-kosarev · 2026-10-14T13:16:10Z

Re the "previously missed" finding (require a nonzero stop timer even with slot reuse): fixed in be9b210. checkBalanced now requires a nonzero stopTimer on every KernelCh, and slotReuse only relaxes the stop-after-start ordering. On gfx942 (8 ranks) the suite, including the 512-op burst, passes; on gfx950 (ROCm 10.1.0, NCCL_CUMEM_ENABLE=1) the symmetric test passed 5/5 at 8 ranks and 5/5 at 2 ranks.

amd-argus

ARGUS

This PR changes 1,510 lines across 5 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: LGTM.

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

Copilot

🟡 Changes recommended

Rank-local graph launch failures can desynchronize collective execution and hang the MPI tests.

1 open finding

Coordinate graph launch failures before stream synchronization · New

1 resolved since last review

Pin NCCL_GRAPH_MIXING_SUPPORT for graph mixing tests

🧠 Review effort: Balanced

Copilot

🔵 Needs a closer look

Hardware-dependent timing assertions and MPI abort lifecycles warrant final human validation despite no confirmed blocking defects.

0 open findings

1 resolved since last review

Coordinate graph launch failures before stream synchronization

🧠 Review effort: Balanced

…nelch-profiler-thread-tests Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/test/CMakeLists.txt

Copilot

🔵 Needs a closer look

Distributed GPU concurrency, graph replay, and abort behavior warrant final human review despite no identified blocking findings.

0 open findings

🧠 Review effort: Balanced

With runtime connect, the first captured collective sets up connections with its peers, so a rank whose hipStreamBeginCapture failed would leave them waiting in it. The ranks now agree that capture began before any of them enqueues work. checkBalanced only kept KernelCh off the caller and the proxy threads. On a graph replay the Coll and P2p events come from HIP's host callback thread, so KernelCh delivered on that thread passed. KernelCh must now avoid every thread that delivered a Coll or P2p event. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

This PR changes 1,529 lines across 5 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: LGTM.

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

alex-breslow-amd

LGTM

systems-assistant · 2026-10-22T19:12:56Z

TheRock Submodule Bump Activity

Newest first

2026-10-22 — Included in TheRock via ROCm/TheRock#9425.

github-actions Bot added the project: rccl label Oct 2, 2026

i-kosarev added this pull request to stack #12716 October 2, 2026 20:51

i-kosarev requested review from mch and thomas-huber and a balanced review from Copilot October 2, 2026 21:05

Copilot started reviewing on behalf of i-kosarev October 2, 2026 21:06

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

systems-assistant Bot added the organization: ROCm label Oct 2, 2026

i-kosarev marked this pull request as ready for review October 2, 2026 21:23

i-kosarev requested review from a team October 2, 2026 21:23

amd-argus Bot reviewed Oct 2, 2026

View reviewed changes

i-kosarev requested a balanced review from Copilot and removed request for mch and thomas-huber October 5, 2026 17:11

Copilot started reviewing on behalf of i-kosarev October 5, 2026 17:12

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

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

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

i-kosarev mentioned this pull request Oct 7, 2026

docs(rccl): document that graph mixing is off by default #13033

Open

1 task done

i-kosarev force-pushed the users/ilkosare/kernelch-profiler-thread-tests branch from 2480c6b to d72bce6 Compare October 7, 2026 16:45

i-kosarev mentioned this pull request Oct 7, 2026

fix(rccl): let NCCL_P2P_DISABLE / NCCL_P2P_LEVEL override the CPU defaults #12714

Merged

1 task done

amd-argus Bot reviewed Oct 7, 2026

View reviewed changes

i-kosarev force-pushed the users/ilkosare/kernelch-profiler-thread-tests branch from d72bce6 to 948faab Compare October 7, 2026 23:30

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

i-kosarev requested a balanced review from Copilot October 8, 2026 11:10

Copilot started reviewing on behalf of i-kosarev October 8, 2026 11:11

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

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

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

i-kosarev requested a balanced review from Copilot October 13, 2026 22:16

Copilot started reviewing on behalf of i-kosarev October 13, 2026 22:17

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread projects/rccl/test/test_categories_mpi.yaml Resolved

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

i-kosarev requested a balanced review from Copilot October 14, 2026 15:31

Copilot started reviewing on behalf of i-kosarev October 14, 2026 15:32

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

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

i-kosarev requested a balanced review from Copilot October 14, 2026 18:39

Copilot started reviewing on behalf of i-kosarev October 14, 2026 18:44

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

i-kosarev requested a review from BertanDogancay October 14, 2026 21:16

Merge remote-tracking branch 'origin/develop' into users/ilkosare/ker…

ecbab13

…nelch-profiler-thread-tests Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/test/CMakeLists.txt

i-kosarev requested a balanced review from Copilot October 15, 2026 07:14

Copilot AI reviewed Oct 15, 2026

View reviewed changes

Copilot started reviewing on behalf of i-kosarev October 15, 2026 07:39

View session

BertanDogancay reviewed Oct 20, 2026

View reviewed changes

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

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

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

i-kosarev requested a review from BertanDogancay October 21, 2026 02:25

BertanDogancay approved these changes Oct 21, 2026

View reviewed changes

i-kosarev requested a review from alex-breslow-amd October 21, 2026 14:06

alex-breslow-amd approved these changes Oct 21, 2026

View reviewed changes

i-kosarev merged commit e073e2c into develop Oct 21, 2026
68 of 73 checks passed

i-kosarev deleted the users/ilkosare/kernelch-profiler-thread-tests branch October 21, 2026 14:45

Conversation

i-kosarev commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Oct 2, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

i-kosarev commented Oct 8, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

i-kosarev commented Oct 14, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

i-kosarev commented Oct 2, 2026 •

edited

Loading