Repository navigation
fix(rccl/gin): IB-CAST RMA backend and per-context uDMA pipelines for GIN on AINIC - #14982
Open
JinghanHuang wants to merge 3 commits into
Open
JinghanHuang wants to merge 3 commits into
JinghanHuang wants to merge 3 commits into
Conversation
GIN proxy traffic goes through RCCL's internal RMA backend. plugin/net.cc picks the IB-CAST NET plugin for NCCL_NET=IB-CAST, for its alias NCCL_NET=ROCM-IB (rcclCanonicalNetName), and for an unset NCCL_NET on an AINIC host. plugin/rma.cc picked the IB-CAST RMA backend only for a literal NCCL_NET=IB-CAST, so with ROCM-IB (what AMD's AINIC test configs set) or an unset NCCL_NET, GIN on AINIC ran over the generic IB backend, without IB-CAST's AINIC handling such as its uDMA pipeline assignment. rma.cc now makes the NET plugin's choice: rcclCanonicalNetName(NCCL_NET) == IB-CAST, or NCCL_NET unset on an AINIC host. Unlike plugin/net.cc it does not look at NCCL_NET_PLUGIN: when an external NET plugin is requested, RMA still uses the internal backend unless that plugin provides RMA. The include/net.h note on rcclCanonicalNetName now lists rma.cc as a plugin-selection user. An INFO line names the chosen backend. ROCm#9085 made the same choice for GIN in plugin/gin.cc before the GIN/RMA split. Host tests (RmaInternalBackendTest, registered in test_categories_micro.yaml) cover unset, IB-CAST, IB and ROCM-IB with and without AINIC detection.
On AINIC a queue pair's channel id picks its uDMA pipeline (net_ib_cast connect.cc): under the default RCCL_IB_UDMA_POLICY (ChannelAffinity) the first QP of each (NIC, channel id) takes the next pipeline in turn. The IB-CAST RMA backend, which carries GIN proxy traffic, connected every context with no channel context, so all GIN and RMA proxy contexts got channel 0 and shared one pipeline, which caps GIN proxy bandwidth per NIC. Each context now takes its own channel id from a process-wide counter, counting down from MAXCHANNELS - 1 and wrapping at MAXCHANNELS / 2, so under the default policy consecutive contexts land on different pipelines. NET channels use the low ids and reach these only when the channel count is raised past MAXCHANNELS / 2; a NET channel and a context then share a pipeline, which costs bandwidth, not correctness. The id only selects the local pipeline and telemetry bucket and is never sent to the peer, so ranks need not agree on it; connect.cc reads it only in AINIC mode, so other hosts are unaffected. With RCCL_TELEMETRY_ENABLE=1, AINIC devices that carry GIN or RMA proxy contexts now report per-channel ids instead of null, and num_channels (highest id + 1) reads up to MAXCHANNELS. A host test checks the id each connect and accept receives.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
ARGUS
Findings
3 more finding(s), not posted inline
projects/rccl/src/transport/net_ib_cast/gin.cc:464(Low) Top-range chId also selects the telemetry bucket, so num_channels reads 256 on AINIC jobs with a GIN context even when few NET channels run; consider reporting GIN/RMA ids separately.projects/rccl/test/host/transport/net_ib_cast/net-ib-cast-gin-test.cc:671(Low) No test fails if the% (MAXCHANNELS / 2)wrap at gin.cc:464 is dropped; three contexts never reach the wrap, which keeps chId inside nccl_channel_ud_map.projects/rccl/test/host/transport/net_ib_cast/net-ib-cast-gin-test.cc:634(Nit) accept_ hook line 631 is 121 columns, past the 120 ColumnLimit; wrap it like connect_.
Bottom line: Worth settling before approval: IB-CAST becomes the default internal RMA backend on AINIC with no opt-in, and if its init fails GIN falls back to the socket proxy instead of generic IB; the deliberate NCCL_NET_PLUGIN split is also untested.
Comment @argus review to re-run, or @argus re-review after pushing.
…low net.cc's NET plugin rule - When IB-CAST is the internal RMA backend, the generic IB backend now follows it, so if IB-CAST cannot initialize (for example with an older libionic) RMA and GIN fall back to generic IB as before, not to the socket backend. - The choice now applies plugin/net.cc's whole rule: a request for an external NET plugin keeps the generic IB backend, as it keeps the internal NET plugin on generic IB. - Tests pin the internal backend order for each NCCL_NET, NIC type and NCCL_NET_PLUGIN, and the wrap of the RMA contexts' channel ids. Shorter comments; the CHANGELOG entry names NCCL_NET=IB as the opt-out.
Contributor
Author
|
Thanks for the review. On the three findings in the summary:
|
Contributor
Author
|
@argus re-review |
Contributor
|
@JinghanHuang I can't trigger on this PR yet -- your association here reads as |
This branch has not been deployed
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
Two problems with GIN over the proxy on AINIC hosts:
NCCL_NET=ROCM-IB(what AMD's AINIC test configs set) orNCCL_NETunset, the data path uses the IB-CAST NET plugin (plugin/net.cc), but the internal RMA backend that carries GIN proxy traffic is the generic IB one, without IB-CAST's AINIC handling.plugin/rma.ccpicked IB-CAST only for a literalNCCL_NET=IB-CAST.Together: GIN proxy puts on 2 MI355X nodes go from 29.0 to 46.6 GB/s per rank at 64 MiB (results below).
Technical Details
Two commits:
plugin/rma.cc). The backend is IB-CAST forrcclCanonicalNetName(NCCL_NET) == "IB-CAST"(soIB-CASTandROCM-IB), or for an unsetNCCL_NETon an AINIC host, the same values that select the IB-CAST NET plugin. It isplugin/net.cc's whole rule, so a request for an external NET plugin keeps the generic IB backend, as it keeps the internal NET plugin on generic IB. The generic IB backend follows IB-CAST as a fallback, so if IB-CAST cannot initialize (for example with an older libionic), RMA and GIN run on generic IB as before rather than on the socket backend.NCCL_NET=IBopts out. Theinclude/net.hnote onrcclCanonicalNetNamenow listsrma.ccas a plugin-selection user. An INFO line names the chosen backend. ROCM-27070: honor NCCL_GIN_ENABLE=0 and select ib-cast GIN backend on AINIC #9085 made the same choice for GIN inplugin/gin.ccbefore the GIN/RMA split.transport/net_ib_cast/gin.cc). Each IB-CAST RMA context passes its own channel id to connect and accept, from a process-wide counter counting down fromMAXCHANNELS - 1and wrapping atMAXCHANNELS / 2. Under the defaultRCCL_IB_UDMA_POLICY(ChannelAffinity, fix(rccl): fixed udma balancing for p2p comms (develop) #12948) consecutive contexts then land on different uDMA pipelines; with the driver-default policy or QP sharing the id does not pick the pipeline. NET channels use the low ids and reach these only when the channel count is raised pastMAXCHANNELS / 2; a NET channel and a context then share a pipeline (bandwidth, not correctness). The id only selects the local pipeline and telemetry bucket and is never sent to the peer, so ranks need not agree on it.connect.ccreads it only in AINIC mode, so other hosts are unaffected. WithRCCL_TELEMETRY_ENABLE=1, AINIC devices that carry GIN or RMA proxy contexts now report per-channel ids instead of null, andnum_channels(highest id + 1) reads up toMAXCHANNELS.RmaInternalBackendTestsuite is registered intest/test_categories_micro.yaml, so the CTest categories run it); CHANGELOG entries under Resolved issues. A third commit addresses the first review: the IB fallback,plugin/net.cc's external NET plugin rule, the channel id wrap test and shorter comments.Issue Tracking
Related: #9085 (IB-CAST GIN backend on AINIC), #12948 (UDMA balancing).
Test Plan
rccl-UnitTestsMicro(RmaPluginInitTest.*,RmaInternalBackendTest.*) andrccl-UnitTestsMicroTransportNetIbCastGin, with and without the source changes.developand this branch run on the same two hosts. A small GIN proxy benchmark: every rank puts to its rail peer on the other node over 4 GIN contexts, the receiver checks every byte, median of 50 rounds per size.NCCL_GIN_TYPE=2 NCCL_CUMEM_ENABLE=1 NCCL_DMABUF_ENABLE=1 NCCL_IB_HCA=ionic, with the hosts' defaultNCCL_NET(which resolves toROCM-IBthere) and withNCCL_NET=IB-CAST. Warm-up plus 3 repeats, arm order rotated each repeat.Test Result
rccl-UnitTestsMicroTransportNetIbCastGin50/50, RMA plugin tests 14/14. Withdevelop'srma.cc(tests kept), exactly the fiveRmaInternalBackendTestcases that expect IB-CAST with its IB fallback, or the generic IB backend under an external NET plugin, fail; the two plain IB cases pass both ways. Withdevelop'sgin.cc,CreateContext_EachContextPassesItsOwnTopRangeChannelIdfails, and removing the% (MAXCHANNELS / 2)wrap fails exactlyCreateContext_ChannelIdsWrapWithinTheTopHalf.NCCL_NETROCM-IB)ROCM-IB)IB-CASTIB-CASTThe first commit moves the default configuration onto IB-CAST (rows 1 to 2; rows 1 and 3 show that alone is worth about 4% at 64 MiB, 3% at 16 MiB and within noise at 4 MiB), and the second spreads the contexts over the uDMA pipelines (rows 3 to 4, +54% at 64 MiB). The backend is confirmed by the logs:
RMA/Plugin: internal RMA backend IB-CASTandIbCastConnect ... isRMA=1on this branch, genericncclIbConnectfor GIN ondevelopwith the defaultNCCL_NET.The GIN proxy arms ran with
NCCL_DMABUF_ENABLE=1, as the GIN how-to (docs/how-to/device-api-gin.rst) requires.Submission Checklist