Repository navigation
fix(rccl): back host-memory shared P2P pools with huge pages [ROCM-33021] - #13734
Merged
Merged
Conversation
…021] Shared P2P staging pools that live in host memory (P2P connections without GDR, e.g. through a NIC that is not local to the GPU) are registered on every NIC that serves such a connection. hipHostMalloc backs them with 4 KiB pages, so one 256 MiB pool costs 65,536 NIC page-table entries. AINIC firmware caps those per LIF (rdma_npts_per_lif = 1,048,576): with Direct AllGather enabled on AINIC for <= 8 nodes (#11635) one communicator's pools nearly fill a NIC and the next communicator fails in ibv_reg_mr_iova2 with EINVAL. Allocate these pools as a 2 MiB-aligned anonymous mapping with MADV_HUGEPAGE and register it with cudaHostRegister, the same way the legacy SHM path shares host memory with the GPU. The MR can then use 2 MiB pages (128 entries per 256 MiB). The device-side address from cudaHostGetDevicePointer is used for the GPU mapping. Any failure falls back to hipHostMalloc. RCCL_NET_SHARED_HOST_HUGEPAGES=0 restores the previous allocation. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No undocumented RCCL or NCCL environment-variable changes. |
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
documentation
mkuznet1
marked this pull request as ready for review
October 21, 2026 10:38
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/src/transport/net.cc:925(Low) Huge-page fallback at net.cc:925 is logged with INFO; the style guide asks for ATTN() on configuration fallbacksprojects/rccl/src/transport/net.cc:916(Low) CUDACHECK early returns at net.cc:908 and :916 leak the huge-page mapping (still registered after 916)projects/rccl/src/transport/net.cc:911(Low) Capture-mode exchange + cudaHostRegister (uncached split) + cudaHostGetDevicePointer at net.cc:907-916 duplicates os/linux.cc:857-868projects/rccl/src/transport/net.cc:869(Low) RCCL_NET_SHARED_HOST_HUGEPAGES=0 (old hipHostMalloc path) loses its incidental test coverage once the default flipsprojects/rccl/src/include/proxy.h:275(Nit) proxy.h:274-275 aligns trailing comments; the RCCL style guide says adjacent trailing comments are not aligned
Bottom line: Worth settling before approval: sharedHostBuffFree (net.cc:939) calls cudaHostUnregister without the rcclShutdownFlag() guard that ncclCudaHostFree gave this pool during process exit.
Comment @argus review to re-run, or @argus re-review after pushing.
- Skip cudaHostUnregister/munmap when rcclShutdownFlag() is set, as ncclCudaHostFree does, so proxy teardown at exit does not call into a destroyed HIP runtime. - Do not return from the middle of the allocation: a failed capture-mode exchange used to leak the mapping (and, after cudaHostRegister, leave it registered). Failures now unwind and fall back to hipHostMalloc. - Report the fallback with ATTN() and log mmap failures, so a pool that is back on 4 KiB pages is visible without NCCL_DEBUG=INFO. - Compile the Linux-only mmap/madvise/smaps path under NCCL_OS_LINUX and keep hipHostMalloc on other platforms. - proxy.h: do not align adjacent trailing comments.
Contributor
Author
|
Addressed in c965db6: ATTN() on fallback, no mapping leak on early CUDA errors, Linux-only path under NCCL_OS_LINUX, proxy.h comments unaligned. Not changing the register block duplicated from os/linux.cc:857-868: a shared helper would touch the SHM path in a bugfix. Build-checked for gfx950, not re-run on hardware. @argus re-review |
Contributor
Author
|
@alexander-sannikov ARGUS asks about coverage of RCCL_NET_SHARED_HOST_HUGEPAGES=0. It is covered by the hardware runs in the description (=0 reproduces the ibv_reg_mr_iova2 failure, default passes). No host test: net.cc has no micro-test binary, so it needs a new one. Enough, or do you want a test in this PR? |
Contributor
|
@mkuznet1 the current head ( |
alexander-sannikov
approved these changes
Oct 30, 2026
Contributor
pr-rocm-systems-13734-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13734-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13734-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 24 tests skipped. |
…est [ROCM-33021] net.cc now calls hipHostRegister/hipHostUnregister, which the host micro-test binaries (linked without the HIP runtime) did not define, breaking the rccl-UnitTestsMicroNet link. Route hipHostRegister, hipHostUnregister and hipHostGetDevicePointer through hip_fakes seams (defaults keep the old behavior) and add NetSharedHostHugePagesMicrotest for the new paths: registration and device alias, refcounted reuse, free, shutdown skip, RCCL_NET_SHARED_HOST_HUGEPAGES=0, small pools, and both registration failure fallbacks. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
CI fix: net.cc now calls hipHostRegister/hipHostUnregister, which the host micro-test (net-test.cc, linked without the HIP runtime) did not fake, so rccl-UnitTestsMicroNet failed to link. Added the fakes and 8 tests for the huge-page pool (HUGEPAGES=0, register/devptr failure fallback, refcount, free, shutdown). MicroNet passes 307/307 locally. |
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
… [ROCM-33021] Compare the full flag set instead of a Portable|Mapped mask, so dropping hipExtHostRegisterUncached under HIP_HOST_UNCACHED_MEMORY (required on gfx950, see checkHostUncacheMemSetting in init.cc) fails the test. Same pattern as rma-socket-mem-test.cc. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
ddurnov
approved these changes
Nov 6, 2026
Conflict in projects/rccl/CHANGELOG.md: develop #14929 moved entries whose code landed after the release/therock-10.2 pin to the ROCm 10.3.0 (Unreleased) section. This fix is not merged yet, so its Resolved issues entry moves there too.
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
…ixes Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/CHANGELOG.md
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
…ixes Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rccl/CHANGELOG.md
Contributor
pr-rocm-systems-13734-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
Contributor
pr-rocm-systems-13734-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 theGPU) the pool is host memory from
hipHostMalloc, which uses 4 KiB pages. Each pool isregistered 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'spools take about 15/16 of a NIC, and the next communicator that opens P2P connections (the FSDP2
data-parallel group) fails
CREATE_MRwithBAD_ATTR, which verbs reports asEINVAL.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 withcudaHostRegister(Portable | Mapped, plus
hipExtHostRegisterUncachedunderHIP_HOST_UNCACHED_MEMORY), thesame 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.
ncclProxySharedP2pkeepshostBuffDevfromcudaHostGetDevicePointerandsharedNetBuffersInithands it out as the GPU pointer.hipHostMalloc.RCCL_NET_SHARED_HOST_HUGEPAGES=0restores the previous allocation.INFOline 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 at0bf70ef6), 2x/4x MI355X nodes with8x AINIC each, Primus Megatron through madengine, 10 iterations,
NCCL_DEBUG=INFO. The patch wasapplied to
0bf70ef6, built for gfx950 inside the image, and bind-mounted over bothlibrccl.so.1copies in the image. No workaround variables are set unless listed. The branch rebased on
developwas also built for gfx950 in the same image with no new warnings.Test Result
ibv_reg_mr_iova2EINVALRCCL_DIRECT_ALLGATHER_DISABLE=1RCCL_NET_SHARED_HOST_HUGEPAGES=0ibv_reg_mr_iova2EINVAL (9/16 ranks)ibv_reg_mr_iova2EINVALRCCL_DIRECT_ALLGATHER_DISABLE=1ibv_reg_mr_iova2EINVAL on all 4 nodes)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 communicatorsper rank, as in the failing baseline. Performance matches the
RCCL_DIRECT_ALLGATHER_DISABLE=1workaround on this setup.
Environment workarounds measured on the same setup (qwen3_32B, stock RCCL), for reference:
NCCL_P2P_NET_CHUNKSIZE=131072pass 695.2,NCCL_COMM_SPLIT_SHARE_RESOURCES=1pass 702.8,NCCL_PXN_DISABLE=0pass 687.8,NCCL_LOCAL_REGISTER=0FAIL 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 withTHP set to
neverstill get 4 KiB pages and keep the old footprint (theINFOline shows0 kB). Independent of ROCM-33020 / #13645, which is a different AllGather defect in the sameimage.
Submission Checklist