Repository navigation
NetIbMPI tests multithread mode for IB-CAST and NIC fusion - #10540
Conversation
i-kosarev
changed the title
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
Pull request overview
Adds concurrent worker coverage for IB-CAST schedulers and fused-device transport paths.
Changes:
- Adds threaded CAST helpers and tests at multiple worker counts.
- Adds concurrent fused-vNIC registration, transfer, and flush coverage.
- Enables threaded CAST suites across supported platform configurations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
net_ib_transport.json |
Registers multi-count CAST and fused-vNIC suites. |
mi455_ainic_roce.json |
Enables four-worker CAST coverage. |
mi455_ainic_roce_func.json |
Enables functional CAST concurrency coverage. |
mi355x_thor2_roce.json |
Enables four-worker CAST coverage. |
mi355x_thor2_roce_func.json |
Enables functional CAST concurrency coverage. |
mi300x_mellanox_ib.json |
Enables four-worker CAST coverage. |
mi300_mellanox_ib_func.json |
Enables functional CAST concurrency coverage. |
ainic.json |
Adds the A18 CAST multithread suite. |
NicFusionTests.cpp |
Adds threaded fused-vNIC test paths. |
NetIbMPITestBase.hpp |
Adds worker-safe CAST scheduler helpers. |
CastTests.cpp |
Adds concurrent scheduler test branches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2468085 to
778675a
Compare
778675a to
836b856
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
projects/rccl/test/transport/NetIbMPI/CastTests.cpp:1107
- The shared preparation helper writes a 64-byte warm-up, while
bufSizecan be smaller than 64 for a valid lowRCCL_IB_QP_SCHED_SPLIT_DATA_MIN. That overruns both the allocation and MR registration before this sweep begins. Clamp the backing buffer size to 64 bytes while retaining the calculated transfer sizes.
const size_t bufSize = threshold * 2;
836b856 to
21123e6
Compare
21123e6 to
28d9a72
Compare
i-kosarev
changed the title
28d9a72 to
d5b7307
Compare
d5b7307 to
55b8de4
Compare
RCCL Perf-Regression Gate:
|
| 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 |
There was a problem hiding this comment.
🟡 Changes recommended
The repeated threaded flush test can falsely pass using stale GPU data from an earlier iteration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The threaded paths synchronize failures, use live QP counts, preserve resource cleanup, and are consistently enabled by the runner configurations.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
There was a problem hiding this comment.
ARGUS
This PR changes 1,117 lines across 11 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 threaded CastStressMultiRoundTwoConns ledger audit at CastTests.cpp:1623 holds whether or not WRR consumed a token, so its 100-message phase asserts nothing. Everything else is consolidation.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 1,080 lines across 11 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 once the kThreadedMsgs / EqualTokens coupling is made explicit. That and the remaining comment and key-order nits are mechanical, so they do not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 1,083 lines across 11 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 once the phase-1 payload seed at CastTests.cpp:1547 is worker-constant. That and the three comment nits are mechanical, so they do not need another pass from me.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 1,089 lines across 11 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. 3 finding(s) below Medium not posted this round; ask for a full pass if you want them.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently handle live QP counts, synchronized failures, worker isolation, and resource cleanup without identified unresolved defects.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:2181(Critical) RunThreadedSizeSweep still references the localhthis PR renamed, so the header does not compileWorkerRetainHostBuffer(result, h)at NetIbMPITestBase.hpp:2181 still namesh, but this hunk renamed that buffer towholeat 2147 andstepat 2166, so the header no longer compiles and every translation unit including it fails to build withMPI_TESTS_ENABLED. The live object is mode dependent, so it needsperSize ? step : whole: retainingwholein PerSize mode releases an empty guard and letsstepfree the buffer anyway.
This PR changes 1,089 lines across 11 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: NetIbMPITestBase.hpp:2181 still names the h this PR renamed, so the header does not compile with MPI tests enabled.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
🟡 Changes recommended
The size-sweep refactor references an undeclared variable and prevents compilation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
ARGUS
This PR changes 1,096 lines across 11 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. 7 finding(s) below High not posted this round; comment @argus re-review full for a full pass if you want them.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
🟡 Changes recommended
Several timeout paths release registered memory while requests may remain active.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
projects/rccl/test/transport/NetIbMPI/NetIbMPITestBase.hpp:1632
- This transfer wrapper also discards the
outstandingflag. On a wait timeout, each caller returns and destroys its registered host buffer while the request can still reference it; expose that state and retain the caller's buffer before unwinding.
ThreadResult WorkerCastTransferExpectTokens(int rank, ConnectionPair& pair, void* buffer,
size_t size, int tag, void* mhandle, int seed,
int expectedTokenDelta,
int timeoutMs = kLargeTransferTimeoutMs) {
projects/rccl/test/transport/NetIbMPI/NicFusionTests.cpp:1204
- A transfer timeout leaves the request active, but this call does not request the
outstandingresult, so the lambda immediately deregisters and freesgpuBuffer. Preserve both guards on that path, matching the GPU stress branch's timeout handling.
int received = 0;
result = WorkerSendRecvRaw(rank, pair, gpuBuffer, size, 600, mhandle,
kDefaultTimeoutMs, &received);
if (!result.ok) return result;
projects/rccl/test/transport/NetIbMPI/NicFusionTests.cpp:1230
- If the flush wait times out, the flush request may still reference this GPU buffer, yet both guards are destroyed on return. Retain the registration and allocation on this failure path, as the existing threaded GPU flush test does.
if (flushRequest != nullptr) {
result = WorkerWait(flushRequest, nullptr);
if (!result.ok) return result;
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Two failure paths introduce unsafe resource-lifetime behavior, including a data race and possible teardown of in-flight registered memory.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
ARGUS
This PR changes 1,174 lines across 11 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. 3 finding(s) below High not posted this round; comment @argus re-review full for a full pass if you want them.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 1,265 lines across 11 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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Probe allocation can deadlock ranks, and forced token initialization leaves split-path weights unset.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
Token ledgers and the round-robin cursor are per send communicator, so a worker can arm and read its own connection without disturbing its siblings -- which is what lets the CAST scheduler tests run threaded at all. WorkerCastSetTokens/WorkerCastGetSchedState/WorkerCastLiveNqps wrap the inspection API in the ThreadResult convention the rest of the threaded harness uses, and ThreadedCastAgreedNqps establishes the one fact the whole family needs before it can size anything: the connection's live queue-pair count, agreed across ranks. That count cannot be taken from NCCL_IB_QPS_PER_CONNECTION. The variable states a request; on a merged device the plugin creates that many per member, so a split-data threshold derived from the environment sits on the wrong side of the boundary and the token-delta assertions test nothing. A worker cannot broadcast the real count either, so the probe brings up its own throwaway connection on the main thread, reads the count from the scheduler once it is warm, and agrees it before any worker starts.
RunThreadedSizeSweep registered one buffer covering its largest step and reused that handle for every step. That is the right shape for some tests and the wrong one for the fused-device sweeps this branch adds: on a merged device every registration fans out across both members' protection domains and MR caches, which is the thing under test, and registering sub-ranges of one block keeps handing back the same cache entry once a covering registration is live -- so the churn never happens. SweepRegistration::PerSize allocates and registers each step fresh and releases it at the end of that step; Once keeps the previous behaviour and stays the default. A threaded branch has to match whichever shape its serial body uses or it quietly covers less, which is why this is a parameter rather than a change of default. The timeout cleanup picks up whichever holder owns the step -- the fresh one under PerSize, the sweep-wide one under Once -- so a request still in flight cannot have its memory released underneath it either way.
…orker counts Threaded bodies for the CAST scheduler tests (token distribution, the WRR/split boundary, the size ladder, zero-size, the large transfer, the multi-round stress) and for the fused-device tests (registration cycling, repeated flush, sequential transfers, the vNIC size sweep). Each worker drives its own connection with a per-worker payload pattern, so a transfer delivered on the wrong connection fails verification rather than passing quietly. Two things the bodies have to get right that the serial versions do not: The payload pattern is per worker and constant across the phase, not folded from the size or the message index. makeBytePattern keeps only the seed modulo 256 and the worker stride is small, so a per-message seed collides across workers -- one worker's message 0 and another's message 95 produce the same byte -- and with every worker sending the same size the size check cannot separate them either. Freshness per message still holds because the receiver clears its buffer first. Token-delta assertions need the RTT-driven ledger update suspended: with several workers the wall-clock gap between the before and after snapshots is wide enough for the timer to rewrite the ledger underneath them, so those bodies require a long update interval and skip otherwise. Every transfer captures the outstanding signal and retains its buffer and registration on a timeout, since a timed-out request is still live and the guards would otherwise free memory the NIC can write into. NicFusionTests needs three shapes of that: the shared cross-worker allocation in RegDeregCycling_VNic is dismissed from inside the worker, FlushRepeated_VNic retains its per-worker GPU buffer and registration directly, and SequentialTransfers_VNic uses the plain WorkerHostBuffer path.
Adds the new threaded suites to every config that carries their serial siblings, and marks a_cast_multithread (A18) as smoke on the AINIC config -- it is a Component suite, so the name-glob the gate selects on already covers it and the flag makes that explicit.
There was a problem hiding this comment.
ARGUS
This PR changes 1,342 lines across 11 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.
TheRock Submodule Bump ActivityNewest first
|
Motivation
Third of four stacked PRs. IB-CAST keeps its token ledger per communicator, so the serial bodies already build several connections but drive them one after another; here the schedulers advance at the same time, each auditing its own ledger. Fused devices come with it because a vNIC is still the plain send/recv path — the difference is that every registration fans out across both members' protection domains and caches, doubling the contended surface for the same test body.
Base:
users/ilkosare/netib-threaded-independent.Technical Details
The threaded CAST branches query the live QP count from the scheduler instead of
trusting
NCCL_IB_QPS_PER_CONNECTION: that variable states a request, and on amerged device the plugin creates that many per member, so an expectation built from
the environment would be wrong on exactly the configurations these tests care
about. The suites hold
RCCL_IB_QP_SCHED_UPDATE_INTERVALat 10 s so the RTT timercannot rewrite tokens mid-assertion.
The vNIC is created once on the main thread and its index handed to the workers,
since the device table is process-global.
Issue Tracking
JIRA ID: AICOMNET-323
Test Plan
Enabled tests
Worker counts are 2, 4 and 16.
IB-CAST scheduler, with the RTT update suspended so token deltas are meaningful:
CastStressMultiRoundTwoConns— the serial body builds many connections butdrives them one at a time; here the schedulers advance simultaneously, each
auditing its own ledger. Deliberately weaker in one respect: the serial body
asserts that the RTT timer fired, which cannot hold at the 10 s interval the
threaded branches need, so the threaded variant drops that assertion and keeps
the ledger audit.
CastSendRecvMultipleSizes— both sides of the WRR/split boundary.CastLargeTransfer— the 16 MB split path, no tokens consumed.CastSendRecvZeroSize— a zero-byte send still takes the WRR path.CastSplitDataThresholdBoundary— exactly at the threshold and one byte below.Fused devices (vNIC created once on the main thread, its index handed to workers):
RegDeregCycling_VNic— every registration fans out across both members'protection domains and caches, doubling the contended surface.
SequentialTransfers_VNic— long-lived registrations, one per worker.MixedSizes_VNic— uneven QP splits across the fused device.LargeTransfer_VNic— striped transfers; the per-worker size scales down withthe worker count, since each registration is pinned once per member.
UnalignedSizeTransfer_VNic— the 128-byte striping boundary.FlushRepeated_VNic— concurrent GPU receives plusiflush.Procedure
Hardware: mia1, AINIC/Pensando with RoCE on
rdma0..rdma3, gfx950, host build,two ranks.
rccl-UnitTestsMPIin a debug configuration with MPI tests enabled.with everything the two levels below ship.
share with the threaded CAST branches.
sources with fault injection off and with MPI tests off.
Test Result
mia1, gfx950, two ranks, on
developat60030ad2f2:0 FAIL / 0 HANG / 1 SKIP. The skip is the serialization gate declining two
workers. The count is higher than it was when this PR was opened because the
suites grew while the stack was under review, not because tests were re-counted.
TestConfigProcessor; the translation units compile withfault injection off and with MPI tests off.
Submission Checklist
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.