[i-kosarev]

Motivation

The NetIbMPI validation suite (rccl-UnitTestsMPI) previously exercised the
NET/IB transport through one host worker per MPI rank. This change adds
coverage for the safe multi-worker model used by rccl-tests -t: independent
transport contexts and connections driven concurrently within one process.

Two things are intentionally out of scope. Concurrent calls on one NET/IB
communicator: NCCL communicators are reentrant but not thread-safe, and the
NET/IB send/receive communicator contains shared FIFO, request, and
work-request state, so such a test would exercise an unsupported contract
rather than validate rccl-tests behavior. Concurrent plugin
init/finalize: the plugin keeps process-global device tables and a
reference count, so worker contexts are created and destroyed by the main
thread. This change is therefore confined to the test suite and needs no
transport code changes.

Technical Details

• Adds --net_ib_nthreads=N to rccl-UnitTestsMPI. The default is 1;
  positive values above 16 clamp to 16; malformed or conflicting values
  fail before GTest starts. Conflicts are detected on the values as written,
  before clamping, so two distinct out-of-range requests are rejected rather
  than silently collapsing to the same capped value. All MPI ranks verify that
  they resolved the same effective value.
• Preserves the original single-threaded paths for N=1.
• Adds a main-thread control plane for the threaded path: the main thread
  creates every worker context and connection, synchronizes setup failures
  across ranks before data transfer, and begins teardown only after all
  workers complete.
• Adds a two-stage worker start gate. Worker bodies do not call MPI or GTest;
  they return ThreadResult, which the main thread reports after joining.
• Covers the independent worker model through SimpleSendRecv and
  MultipleSequentialTransfers, with runner configurations at 2, 4, and 16
  workers per rank. Every other NetIB test ignores the option and keeps its
  single-worker behavior.
• Asserts that all N workers are concurrently inside the body, not merely
  that two overlap. The start/body gate makes that peak deterministic, so the
  exact bound also catches a regression in the gate itself; a scheduler that
  serialized the workers fails the test rather than reporting false coverage.
• Checks RDMA resource counts around the threaded paths to catch QP, CQ, MR,
  or PD leaks. Counts are scoped to the test process by PID with no
  system-wide fallback, so an unrelated process on a shared node cannot
  perturb the comparison and both snapshots are always taken the same way.
• Guards the host-VMM registration tests on HIP SDKs that do not declare
  hipMemLocationTypeHost, so unsupported runtimes skip only that scenario.
• Documents the option, the threading model it reproduces, and the concurrency
  deliberately left untested as a multithread-validation module of the
  feature-unit-testing skill.

No public RCCL API or ABI changes are introduced.

Issue Tracking

JIRA ID: AICOMNET-323

Test Plan

Hardware: mia1, AINIC/Pensando with RoCE on rdma0..rdma3, gfx950, host
build.

• Rebuild rccl-UnitTestsMPI in a debug configuration with MPI tests enabled.
• Run the independent worker matrix at
  --net_ib_nthreads=2, 4, and 16.
• Verify CLI behavior:
  • --net_ib_nthreads=99 clamps to 16 and passes;
  • --net_ib_nthreads=abc is rejected;
  • conflicting duplicate values are rejected.
• Run the complete NetIB transport configuration through isolated mpirun
  processes.

The functional acceptance target for this change is Basic coverage of the
independent-context lifecycle and data paths. Branch-coverage measurement of
the new failure-control paths remains follow-up work.

Test Result

On mia1-p01-g25 ↔ mia1-p01-g27, one rank per node with eight GPUs available
per node:

• Threaded matrix at 2, 4, and 16 workers per rank:
  6 PASS / 0 FAIL / 0 HANG / 0 SKIP / 0 ERROR.
• Full NetIB configuration: 339 PASS / 0 FAIL / 0 HANG / 6 SKIP / 2 ERROR
  (347 total).
• A single-node repeat with both ranks on mia1-p01-g30 gives the same totals
  and the same non-passing tests.
• The two errors are the established baseline failures:
  NetIB_CtsDepthStress (MPI_Cancel abort, rc=7) and
  Cast_MaxQPCount128 (SIGSEGV, rc=139).
• The six skips are capability/configuration preconditions:
  RapidConnectDisconnect for NET/IB and IB-CAST plus four WRR scheduler
  tests that require a longer scheduler update interval.
• CLI behavior confirmed: --net_ib_nthreads=99 clamps to 16 and passes,
  --net_ib_nthreads=abc is rejected before GTest, and conflicting duplicate
  values are rejected.

Re-verified after the review-fix commit on mia1-p01-g24 ↔ mia1-p01-g25,
one rank per node. This run covers the changed paths, not the full suite; the
totals above still refer to the complete configuration run on the parent
commit.

• Threaded matrix at 2, 4, and 16 workers per rank with the exact-peak
  fan-out assertion and the PID-scoped RDMA counting:
  6 PASS / 0 FAIL / 0 HANG / 0 SKIP / 0 ERROR.
