Repository navigation
fix(rccl): fixed default behavior on AINIC - #11635
Conversation
✅ 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.
ARGUS
Findings
4 finding(s) that could not be posted as inline comments
projects/rccl/CHANGELOG.md:17(Medium) User-visible default change lands with no CHANGELOG entry, though the project records exactly this class of change This changes a default that library users see, with no entry under### Changedin the open 2.30.7 (Unreleased) section at projects/rccl/CHANGELOG.md:17. The project records exactly this: CHANGELOG.md:129 gates multi-node Direct AllGather on PXN, CHANGELOG.md:120 documents theRCCL_CTS_OFFLOAD_ENABLEDtri-state, and projects/rccl/.github/PULL_REQUEST_TEMPLATE.md:20 makes the entry an approval checklist item.projects/rccl/test/RcclWrapTests.cpp:1297(Medium) Both existing node-count cases return false under the old and the new code, so nothing asserts the side this PR flips Neither case inRcclUseAllGatherDirectNodeCountTests(projects/rccl/test/RcclWrapTests.cpp:1297) changes verdict across this PR: both returned false under the old unconditional AINIC disable and still do under the newnNodes > 8gate. The side this PR flips, AINIC at 8 nodes or fewer going false to true, has no assertion anywhere. A gfx950 row withnNodes=8andrequiresAinicwould pin it, thoughAGDirectNodeCountTestCaseneeds anexpectedfield first since RcclWrapTests.cpp:1353 hardcodesEXPECT_FALSE.projects/rccl/test/RcclWrapTests.cpp:1657(Low) This PR makes an existing GTEST_SKIP reason stale, so the test keeps skipping on the hardware the change targets The skip at projects/rccl/test/RcclWrapTests.cpp:1657, "Direct AllGather is disabled on AINIC", quotes the unconditional AINIC disable this PR removes. That test pinsRCCL_DIRECT_ALLGATHER_DISABLE=0at RcclWrapTests.cpp:1714, sorcclUseAinic()is no longer consulted at all, and its comms arenNodes=1(test/common/MockComm.hpp:39). It now suppresses a test that would run and pass on AINIC. The precondition skip at RcclWrapTests.cpp:1685 already covers the case.projects/rccl/src/rccl_wrap.cc:796(Low) The PXN gate on Direct AllGather never reaches the decision, and enabling the AINIC default at 8 nodes or fewer exposes that path there rccl_wrap.cc:796 overwritescomm->enableCustCollwith a pure arch test just before line 802 reads it, so the PXN gate recorded at CHANGELOG.md:129 and computed byrcclSetPxn(rccl_wrap.cc:1439) never reaches this decision. It predates the PR and affects non-AINIC callers too, so it wants a separate change; noting it once because turning the AINIC default on at 8 nodes or fewer is what exposes it there.
Bottom line: Worth settling before approval: the store at rccl_wrap.cc:759 writes a per-comm decision into a process-wide static, so the new AINIC gate stops applying after the first small comm.
Comment @argus review to re-run, or @argus re-review after pushing.
|
can you add documentation that explains how to enable direct AG on AINIC? Also some estimations on additional QPs required related to total number of GPUs when direct AG is enabled. |
2cd1e69 to
f378cc1
Compare
|
@argus re-review |
|
@argus review |
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/test/RcclWrapTests.cpp:1309(Low) The only AINIC-gated case in RcclWrapTests is permanently skipped in CI, on a premise that is falseNCCL_NET=ROCM-IBforces AINIC mode at src/transport/net.cc:257 regardless of hardware, so the comment at projects/rccl/test/RcclWrapTests.cpp:1331 is wrong andAGDirect_Disabled_AINIC_nNodes17at RcclWrapTests.cpp:1309 need not skip. ItsextraEnvat RcclWrapTests.cpp:1314 is empty and RcclWrapTests.cpp:1360 merges that map into the child environment, so adding that one variable makes the only AINIC-gated case in this suite actually run.
Bottom line: Worth settling before approval: the new nNodes > 8 gate makes the CI microtest UseAllGatherDirect_AinicDisablesDirect (wrap-test.cc:2027) assert the opposite of the new behaviour, so it fails as written.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: RCCL_DIRECT_ALLGATHER_DISABLE=0 was the parameter's own old default and now skips the AINIC guard instead of meaning no override.
Comment @argus review to re-run, or @argus re-review after pushing.
4c7eb1e to
a0237fc
Compare
a0237fc to
4f69bb6
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 |
TheRock Submodule Bump ActivityNewest first
|
Fixed default/out-of-box behavior on AINIC. - DIRECT ALLGATHER on AINIC : - disabled by default for > 8 nodes. - enabled for <=8 nodes - Number of network channels per peer: restored pre-2.29 logic for AINIC. JIRA ID: AICOMRCCL-2215 <!-- Explain any relevant testing done to verify this PR. --> <!-- Briefly summarize test outcomes. --> - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
…er peer (#13468) ## Motivation Commit 242e0a9 (#11635) raised `p2pnChannelsPerPeer` from 1 to 2 on AINIC for jobs of up to 8 nodes, so that a single peer can saturate a NIC. That helps point-to-point traffic, but `ncclScatter` regressed by 15 to 21% from 32 MB upward on multi-node AINIC jobs compared with 7.14. Scatter already drives all `nRanks - 1` peers from one root, so splitting each peer into two parts adds no bandwidth and costs some. This PR restores scatter performance without changing the channel count for the other collectives. ## Technical Details - New helper `rcclP2pTaskParts(comm, collAPI)` returns how many of the `p2pnChannelsPerPeer` parts a P2P task uses and connects. For `ncclFuncScatter` on multi-node AINIC it returns the largest power of two not above `p2pnChannels / (nRanks - 1)`, capped at `p2pnChannelsPerPeer` and at least 1, so the root's flows still fit the P2P channel pool. A user-set `NCCL_NCHANNELS_PER_NET_PEER` is kept, as in `ncclTopoComputeP2pChannels`. Otherwise it returns `p2pnChannelsPerPeer`. - The decision depends only on `collAPI` and comm-wide sizes, so sender and receiver always agree on the number of parts. - Limited to AINIC: it is the only NIC path that raises `p2pnChannelsPerPeer` back to 2 on multi-node (#11635), and the only one measured. On other NICs the halving loop in `ncclTopoComputeP2pChannels` already reduces the parts per peer when the peers do not fit the pool. - Used in three places: - channel count selection (`rcclP2pPolicyChannels`), also with P2P disabled, since the limit is about network flows; - the connector checks and buffer registration in `addP2pToPlan`; - preconnect (`p2pTaskAppend`, `postTuneP2pRecordPreconnect`). - NIC buffer registration (`postTuneP2pRegisterBuffer`) keeps `p2pnChannelsPerPeer`: Scatter tasks have `allowUB` off and never reach it. - When only some parts are connected, `sendSeen` / `recvSeen` stay clear, so a later task to the same peer that uses all parts still connects the rest. - For every other collective and for send/recv the helper returns `p2pnChannelsPerPeer`, which equals `nChannelsMax` in `addP2pToPlan`, so their code path takes the same decisions as before. - Host microtests: - `EnqueueMicrotest.P2pTaskParts_*`: the part rule (pools of 2, 3, 4, 8 and 16 nodes, the pool boundary, power-of-two rounding, the one-part floor, non-AINIC, single node, user-set count); - `EnqueueMicrotest.P2pPolicyChannels_ScatterBelowPerPeerReturnsItsParts`: the channel count; - `TaskPostTuningMicrotest.P2pRecordPreconnect_ScatterWithFewerParts_ClaimsItsPartsAndLeavesTheRestForLater`: the partial-to-full preconnect on both sides. Limiting only the channel count is not enough. When scatter sends on part 0 but connects both parts itself, it stays at or below the two-channel speed in most runs (4 of 5 runs at about 110 GB/s instead of 137 GB/s). Not connecting part 1 makes the result stable. Part 1 connected later by another collective on the same communicator (alltoall) does not slow scatter down. The exact runtime cause of the slowdown is not identified yet: the connection setup logs of fast and slow runs are identical. With `NCCL_PXN_DISABLE=1` there is no regression, because the root is limited by its own NIC. ## Issue Tracking JIRA ID : ROCM-31938 ## Test Plan - 4 nodes x 8 MI355X (32 ranks), AINIC (`NCCL_NET=ROCM-IB`, `RCCL_AINIC_ROCE=1`), PXN on. - rccl-tests with data check on (`-c 1`), `-n 20 -w 5`, 5 cycles per run. - Three builds from the same base, run back to back in every repeat: - revert: develop without the AINIC `p2pnChannelsPerPeer` raise from 242e0a9 (matches 7.14); - develop; - this PR. - All P2P collectives (scatter, gather, alltoall, alltoallv, all_gather, reduce_scatter) up to 16 GB, 2 repeats. - All collectives (also sendrecv, all_reduce, broadcast, reduce) up to 2 GB, 3 repeats. - Scatter on one communicator before and after alltoall and after ring sendrecv, with every received byte checked. - Scatter with `NCCL_PXN_DISABLE=1` and on 2 nodes. - Scatter 16M–1G, root 0, on 2, 3, 4, 5 and 8 nodes, interleaved with develop and the first version of this PR (one part per peer whenever the parts did not fit); all roots on 3 and 4 nodes (from #13528). - Host microtests `EnqueueMicrotest.*` and `TaskPostTuningMicrotest.*`. ## Test Result Scatter, 4 nodes, 32 ranks, median time over 10 samples. Measured with the first version of this PR; on 4 nodes the current rule picks the same single part. | Size | revert (us) | develop (us) | this PR (us) | revert busbw (GB/s) | develop busbw (GB/s) | this PR busbw (GB/s) | develop / revert | this PR / revert | |---:|---:|---:|---:|---:|---:|---:|---:|---:| | 8M | 48.9 | 49.9 | 48.6 | 166.2 | 162.9 | 167.2 | 1.02 | 0.99 | | 16M | 69.9 | 73.3 | 70.1 | 232.5 | 221.7 | 231.9 | 1.05 | 1.00 | | 32M | 109.2 | 132.2 | 111.0 | 297.7 | 245.9 | 292.8 | 1.21 | 1.02 | | 64M | 190.4 | 228.6 | 190.7 | 341.4 | 284.4 | 340.9 | 1.20 | 1.00 | | 128M | 346.9 | 413.8 | 347.5 | 374.8 | 314.2 | 374.2 | 1.19 | 1.00 | | 256M | 655.5 | 768.9 | 655.3 | 396.7 | 338.2 | 396.8 | 1.17 | 1.00 | | 512M | 1281.6 | 1490.8 | 1278.1 | 405.8 | 348.9 | 406.9 | 1.16 | 1.00 | | 1G | 2537.6 | 2951.9 | 2534.4 | 409.9 | 352.4 | 410.4 | 1.16 | 1.00 | | 2G | 5048.6 | 5865.6 | 5050.6 | 412.1 | 354.7 | 411.9 | 1.16 | 1.00 | | 4G | 10082.9 | 11698.2 | 10084.8 | 412.7 | 355.7 | 412.6 | 1.16 | 1.00 | | 8G | 20167.1 | 23342.6 | 20175.5 | 412.6 | 356.5 | 412.5 | 1.16 | 1.00 | | 16G | 40433.2 | 46671.4 | 40393.4 | 411.6 | 356.6 | 412.0 | 1.15 | 1.00 | Scatter by node count, root 0, time ratio vs develop over 16M–1G (from #13528, merged here): | Nodes | Parts per peer (develop / first version / this PR) | first version | this PR | | --- | --- | --- | --- | | 2 | 2 / 2 / 2 | 0.99–1.01 | 0.96–1.00 | | 3 | 4 / 1 / 2 | 1.04–1.13 | 0.85–0.87 | | 4 | 2 / 1 / 1 | 0.80–0.86 | 0.79–0.86 | | 5 | 2 / 1 / 1 | 0.92–1.11 | 0.91–1.08 | | 8 | 2 / 1 / 1 | 0.97–1.02 | 0.94–1.00 | - Scatter matches revert from 8 MB to 16 GB. Develop is 15 to 21% slower from 32 MB up. - 3 nodes: the first version dropped 4 parts to 1 and left two-thirds of the 64-channel pool idle; this PR keeps the 2 parts that fit. All roots on 3 nodes: median 0.82–0.90 vs develop (first version: 0.99–1.10). All roots on 4 nodes: both at 0.84–0.90. - Scatter after alltoall on the same communicator stays at revert level (1 GB: about 2530 us, develop about 2980 us). - With root != 0, scatter shows run-to-run bimodality (about 2.5 vs about 3.5 ms at 1 GB), as in the revert build (pre-242e0a93afa, one part per peer); develop, with two parts per peer, does not show it. The median over all roots is 0.82–0.89 of develop, and root 0 is stable at revert level in every run. - gather, all_gather, reduce_scatter, all_reduce, broadcast, reduce, sendrecv: no difference from develop beyond run-to-run noise. Outliers above 5% only at latency-bound sizes from 1 KB to 1 MB (a few us at a 20 to 30 us spread), also in collectives that do not go through the changed code. - alltoall and alltoallv show the same difference against revert in develop and in this PR (alltoall: develop 1.05, this PR 1.02 to 1.045 at 256 MB to 16 GB). This PR does not add to it. - 2 nodes: no regression in develop, this PR is unchanged. - `NCCL_PXN_DISABLE=1`: all builds at about 64 GB/s at 256 MB, no regression. - Joint validation on ROCm 10.2 (branch ROCM-31938-validation = develop + #13451 + this PR + #13536, 4 nodes, median of 10 from two launches in opposite build order): scatter from 16 MB 0.78–0.86 of develop; against 7.2.3 16 MB 1.13, 32 MB 1.07, from 128 MB 0.95–0.98. The other collectives match develop within noise; on alltoallv two copies of the same develop build differ by up to 6% (A/A run), and validation is 0.98–1.04 of develop over 20 measurements. - Correctness: `#wrong = 0` and no out-of-bounds values in every run. - Host microtests `EnqueueMicrotest.*` and `TaskPostTuningMicrotest.*` pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Ilia Kosarev <Ilia.Kosarev@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…021] (#13734) ## Motivation JIRA: ROCM-33021 ROCM-33021: Megatron FSDP2 training on MI355X with AMD AINIC (2 and 4 nodes) fails at the first FSDP2 all-gather with `Call to ibv_reg_mr_iova2 failed with error Invalid argument`. #11635 (AICOMRCCL-2215) enabled Direct AllGather on AINIC for <= 8 nodes. Direct AllGather sends to every peer over P2P net connections, which use the shared P2P staging pools (`p2pnChannels * NCCL_SHARED_STEPS * p2pChunkSize`, 256 MiB with Primus' `NCCL_P2P_NET_CHUNKSIZE=524288`). For connections without GDR (any NIC that is not local to the GPU) the pool is host memory from `hipHostMalloc`, which uses 4 KiB pages. Each pool is registered on every NIC that serves such a connection, so one pool costs 65,536 NIC page-table entries. AINIC firmware caps them per LIF (`rdma_npts_per_lif` = 1,048,576). One communicator's pools take about 15/16 of a NIC, and the next communicator that opens P2P connections (the FSDP2 data-parallel group) fails `CREATE_MR` with `BAD_ATTR`, which verbs reports as `EINVAL`. Pools in GPU memory are not affected: they are registered with 2 MiB pages (128 entries). ## Technical Details Host-memory shared P2P pools of at least 2 MiB are now allocated as a 2 MiB-aligned anonymous mapping with `madvise(MADV_HUGEPAGE)`, zero-filled, and registered with `cudaHostRegister` (Portable | Mapped, plus `hipExtHostRegisterUncached` under `HIP_HOST_UNCACHED_MEMORY`), the same way the legacy SHM path shares host memory with the GPU. The NIC MR then uses 2 MiB pages, so a 256 MiB pool costs 128 entries instead of 65,536. - The registered range has a device address different from the host address on ROCm, so `ncclProxySharedP2p` keeps `hostBuffDev` from `cudaHostGetDevicePointer` and `sharedNetBuffersInit` hands it out as the GPU pointer. - Any failure (mmap, register, device pointer) falls back to `hipHostMalloc`. - `RCCL_NET_SHARED_HOST_HUGEPAGES=0` restores the previous allocation. - An `INFO` line reports how much of each pool is backed by huge pages (from `/proc/self/smaps`). Direct AllGather defaults are unchanged. ## Test Plan Nightly image `primus_rocm10.2_20261003` (RCCL 2.31.2 at `0bf70ef6`), 2x/4x MI355X nodes with 8x AINIC each, Primus Megatron through madengine, 10 iterations, `NCCL_DEBUG=INFO`. The patch was applied to `0bf70ef6`, built for gfx950 inside the image, and bind-mounted over both `librccl.so.1` copies in the image. No workaround variables are set unless listed. The branch rebased on `develop` was also built for gfx950 in the same image with no new warnings. ## Test Result | Model, nodes | RCCL | Env | Result | ms/iter | TFLOP/s/GPU | |---|---|---|---|---|---| | qwen3_32B BF16, 2 | stock | none | FAIL `ibv_reg_mr_iova2` EINVAL | | | | qwen3_32B BF16, 2 | stock | `RCCL_DIRECT_ALLGATHER_DISABLE=1` | pass | 8581 | 693.9 | | qwen3_32B BF16, 2 | this PR | none | pass | 8589 | 693.2 | | qwen3_32B BF16, 2 | this PR | `RCCL_NET_SHARED_HOST_HUGEPAGES=0` | FAIL `ibv_reg_mr_iova2` EINVAL (9/16 ranks) | | | | llama3.3_70B BF16, 2 | stock | none | FAIL `ibv_reg_mr_iova2` EINVAL | | | | llama3.3_70B BF16, 2 | stock | `RCCL_DIRECT_ALLGATHER_DISABLE=1` | pass | 27308 | 808.8 | | llama3.3_70B BF16, 2 | this PR | none | pass | 27279 | 809.7 | | llama3.3_70B BF16, 4 | stock | none | FAIL (`ibv_reg_mr_iova2` EINVAL on all 4 nodes) | - | - | | llama3.3_70B BF16, 4 | this PR | none | pass | 27336 | 808.0 | With the patch every observed host pool (8 per 2-node run, 64 in the 4-node run) reports `268435456: 262144 kB on huge pages`, and the P2P connections still open on two communicators per rank, as in the failing baseline. Performance matches the `RCCL_DIRECT_ALLGATHER_DISABLE=1` workaround on this setup. Environment workarounds measured on the same setup (qwen3_32B, stock RCCL), for reference: `NCCL_P2P_NET_CHUNKSIZE=131072` pass 695.2, `NCCL_COMM_SPLIT_SHARE_RESOURCES=1` pass 702.8, `NCCL_PXN_DISABLE=0` pass 687.8, `NCCL_LOCAL_REGISTER=0` FAIL with the same error (user-buffer registration is not the consumer). Not covered: no host unit test (the allocation needs a GPU and `cudaHostRegister`); hosts with THP set to `never` still get 4 KiB pages and keep the old footprint (the `INFO` line shows `0 kB`). Independent of ROCM-33020 / #13645, which is a different AllGather defect in the same image. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Alexander Sannikov <Aleksandr.Sannikov@amd.com>
Motivation
Fixed default/out-of-box behavior on AINIC.
Technical Details
Issue Tracking
JIRA ID: AICOMRCCL-2215
Test Plan
Test Result
Submission Checklist