test(rccl): add host-only test for GIN proxy NUMA affinity pin by mch · Pull Request #12475 · ROCm/rocm-systems

mch · 2026-09-29T17:39:43Z

Adds host-only unit-test coverage for the GIN proxy-thread NUMA affinity pin (upstream NCCL commit 80e715f91f8, "Pin GIN proxy thread to NUMA-local CPU set"), which re-applies comm->cpuAffinity at the top of ncclGinProgress so the GIN progress thread is NUMA-pinned like the traditional proxy thread instead of inheriting the caller's affinity after init restored its temporary pin. src/gin/gin_host.cc previously had no host coverage, so nothing guarded this behaviour.

The test (gin-proxy-affinity-test.cc) compiles the real gin_host.cc into rccl-UnitTestsMicroInit as a second unit under test. It pre-sets proxyThreadStopSignal so ncclGinProgress applies the affinity and returns at the top of its loop without touching any backend, devComm, or the network — no GPU, no plugin, no librccl. It asserts via the os_fakes recorders that a non-empty stashed mask pins the thread exactly once and an empty mask is a no-op.

Because init.cc (the binary's other unit under test) drives the GIN entry points gin_host.cc defines through fakes, the three colliding symbols (ncclGinHostFinalize, ncclGinQueryLastError, ncclGinSetDefaultBackend) are renamed with a Uut suffix immediately before the include, so init.cc's calls still bind to the fakes while gin_host.cc's copies become distinct, uncalled symbols that -Wl,--gc-sections drops.

Testing: run_host_tests.sh all passes; proven red/green by reverting the fix in the hipified tree (the non-empty case fails); llvm-cov confirms the two added lines are covered.

JIRA ID : AICOMRCCL-1859

…y pin NCCL 2.30.7 / NVIDIA nccl PR #2182 ("Pin GIN proxy thread to NUMA-local CPU set", commit 80e715f) re-applies comm->cpuAffinity at the top of ncclGinProgress so the GIN progress thread is NUMA-pinned like the traditional proxy thread, rather than inheriting whatever affinity the caller had after init restored the temporary pin. gin_host.cc had no host coverage, so nothing guarded this behaviour. Add gin-proxy-affinity-test.cc, compiled into rccl-UnitTestsMicroInit as a second unit under test alongside init.cc. It pre-sets proxyThreadStopSignal so ncclGinProgress applies the affinity and returns at the top of its loop without touching any backend, devComm, or the network -- no GPU, no plugin, no librccl. It asserts on the os_fakes affinity recorders: - a non-empty stashed mask is applied to the proxy thread exactly once, with the stashed cpu set; - an empty mask leaves the thread affinity untouched (the guard). init.cc (the binary's other UUT) drives the GIN entry points gin_host.cc DEFINES through fakes, so compiling the real gin_host.cc here would duplicate ncclGinHostFinalize / ncclGinQueryLastError / ncclGinSetDefaultBackend. The test renames just those three with a Uut suffix immediately before #include GIN_HOST_CC_PATH, so init.cc's calls still bind to the fakes while gin_host.cc's own copies become distinct, uncalled symbols that -Wl,--gc-sections drops. This avoids a dedicated binary for the file. Proven red/green by reverting the fix in the hipified tree (the non-empty case fails), the full init suite still passes, and llvm-cov confirms the two added lines are covered. Co-authored-by: Pi Agent

therock-pr-bot · 2026-09-29T17:40:20Z

✅ 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?

amd-argus

ARGUS

Bottom line: Worth settling before approval: GinProxyAffinityTest resets the os fakes only in SetUp, so it leaks recorder state into whichever InitMicrotest case gtest shuffles in next.

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

…ffinity suite Add a TearDown that resets the os fakes so the suite cannot leak global state into later shuffled suites, and add a positive ncclOsCpuCount anchor to the empty-affinity case so it proves the guard actually ran. Correct the include-time rename comment (ncclGinSetDefaultBackend is not defined in gin_host.cc) and drop the dead define/undef. Register the suite in test_categories_micro_init.yaml, name gin_host.cc in the Init source-list header and README, and add a file-header describing the unit under test. Co-authored-by: Pi Agent

mch · 2026-09-29T19:16:56Z

@argus re-review

amd-argus

ARGUS

Bottom line: All six round-1 findings are fixed. LGTM once the header comment notes that the mask stash at gin_host.cc:393 is out of scope and the new file is reindented to the repo's 2-space clang-format.

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

… 2-space indent Add GinProxyAffinitySetupTest, which drives the file-static ginDevCommSetupWithBackend through a scripted GIN backend so the branch that stashes comm->cpuAffinity into ginState (and spawns the progress threads) is exercised. Without it, that assignment could be deleted with the consumer cases still green. Stub ncclTeamRail and ncclSetThreadName locally, the two symbols the now-live setup path reaches that no other TU in the Init binary provides. Reindent the file to 2 spaces to match the majority of test/host sources, and register GinProxyAffinitySetupTest in the CTest category yaml and README. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: Close to ready. Worth a look before approval: GinProxyAffinitySetupTest never frees the ncclGinStateDevComm the setup path allocates.

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

…onsumes the pin Free the ncclGinStateDevComm that ginDevCommSetupWithBackend allocates and links into ginState->devComms, in TearDown so it runs even when an assertion returns early; this stops the leak the ASAN arm would report and also exercises FakeDestroyContext. Extend the setup test to join the spawned proxy worker and assert it pinned itself to the stashed comm mask, covering the full produce -> spawn -> consume handoff on a real thread. Record in the README how gin_host.cc's ncclGinHostFinalize/ncclGinQueryLastError avoid a duplicate symbol against the gin/nccl fakes kept in the same target. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: LGTM.

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

The GIN proxy-affinity tests rode in rccl-UnitTestsMicroInit, which forced gin_host.cc's ncclGinQueryLastError/ncclGinHostFinalize to be renamed to *Uut at include time so they could coexist with the gin_fakes.cc/nccl_stubs.cc doubles init.cc needs. Move them into rccl-UnitTestsMicroGinHost instead: the rename hack goes away, init's binary is byte-for-byte what it was before this branch, and gin_host.cc's coverage is attributed to a binary of its own. Also document the naming rule the new binary follows, so the next unit does not have to invent one: src/<dirs>/<stem>.cc -> <STEM>_CC_PATH -> <stem>-test.cc -> rccl-UnitTestsMicro<Stem> -> test_categories_micro_<stem>.yaml, with every gtest suite in the TU prefixed <Stem> so the yaml needs a single pattern and a new suite cannot be silently dropped. gin-proxy-affinity-test.cc is renamed to gin-host-test.cc and its suites to GinHost*Microtest accordingly. Wired into every place that enumerates the host binaries: the binaries array in run_host_tests.sh, the config + test_suites entries in ci-precheckin.json, and the coverage --object list in test_executor.py. The new target's source list is the shortest here (test TU + nccl_fakes.cc + os_fakes.cc + main): --gc-sections drops everything the covered paths never reach, so the usual stub floor is unreferenced; a fakes file goes in when the linker asks for it. Verified with test/host/run_host_tests.sh all: 4760 tests across 11 binaries pass, gin_host.cc coverage reports under rccl-UnitTestsMicroGinHost, and the overall line/branch union is unchanged. Co-authored-by: Pi Agent

…vious Drop the commentary that restates the code it sits above, in the GIN test, its CMake source list and its categories yaml, and drop MICROTEST_README's per-binary inventory and standalone binary listing: both duplicated CMakeLists.txt and went stale the moment a target changed. Repoint the three references the removed text left dangling: "Adding a new test" pointed at a deleted checklist, the standalone section opened on "three of those names" with no list above it, and the only record of test_executor.py's coverage --object list went with the checklist, so a new binary would silently lose coverage attribution. The naming section now names all four places in one line. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: LGTM.

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

…omrccl-1859-gin-affinity

amd-argus

ARGUS

Bottom line: LGTM.

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

…omrccl-1859-gin-affinity # Conflicts: # projects/rccl/test/host/MICROTEST_README.md

amd-argus

ARGUS

Bottom line: LGTM.

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

…omrccl-1859-gin-affinity

amd-argus

ARGUS

Bottom line: LGTM.

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

…omrccl-1859-gin-affinity Conflict in projects/rccl/test/host/MICROTEST_README.md: develop added a `ras/ras_param.cc` bullet to the "Units under test" section that this branch replaces with "Naming a new microtest". Kept this branch's section; develop's note about the renamed `ras_param.cc` symbols survives in the fakes table.

…anch deleted This branch had replaced the "Units under test" section with a new "Naming a new microtest" section, and had collapsed the two per-binary lists in "Standalone host-only build" down to a single run_host_tests.sh invocation. All three are lists that every microtest PR edits, so deleting them turned each of those PRs into a merge conflict here. Over the last 90 days develop touched this README 35 times: 7 of those commits edited the standalone-build enumeration and 9 edited the ./build/ run list. Restore all three byte-identically to develop and keep only the additive part: the "Naming a new microtest" table, placed ahead of "Units under test", plus two cross-references from "Adding a new test". The README diff against develop is now +29/-1 and touches no line another PR is likely to edit. The gin_host.cc binary this PR adds is documented as the worked example in the naming table rather than by appending to the three inventories. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: LGTM.

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

…omrccl-1859-gin-affinity Resolve the gin_host microtest conflict with #12485 (GIN_PROXY_NTHREADS) by keeping every GinHostTest case from develop and adding this branch's three proxy-affinity tests on top: - gin-host-test.cc: take develop's TU wholesale; extend its in-TU ncclOsCpuCount /ncclOsSetAffinity seams to return a settable CPU count and record the masks (mutex-guarded, written from progress threads), then append GinHostProxyAffinityMicrotest and GinHostProxyAffinitySetupMicrotest. The branch's fakes/os_fakes.cc route is dropped: it would duplicate the symbols develop's TU already owns. - Drop this branch's duplicate rccl-UnitTestsMicroGinHost source list, target, JUnit XML entry and test_categories_micro_ginhost.yaml; develop's _gin_host-named equivalents are the single registration. The categories yaml pattern widens back to GinHost* so new suites in the TU are picked up. - ci-precheckin.json / MICROTEST_README.md: keep develop's unit_tests_micro_gin_host naming, describe both the nthreads and affinity coverage. Verified with test/host/run_host_tests.sh all: 3544 tests pass, develop's 3541 plus exactly the three new affinity tests; lines 58.1%, branches 49.2%. Co-authored-by: Pi Agent

amd-argus

ARGUS

Bottom line: LGTM.

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

…-only tests PR #12475 added the first host-only coverage of `src/gin/gin_host.cc` (the GIN proxy NUMA affinity pin) and PR #12245 the GIN_PROXY_NTHREADS progress-thread behaviour, which together left the file at 59% of lines and 34% of branches. This builds out the rest, so every function in the file now has a test. The test TU's `ncclGin_t` fake grows a uniform `ScriptedFailure` per plugin entry point ("fail call N of this function"), recorders for the `ginConfig`s and `regMrSym`/`deregMrSym` arguments, and knobs for a NULL-returning `createContext` and a `queryLastError` that reports an error on a chosen call. That makes each failure arm reachable without a bespoke fake per test. The same mechanism now also covers `ncclTopoGetLocalGinDevs` and `bootstrapAllGather`, and the rail team's stride is a knob so the stride-validation branches are reachable. New suites, one per entry point: `GinHostTypeQueryMicrotest`, `GinHostSignalRequestMicrotest`, `GinHostConnectOnceMicrotest`, `GinHostBackendSelectMicrotest`, `GinHostDevCommSetupMicrotest`, `GinHostDevCommFreeMicrotest`, `GinHostFinalizeMicrotest`, `GinHostRegisterMicrotest` and `GinHostQueryLastErrorMicrotest`. They all start with `GinHost`, so the existing `GinHost*` CTest pattern picks them up without touching the categories yaml beyond its description. Coverage of `gin_host.cc` in `rccl-UnitTestsMicroGinHost` goes from 59.01% to 100% of lines, 66.67% to 100% of functions, and 33.79% to 74.43% of branches; no source-level branch is left uncovered, the remainder being `NCCLCHECK`/`WARN` macro expansions at call sites whose failure arm no test drives. Testing: `run_host_tests.sh all` passes (80 GinHost tests, shuffled); ten mutations of the hipified `gin_host.cc` -- dropping the NULL-window check, the multi-segment DMABUF requirement, the stride-multiple check, the empty-slot skip, the early return on a reported error, the context round-up, the connectivity check in `ncclGetGinType`, the VA-signal requirement check, the devComm reset on failure, and the fail-path `closeListen` -- each turn exactly the matching test red. Co-authored-by: Pi Agent

Adds host-only unit-test coverage for the GIN proxy-thread NUMA affinity pin (upstream NCCL commit `80e715f91f8`, "Pin GIN proxy thread to NUMA-local CPU set"), which re-applies `comm->cpuAffinity` at the top of `ncclGinProgress` so the GIN progress thread is NUMA-pinned like the traditional proxy thread instead of inheriting the caller's affinity after init restored its temporary pin. `src/gin/gin_host.cc` previously had no host coverage, so nothing guarded this behaviour. The test (`gin-proxy-affinity-test.cc`) compiles the real `gin_host.cc` into `rccl-UnitTestsMicroInit` as a second unit under test. It pre-sets `proxyThreadStopSignal` so `ncclGinProgress` applies the affinity and returns at the top of its loop without touching any backend, devComm, or the network — no GPU, no plugin, no `librccl`. It asserts via the `os_fakes` recorders that a non-empty stashed mask pins the thread exactly once and an empty mask is a no-op. Because `init.cc` (the binary's other unit under test) drives the GIN entry points `gin_host.cc` defines through fakes, the three colliding symbols (`ncclGinHostFinalize`, `ncclGinQueryLastError`, `ncclGinSetDefaultBackend`) are renamed with a `Uut` suffix immediately before the include, so `init.cc`'s calls still bind to the fakes while `gin_host.cc`'s copies become distinct, uncalled symbols that `-Wl,--gc-sections` drops. Testing: `run_host_tests.sh all` passes; proven red/green by reverting the fix in the hipified tree (the non-empty case fails); `llvm-cov` confirms the two added lines are covered. JIRA ID : AICOMRCCL-1859