• CLI parser matrix, five cases: 99 100 rejected as conflicting, 99 99
  and 99 clamp to 16 and pass, 2 4 rejected as conflicting, abc
  rejected as malformed.
• Negative test for the clamp-ordering fix: rebuilt with the pre-fix parser,
  --net_ib_nthreads=99 --net_ib_nthreads=100 is accepted and the test
  passes; with the fix it is rejected. The reported defect reproduces.
• rdma resource show on this cluster emits no pid field for kernel-owned
  objects, so the previous filter matched nothing and every snapshot used the
  system-wide fallback path. Confirmed directly on the node, which is what
  motivated removing the fallback rather than adjusting the match.

Submission Checklist

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

[therock-pr-bot]

✅ 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/test/common/MPIEnvironment.hpp, projects/rccl/test/common/main_mpi.cpp, projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp, projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp; 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]

🚫 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.

[Copilot]

Pull request overview

Adds CLI-controlled multithreading support to the NetIbMPI transport validation tests, enabling two stress modes (independent per-thread connections and shared-connection concurrent ops) and wiring new test-runner suites to exercise them; also includes a HIP feature-detection build fix for older runtimes.

Changes:

• Add --net_ib_nthreads=N parsing (stripped before GTest) and store it on MPIEnvironment::nThreads.
• Introduce multithread execution helpers in NetIbMPITestBase.hpp and update existing NetIbMPI tests to run single-threaded by default, multithreaded when nThreads>1.
• Add test-runner suites for the new multithread coverage and add a CMake/HIP compile-time feature guard for hipMemLocationTypeHost.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
projects/rccl/tools/scripts/test_runner/configs/net_ib_transport.json	Adds new multithread NetIb transport suites using --net_ib_nthreads=4.
projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp	Adds thread fan-out helpers and per-thread init/connect/teardown utilities.
projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp	Parameterizes key NetIb send/recv tests to run in multithread modes.
projects/rccl/test/transport/NetIbMPI/CastTests.cpp	Extends a CAST large-transfer test to run concurrently on a shared connection.
projects/rccl/test/RegistrationMPITests.cpp	Adds compile-time guard handling when hipMemLocationTypeHost is unavailable.
projects/rccl/test/common/MPIEnvironment.hpp	Adds global MPIEnvironment::nThreads setting (default 1).
projects/rccl/test/common/main_mpi.cpp	Parses/strips --net_ib_nthreads= before MPI/GTest initialization.
projects/rccl/test/CMakeLists.txt	Defines RCCL_HAS_HIP_MEM_LOCATION_TYPE_HOST when feature detection succeeds.
projects/rccl/CMakeLists.txt	Adds check_cxx_source_compiles probe for hipMemLocationTypeHost.

---

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

[github-actions]

RCCL Perf-Regression Gate: ⚠️ NO VERDICT (not measured)

This run did not produce a usable answer. It is neither a PASS nor a regression — treat the perf gate as not run for this change.

Why:

• this report predates the trustworthiness flag, or a group could not be scored

Mode: detect (reference vs candidate)
Thresholds: small 17.2% · mid 12.3% · large 12.9%
Keys compared: 0 · Confirmed regressions: 0 · Inconclusive: 0
Provenance: n/a

Per-collective breakdown

group	keys	regressions	inconclusive
all_gather_perf-d=bfloat16-default	0	0	0
all_gather_perf-d=float-default	0	0	0
all_reduce_perf-d=bfloat16-default	0	0	0
all_reduce_perf-d=float-default	0	0	0
broadcast_perf-d=bfloat16-default	0	0	0
broadcast_perf-d=float-default	0	0	0
reduce_scatter_perf-d=bfloat16-default	0	0	0
reduce_scatter_perf-d=float-default	0	0	0

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:704

