Repository navigation
fix(rccl): align one-RPN AllGather scheduling [ROCM-33020] - #13645
Conversation
|
Cursor Agent can help with this pull request. Just |
✅ 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.
🔵 Needs a closer look
The focused regression remains uncompiled, and two-node MI325X/MI355X validation is pending for this host/device scheduling change.
No open issues 🎉
What changed in this PR
Aligns RCCL’s AMD registered one-rank-per-node Ring AllGather host scheduling with the device kernel to address ROCM-33020.
Changes:
- Keeps the ordinary chunk schedule on AMD while preserving non-AMD net-offload behavior.
- Adds an 11 MiB-per-rank regression comparing registered and unregistered scheduling.
| File | Description |
|---|---|
| projects/rccl/test/host/enqueue-test.cc | Adds the focused scheduling regression. |
| projects/rccl/src/enqueue/enqueue.cc | Disables the net-offload scheduling override on AMD. |
Review effort: Balanced
4e77e9f to
36ec27c
Compare
There was a problem hiding this comment.
ARGUS
Findings
1 more finding(s), not posted inline
projects/rccl/CHANGELOG.md:61(Low) No CHANGELOG.md entry under Resolved issues for the registered one-RPN Ring AllGather FIFO-size failure fixprojects/rccl/CHANGELOG.mdrecords user-visible fixes as* Fixed ...bullets under### Resolved issues(line 61, RCCL 2.32.3), and this PR fixes a failure users hit with registered buffers on two-node one-rank-per-node Ring AllGather. Could we add a bullet there?
Bottom line: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
alex-breslow-amd
left a comment
There was a problem hiding this comment.
Looks reasonable
042c6f2 to
6ae01f8
Compare
pr-rocm-systems-13645-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13645-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13645-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
ROCM-33020 exposes a host/device mismatch for registered one-RPN ring AllGather. Keep the AMD proxy on the ordinary chunk schedule because the HIP device kernel does not implement net offload. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
Exercise the 11 MiB-per-rank ring AllGather shape from ROCM-33020 and verify that registered and unregistered AMD paths use the same proxy schedule while retaining distinct registration state. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
Use one compile-time capability for host proxy scheduling, device dispatch, and the device barrier. Keep HIP offload disabled and native CUDA behavior unchanged while tightening the host regression around schedule parity. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
Drop the test that restated the platform preprocessor condition. Keep the focused registered-vs-unregistered schedule parity regression as the functional guard. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
Apply the one-RPN registered AllGather minNetCount cap only when the matching net-offload kernel is supported. Preserve Broadcast and every other registration path, with focused host coverage of the predicate scope. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ilia.kosarev <ilia.kosarev@amd.com>
6ae01f8 to
5669f96
Compare
pr-rocm-systems-13645-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13645-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
…d gate PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED, which is 0 on AMD, but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. coll-reg-test.cc: the loop body becomes a fixture helper taking the func and the channel count it should be left with, so the AllGather and Broadcast cases split into one test each and every expected number is named at its call site. The AllGather case is gated on the macro. enqueue-test.cc: the whole-net-sized-chunks test is gated on the same macro and skips otherwise, pointing at the replacement test PR #13645 added for the unsupported case. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the loop body becomes a fixture helper taking the func and the channel count it should be left with. Broadcast and AllGather split into one test each, named for what they expect, with the numbers at the call site. enqueue-test.cc: CalcCollChunking_NetRegRingOneRpnAllGather_UsesWholeNetSized- Chunks covered a branch RCCL cannot compile and is removed, leaving a comment pointing at the test that covers the schedule RCCL does produce. That test and the channel-cap predicate test both drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the loop body becomes a fixture helper taking the func and the channel count it should be left with. Broadcast and AllGather split into one test each, named for what they expect, with the numbers at the call site. enqueue-test.cc: CalcCollChunking_NetRegRingOneRpnAllGather_UsesWholeNetSized- Chunks covered a branch RCCL cannot compile and is removed. That test's replacement and the channel-cap predicate test both drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the shared one-RPN setup, run and assertion become a fixture helper taking the func, the net count driving the cap and the channel count the task should be left with, so all four one-RPN channel-count tests are a single call naming their own numbers. Broadcast and AllGather are separate tests named for what they expect, rather than one loop over both. enqueue-test.cc: CalcCollChunking_NetRegRingOneRpnAllGather_UsesWholeNetSized- Chunks covered a branch RCCL cannot compile and is removed. That test's replacement and the channel-cap predicate test both drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the shared one-RPN setup, run and assertion become a fixture helper taking the func, the net count driving the cap and the channel count the task should be left with, so all four one-RPN channel-count tests are a single call naming their own numbers. Broadcast and AllGather are separate tests named for what they expect, rather than one loop over both. enqueue-test.cc: the RING arm of the net-registration block is now empty for AllGather, so CalcCollChunking_NetRegRingOneRpnAllGather keeps its inputs -- 8 ranks, 2 channels, 3 GiB -- and asserts the schedule matches an unregistered run instead of the removed NCCL_MAX_NET_SIZE one. That is the only coverage of this path above NCCL_MAX_NET_SIZE or at more than one channel. The predicate test and the companion added by #13645 drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
#13645 compiles out the one-RPN AllGather net-offload schedule on HIP builds: registration no longer caps AllGather channels at minNetCount, and the proxy keeps the ordinary ring schedule instead of NCCL_MAX_NET_SIZE steps. Two tests written against the old schedule failed on develop. Fix: split the channel cap test into Broadcast (still capped) and AllGather (capped only with the net-offload kernel), and assert that registered one-RPN ring AllGather matches the unregistered ring schedule unless the net-offload kernel is available.
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the shared one-RPN setup, run and assertion become a fixture helper taking the func, the net count driving the cap and the channel count the task should be left with, so all four one-RPN channel-count tests are a single call naming their own numbers. Broadcast and AllGather are separate tests named for what they expect, rather than one loop over both. enqueue-test.cc: CalcCollChunking_NetRegRingOneRpnAllGather asserted the NCCL_MAX_NET_SIZE schedule the fix removed. Re-asserting that the schedule now matches an unregistered run would only duplicate the companion #13645 added -- the removed assignment was unconditional, so both tests could only ever fail together. The slot instead covers what the surviving RING arm does and no test checked: RING is accepted rather than reaching the ncclInternalError else, it gets the recv-side wiring every net-reg algorithm gets, and unlike the collnet arms it leaves sendbuff/sendMhandle unset. The predicate test and the companion drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED but left two pre-existing tests asserting the old behaviour. Both fail deterministically on develop, so the host-unit-test gate is red for every open PR. That macro is 0 for every RCCL build: the library is compiled by hipcc and test/host/CMakeLists.txt hardcodes __HIP_PLATFORM_AMD__, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it -- RCCL has exactly one behaviour and the tests now say what it is. coll-reg-test.cc: the shared one-RPN setup, run and assertion become a fixture helper taking the func, the net count driving the cap and the channel count the task should be left with, so all four one-RPN channel-count tests are a single call naming their own numbers. Broadcast and AllGather are separate tests named for what they expect, rather than one loop over both. enqueue-test.cc: CalcCollChunking_NetRegRingOneRpnAllGather asserted the NCCL_MAX_NET_SIZE schedule the fix removed. Re-asserting that the schedule now matches an unregistered run would only duplicate the companion #13645 added -- the removed assignment was unconditional, so both tests could only ever fail together. The slot instead covers what the surviving RING arm does and no test checked: RING is accepted rather than reaching the ncclInternalError else, it gets the recv-side wiring every net-reg algorithm gets, and unlike the collnet arms it leaves sendbuff/sendMhandle unset. The predicate test and the companion drop their #if / GTEST_SKIP and their assertion against the macro, so neither can silently become a no-op. Co-authored-by: Pi Agent
PR #13645 (ROCM-33020) gated the one-RPN AllGather net-offload path behind `NCCL_ALLGATHER_NET_OFFLOAD_SUPPORTED`. It added tests for the new behaviour but left two pre-existing tests asserting the old behaviour, so `RegisterCollRingMicrotest.OneRpnGatherRegistered_CapsChannelsAtNetCount` and `EnqueueMicrotest.CalcCollChunking_NetRegRingOneRpnAllGather_UsesWholeNetSizedChunks` fail deterministically on `develop` and the host-unit-test gate is red for every open PR. That macro is `0` for every RCCL build: the library is compiled by `hipcc`, `test/host/CMakeLists.txt` hardcodes `__HIP_PLATFORM_AMD__` on every target, and RCCL's CMake has no non-AMD platform path at all. It exists only to keep these files diffable against upstream NCCL, so no test needs to branch on it — RCCL has exactly one behaviour and the tests now say what it is. In `coll-reg-test.cc` the shared one-RPN setup, run and assertion become a fixture helper taking the func, the net count driving the cap and the channel count the task should be left with, so all four one-RPN channel-count tests collapse to a single call naming their own numbers. Broadcast and AllGather become separate tests named for what they expect, rather than one loop over both. In `enqueue-test.cc` the stale test asserted the `NCCL_MAX_NET_SIZE` schedule the fix removed. Re-asserting that the schedule now matches an unregistered run would only duplicate the companion #13645 added: the removed assignment was unconditional, so both tests could only ever fail together. The slot instead covers what the surviving RING arm does and no test checked — RING is accepted rather than reaching the `ncclInternalError` else, it gets the recv-side wiring every net-reg algorithm gets, and unlike the COLLNET_DIRECT / NVLS / COLLNET_CHAIN arms it leaves `sendbuff`/`sendMhandle` unset. The predicate test and the companion both drop their `#if` / `GTEST_SKIP` and their assertion against the macro, so neither can silently become a no-op. Testing: `rccl-UnitTestsMicro` (2960 tests) and `rccl-UnitTestsMicroEnqueue` (254 tests) pass under `--gtest_shuffle` across repeated runs, with no skips. Every assertion was mutation-tested against the hipify tree rather than only observed green: reverting the ROCM-33020 fix turns the companion red, and separately breaking RING acceptance, the `sendbuff` asymmetry, the `recvbuff` wiring and the `nbytes` reset each turns the new RING test red. JIRA ID : AICOMRCCL-3479
TheRock Submodule Bump ActivityNewest first
|
…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
ROCM-33020 is a host/device schedule mismatch in registered one-rank-per-node Ring/Simple AllGather. The host proxy expects
NCCL_MAX_NET_SIZEsteps while the HIP kernel uses the ordinary ring schedule, so the proxy FIFO-size check fails.Technical Details
d045d0ca239(legacya6bf9bfc, ROCm/rccl#1645) retained the 1 GiB host schedule but disabled HIP device offload, creating the latent mismatch.Issue Tracking
JIRA ID: ROCM-33020
Testing
The gfx950 matrix used two nodes, one rank/GPU per node, Ring/Simple, 11 MiB per rank, and five fresh launches per arm. These results are from the pre-rebase PR head, not the current SHA or a target MI355X system.
ee0a6d4e614e77e9f3bbdNET send buffer size 11534336 does not match FIFO size 0, then timeout/dev/kfdaccess was denied and no verbs devices were available, so registration/proxy coverage was inconclusiveChannel-cap A/B validation:
07ccc0cd96with the local gated build: +77.4% at 11 MiB, +136.2% at 64 MiB, and +176.8% at 1 GiB; results were correct.Submission Checklist