systems-assistant · 2026-10-12T14:06:17Z

TheRock Submodule Bump Activity

Newest first

2026-10-12 — Included in TheRock via ROCm/TheRock#8932.

…13225) Brings `src/gin/gin_host.cc` to full host-only test coverage in `rccl-UnitTestsMicroGinHost`, and tidies the surrounding microtest infrastructure the work exposed. PR #12475 added the first host-only coverage of this file (the GIN proxy NUMA affinity pin) and PR #12245 the `GIN_PROXY_NTHREADS` progress-thread behaviour, which together left it at 59% of lines and 34% of branches. This builds out the rest: every function in the file now has a test, across nine new suites (one per entry point) covering type query, signal requests, connect-once, backend select, devComm setup/free, finalize, register and last-error query. Coverage of `gin_host.cc` goes from 59.01% to 100% of lines, 66.67% to 100% of functions, and 33.79% to 74.43% of branches; no source-level branch is left uncovered, the remainder being `NCCLCHECK`/`WARN` macro expansions at call sites whose failure arm no test drives. The test TU's `ncclGin_t` fake gains a uniform `ScriptedFailure` per plugin entry point ("fail call N of this function") plus argument recorders, so each failure arm is reachable without a bespoke fake per test. `FakeGin` hands out tagged handles rather than relying on a file-scope singleton, so tests that hold more than one context stay independent. Four reusable utilities that had accumulated inside the test TU are promoted to shared headers for the other host-only suites: `ScopeExit.h`, `ScriptedFailure.h`, `WaitUntil.h` and `JoinThreadsOnExit.h`. Externals the suite had been defining locally move onto the fakes file named for the production TU that defines each, per `MICROTEST_README.md`: `bootstrapAllGather` to `fakes/bootstrap_stubs.cc`, `ncclOsCpuCount`/`ncclOsSetAffinity` to `fakes/os_fakes.cc`, `ncclSetThreadName` to `fakes/nccl_fakes.cc`, and the `ncclTeam*` accessors to a new `fakes/nccl_device_core_fakes.cc`. Each moved seam picks up `ASSERT_HOOK_MATCHES_PROD` signature-drift checking from its new home. Testing: `run_host_tests.sh all` passes — all 13 host-only binaries green, 87 `GinHost` tests, shuffled. Ten mutations of the hipified `gin_host.cc` (dropping the NULL-window check, the multi-segment DMABUF requirement, the stride-multiple check, the empty-slot skip, the early return on a reported error, the context round-up, the connectivity check in `ncclGetGinType`, the VA-signal requirement check, the devComm reset on failure, and the fail-path `closeListen`) each turn exactly the matching test red. JIRA ID : AICOMRCCL-2642