• SkipUnlessMpiThreadMultiple() says to “Skip instead”, but it records an ADD_FAILURE() and returns, which turns unsupported MPI builds into hard test failures rather than clean skips. That also contradicts the PR description’s intent to avoid undefined behavior/hangs when MPI_THREAD_MULTIPLE isn’t granted.

    // Both RunMultiThreaded* helpers issue MPI calls (barriers, handle
    // exchange) from worker threads, which is only legal at
    // MPI_THREAD_MULTIPLE. MPIHelpers::initializeMPI() requests that level but
    // an MPI build may grant less, in which case those calls are undefined
    // behavior and typically manifest as an unexplained hang. Skip instead.
    bool SkipUnlessMpiThreadMultiple() {
        if (MPIEnvironment::mpiThreadSupport >= MPI_THREAD_MULTIPLE) return false;
        ADD_FAILURE() << "multithread mode requires MPI_THREAD_MULTIPLE ("
                      << MPI_THREAD_MULTIPLE << "), but MPI provided "
                      << MPIEnvironment::mpiThreadSupport

projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:755

• On rank 0, if CreateListenCommCtx() fails, pair.handle may contain uninitialized bytes but is still sent via MPI_Send. This can trigger UB/sanitizer findings and makes failures harder to debug; initialize the handle before the possible failure path.

        ThreadResult result;
        if (rank == 0) {
            if (CreateListenCommCtx(ctx, dev, &pair.handle, &pair.listenComm) != ncclSuccess) {
                result.ok = false;
                result.msg = "CreateListenComm failed";

projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:405

• In the multithreaded (nThreads>1) path, the loop reuses the same tag for every message size (threadIdx*kThreadTagStride+100). Since WaitForCompletion() only waits on the local request, one rank can potentially advance and post a new op with the same tag while the peer is still completing the prior one, risking tag/request collisions. Make the tag unique per iteration (e.g., include size or the loop index).

        ThreadResult result;
        for (size_t size : testSizes) {
            const int tag = threadIdx * kThreadTagStride + 100;
            const int seed = threadIdx * 100000 + 2000 + static_cast<int>(size);

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:855

• tag passed as mpiTag to MPI_Send/MPI_Recv scales with kThreadTagStride (10000). With --net_ib_nthreads clamped to 16, this can produce MPI tags up to 150000, which may exceed MPI_TAG_UB on some MPI implementations (the standard minimum is 32767). Use a separate small MPI tag namespace for the handle exchange (e.g., 1000 + threadIdx) so multithread mode stays portable.

            const int tag = threadIdx * kThreadTagStride;
            ThreadResult setupResult = SetupConnectionForThread(ctx, dev, pair, rank, peerRank, tag);

projects/rccl/test/common/MPIEnvironment.hpp:108

• The docstring says callers should “skip” when mpiThreadSupport is below MPI_THREAD_MULTIPLE, but the new multithread helpers deliberately ADD_FAILURE() and return (not skip). Align the documentation with the implemented behavior to avoid confusion for future test authors.

     * MPI_THREAD_MULTIPLE is requested by MPIHelpers::initializeMPI(), but an
     * MPI build may grant less. Test paths that issue MPI calls from worker
     * threads must check this and skip when it is below MPI_THREAD_MULTIPLE,
     * otherwise those calls are undefined behavior (typically a silent hang).

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

projects/rccl/test/transport/NetIbMPI/GeneralTests.cpp:566

• The leading comment block for MultipleSequentialTransfers still describes a "two barriers per iteration" synchronization strategy, but the new threaded path intentionally removes per-iteration MPI barriers. The newly added note should explicitly scope the two-barrier description to the nThreads==1 path, otherwise the test documentation is misleading for the new behavior.

// Parameterized by MPIEnvironment::nThreads. Every worker has an independent
// connection and buffer. N=1 retains the two per-transfer MPI barriers; the
// threaded path leaves rank synchronization to the main-thread harness after
// every worker has completed.

[nikitaxgusev]

LGTM. Test-only change, no public API/ABI impact. The threading model is sound: MPI stays on the main thread, worker bodies use return-code wrappers only (no fatal GTest macros), rank control flow stays symmetric, and the two-gate design makes overlap deterministic. A few non-blocking notes below.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:788

• maxInFlight is incremented before bodyGate and before body() is invoked. Once all workers reach this gate, the recorded maximum is therefore guaranteed to be nThreads, even if the scheduler executes every transport body serially after the gate opens. This makes the new fan-out assertion unable to verify the PR's intended concurrent NET/IB data-path coverage. Move the synchronization/measurement point into the bodies after each worker has posted an outstanding transport request (for example, via a callback/gate passed to the body) so no request can complete before all workers have entered the actual data path.

            const int active = inFlight.fetch_add(1, std::memory_order_relaxed) + 1;
            UpdateMaximum(maxInFlight, active);
            // Do not let an eagerly scheduled worker complete its body before
            // the other workers have actually entered it. This makes the
            // concurrency assertion deterministic rather than scheduler-timing
            // dependent.
            if (!bodyGate.ArriveAndWait()) {

[sdarisala]

Out of curiosity, I see that for both the SimpleSendRecv and MultipleSequentialTransfers tests, we call RunMultiThreadedIndependent() for device 0, even though the nodes have four RDMA devices each. Is this intentional? Is there a plan to also support multiple worker threads spanning multiple devices in the future?

[i-kosarev]

Out of curiosity, I see that for both the SimpleSendRecv and MultipleSequentialTransfers tests, we call RunMultiThreadedIndependent() for device 0, even though the nodes have four RDMA devices each. Is this intentional? Is there a plan to also support multiple worker threads spanning multiple devices in the future?

Yes, that's intentional: it is worth racing here per device, pointing every worker at device 0 is what creates the contention. And also I am introducing workers spreading in follow-up PR #10301 via ThreadDevPolicy::Spread()