mch requested review from a team September 29, 2026 17:39

github-actions Bot added the project: rccl label Sep 29, 2026

systems-assistant Bot added the organization: ROCm label Sep 29, 2026

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread projects/rccl/test/host/gin-proxy-affinity-test.cc Resolved

Comment thread projects/rccl/test/host/gin-proxy-affinity-test.cc Resolved

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread projects/rccl/test/host/gin-proxy-affinity-test.cc Resolved

Comment thread projects/rccl/test/host/gin-host-test.cc Resolved

Comment thread projects/rccl/test/host/MICROTEST_README.md Resolved

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

mch added 2 commits October 2, 2026 16:40

mch commented Oct 2, 2026

View reviewed changes

Comment thread projects/rccl/test/host/MICROTEST_README.md Resolved

amd-argus Bot reviewed Oct 2, 2026

View reviewed changes

pvallem approved these changes Oct 2, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/mahughes/aic…

c649c30

…omrccl-1859-gin-affinity

amd-argus Bot reviewed Oct 2, 2026

View reviewed changes

dhanavikram approved these changes Oct 6, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/mahughes/aic…

c0ad667

…omrccl-1859-gin-affinity # Conflicts: # projects/rccl/test/host/MICROTEST_README.md

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

aduforest approved these changes Oct 6, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/mahughes/aic…

c9ac627

…omrccl-1859-gin-affinity

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

mch added 2 commits October 8, 2026 15:26

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

mch merged commit 7b2ecf7 into develop Oct 9, 2026
73 of 75 checks passed

mch deleted the users/mahughes/aicomrccl-1859-gin-affinity branch October 9, 2026 15:22

mch mentioned this pull request Oct 10, 2026

test(rccl): cover every gin_host.cc entry point with host-only tests #13225

Merged

Conversation

mch commented Sep 29, 2026

Uh oh!

therock-pr-bot Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

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!

mch commented Sep 29, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

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!

amd-argus Bot left a comment

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

systems-assistant Bot commented Oct 12, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

edited

Loading