[RCCL] fix: AINIC CTS inline data path correctness and memory optimization - #9887
Open
karthikarum wants to merge 1 commit into
Open
[RCCL] fix: AINIC CTS inline data path correctness and memory optimization#9887karthikarum wants to merge 1 commit into
karthikarum wants to merge 1 commit into
Conversation
karthikarum
requested review from
VadimKutovoi and
alexander-sannikov
and
a lite review from Copilot
August 7, 2026 23:30
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness and reduces memory usage in the AINIC CTS inline-data FIFO path for RCCL’s ROCm IB transport by properly supporting the 32-byte FIFO layout (vs the standard 64-byte layout) and consolidating overlapping configuration behavior.
Changes:
- Overlay standard/inline FIFO storage using unions to avoid double allocation and simplify MR registration.
- Correct CTS inline FIFO send/recv dispatch (struct selection, remote address stride, rkey handling) and constrain
maxRecvsfor the inline mode. - Consolidate CTS-inline enablement onto
IB_USE_INLINE, adjust GDR flush defaulting behavior, and update related assertions/initialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/rccl/src/transport/net_ib_rocm.cc | Fixes CTS inline FIFO correctness (stride/struct reads/clears), reduces FIFO memory via unions, and updates AINIC init/config behavior. |
| projects/rccl/ext-src/rocm_netib.patch | Mirrors the CTS inline FIFO fixes into the external patch payload for upstream net_ib.cc. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2586
to
2600
| uint64_t slot_addr = (rcclCtsInlineData) ? slots_inline[r].addr : slots[r].addr; | ||
| uint64_t slot_size = (rcclCtsInlineData) ? slots_inline[r].size : slots[r].size; | ||
| uint32_t slot_rkey = (rcclCtsInlineData) ? slots_inline[r].rkeys[0] : slots[r].rkeys[0]; | ||
| uint32_t slot_tag = (rcclCtsInlineData) ? slots_inline[r].tag : slots[r].tag; | ||
| if (reqs[r] != NULL || slot_tag != tag) continue; | ||
|
|
||
| if (size > slots[r].size) size = slots[r].size; | ||
| if (size > slot_size) size = slot_size; | ||
| // Sanity checks | ||
| if (slots[r].size < 0 || slots[r].addr == 0 || slots[r].rkeys[0] == 0) { | ||
| if (slot_size < 0 || slot_addr == 0 || slot_rkey == 0) { | ||
| char line[SOCKET_NAME_MAXLEN + 1]; | ||
| union ncclSocketAddress addr; | ||
| ncclSocketGetAddr(&comm->base.sock, &addr); | ||
| WARN("NET/IB : req %d/%d tag %x peer %s posted incorrect receive info: size %ld addr %lx rkeys[0]=%x", | ||
| r, nreqs, tag, ncclSocketToString(&addr, line), slots[r].size, slots[r].addr, slots[r].rkeys[0]); | ||
| r, nreqs, tag, ncclSocketToString(&addr, line), slot_size, slot_addr, slot_rkey); | ||
| return ncclInternalError; |
Comment on lines
993
to
995
| INFO(NCCL_INIT|NCCL_NET, "NET/IB : AINIC RoCEv2 optimizations enabled: CTS Inline Data: %s; CTS Offload: %s; " | ||
| "IB Use Inline: enabled; GDR Flush: disabled", rcclCtsInlineData ? "Enabled": "Disabled", | ||
| "GDR Flush: disabled", rcclCtsInlineData ? "Enabled": "Disabled", | ||
| rcclCtsOffloadEnabled ? "Enabled": "Disabled"); |
Comment on lines
+523
to
537
| + uint64_t slot_addr = (rcclCtsInlineData) ? slots_inline[r].addr : slots[r].addr; | ||
| + uint64_t slot_size = (rcclCtsInlineData) ? slots_inline[r].size : slots[r].size; | ||
| + uint32_t slot_rkey = (rcclCtsInlineData) ? slots_inline[r].rkeys[0] : slots[r].rkeys[0]; | ||
| + uint32_t slot_tag = (rcclCtsInlineData) ? slots_inline[r].tag : slots[r].tag; | ||
| + if (reqs[r] != NULL || slot_tag != tag) continue; | ||
| + | ||
| + if (size > slots[r].size) size = slots[r].size; | ||
| + if (size > slot_size) size = slot_size; | ||
| + // Sanity checks | ||
| + if (slots[r].size < 0 || slots[r].addr == 0 || slots[r].rkeys[0] == 0) { | ||
| + if (slot_size < 0 || slot_addr == 0 || slot_rkey == 0) { | ||
| + char line[SOCKET_NAME_MAXLEN + 1]; | ||
| + union ncclSocketAddress addr; | ||
| + ncclSocketGetAddr(&comm->base.sock, &addr); | ||
| + WARN("NET/IB : req %d/%d tag %x peer %s posted incorrect receive info: size %ld addr %lx rkeys[0]=%x", | ||
| + r, nreqs, tag, ncclSocketToString(&addr, line), slots[r].size, slots[r].addr, slots[r].rkeys[0]); | ||
| + r, nreqs, tag, ncclSocketToString(&addr, line), slot_size, slot_addr, slot_rkey); | ||
| + return ncclInternalError; |
Comment on lines
88
to
90
| + INFO(NCCL_INIT|NCCL_NET, "NET/IB : AINIC RoCEv2 optimizations enabled: CTS Inline Data: %s; CTS Offload: %s; " | ||
| + "IB Use Inline: enabled; GDR Flush: disabled", rcclCtsInlineData ? "Enabled": "Disabled", | ||
| + "GDR Flush: disabled", rcclCtsInlineData ? "Enabled": "Disabled", | ||
| + rcclCtsOffloadEnabled ? "Enabled": "Disabled"); |
wenkaidu
approved these changes
Aug 7, 2026
…ation The AINIC CTS (Clear-to-Send) inline data path in RCCL's IB transport had multiple correctness and memory efficiency issues: 1. Memory bloat: ncclIbSendComm and ncclIbRemFifo allocated both fifo (64B entries) and fifo_inline (32B entries) arrays as separate members, doubling FIFO memory even though only one format is ever active at runtime. 2. FIFO remote address miscalculation: rocmIbPostFifo() always computed the RDMA remote target offset using sizeof(ncclIbSendFifo) (64B stride), even when CTS inline mode was active and entries are 32B. This caused FIFO writes to land at wrong offsets, leading to data corruption or silent mismatches on the sender side. 3. Wrong FIFO struct accessed on send path: ncclIbMultiSend() and rocmIbIsend() unconditionally read metadata (nreqs, addr, rkeys, tag, size, idx) from the 64B ncclIbSendFifo layout, even when the receiver had written 32B ncclIbSendFifoCtsInline entries - reading garbage from the wrong memory layout. 4. Redundant rkey population: rocmIbPostFifo() looped over all devices (ndevs) to fill rkeys in the CTS inline struct, but the inline path only uses a single rkey (rkeys[0]). 5. Confusing / conflicting env knobs: rcclCtsInlineData had its own independent RCCL_CTS_INLINE_DATA env param, while ncclIbUseInline was also unconditionally forced to true for all AINIC - two knobs controlling overlapping behavior with no clear precedence. 6. GDR flush default mismatch: GDR_FLUSH_DISABLE defaulted to 0 (flush enabled), but AINIC always overrode it to 1 at runtime - the default didn't match the intended AINIC behavior. - Memory layout (structs): fifo/fifo_inline in ncclIbSendComm and elems/elems_cts_inline in ncclIbRemFifo are now wrapped in a named union u, so they overlay the same memory - halving the FIFO footprint. - FIFO remote address math: rocmIbPostFifo() now branches on rcclCtsInlineData to compute remote_addr using sizeof(ncclIbSendFifoCtsInline) (32B) vs sizeof(ncclIbSendFifo) (64B), so the RDMA write targets the correct offset. - Send-path struct dispatch: ncclIbMultiSend() and rocmIbIsend() now maintain a separate slots_inline pointer alongside slots, and branch on rcclCtsInlineData to read nreqs, addr, rkeys, tag, size, idx from the correct struct type. - maxRecvs clamped: rocmIbGetPhysProperties() now returns maxRecvs=1 when CTS inline is active, matching the hardware/protocol constraint. - rkey simplification: rocmIbPostFifo() inline path now writes only rkeys[0] directly instead of looping over all devices. - Env var consolidation: Removed the standalone RCCL_CTS_INLINE_DATA env param. rcclCtsInlineData is now derived from ncclIbUseInline. When CTS offload is enabled and IB inline is not already on, it is auto-enabled with a log message. - GDR flush default: GDR_FLUSH_DISABLE default changed from 0 to 1, matching AINIC intent. - Alignment assertions: Added static_assert for ncclIbSendFifoCtsInline <= 32B. Updated offsetof assert to use u.fifo. - Simplify FIFO MR registration and fix memset for inline mode. Simplify send-side and recv-side ibv_reg_mr to a single unconditional call using the union's larger member (ncclIbSendFifo, 64B). Since fifo and fifo_inline share the same base address in the union, registering the larger size covers both layouts. - Fix memset after send completion to clear the correct struct size: ncclIbSendFifoCtsInline (32B) for inline mode vs ncclIbSendFifo (64B) for standard mode, preventing a 64B zeroing of a 32B-entry region. - CTS inline idx comparison truncation and rkeys out-of-bounds read. Fix uint32_t vs uint64_t idx comparison in rocmIbIsend() inline path: cast idx to uint32_t to match ncclIbSendFifoCtsInline::idx field width, preventing sender stall/spin after fifoHead exceeds 2^32-1. - Fix out-of-bounds rkeys read in ncclIbMultiSend() inline path: use rkeys[0] instead of rkeys[qp->remDevIdx] since the CTS inline struct only has rkeys[1] (single element). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
karthikarum
force-pushed
the
users/karthikarum/drop_2026-02
branch
from
August 8, 2026 01:06
a862002 to
bf208ee
Compare
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
fix(rccl/net_ib_rocm): The AINIC CTS (Clear-to-Send) inline data path in RCCL's IB transport had multiple correctness and memory efficiency issues:
Problem
For AINIC (IbCastAinicRoce), the CTS FIFO supports two data layouts:
A 64-byte ncclIbSendFifo (standard) and a 32-byte ncclIbSendFifoCtsInline (compact, for inline data).
When IbCastAinicCtsInlineData is true, each slot's buffer is reinterpreted as a contiguous array of 32-byte elements
rather than 64-byte elements. The code had several issues preventing correct operation in the 32-byte layout:
Memory bloat: ncclIbSendComm and ncclIbRemFifo allocated both fifo (64B entries) and fifo_inline (32B entries) arrays as separate members, doubling FIFO memory even though only one format is ever active at runtime.
FIFO remote address miscalculation: rocmIbPostFifo() always computed the RDMA remote target offset using sizeof(ncclIbSendFifo) (64B stride), even when CTS inline mode was active and entries are 32B. This caused FIFO writes to land at wrong offsets, leading to data corruption or silent mismatches on the sender side.
Wrong FIFO struct accessed on send path: ncclIbMultiSend() and rocmIbIsend() unconditionally read metadata (nreqs, addr, rkeys, tag, size, idx) from the 64B ncclIbSendFifo layout, even when the receiver had written 32B ncclIbSendFifoCtsInline entries - reading garbage from the wrong memory layout.
Redundant rkey population: rocmIbPostFifo() looped over all devices (ndevs) to fill rkeys in the CTS inline struct, but the inline path only uses a single rkey (rkeys[0]).
Confusing / conflicting env knobs: rcclCtsInlineData had its own independent RCCL_CTS_INLINE_DATA env param, while ncclIbUseInline was also unconditionally forced to true for all AINIC - two knobs controlling overlapping behavior with no clear precedence.
GDR flush default mismatch: GDR_FLUSH_DISABLE defaulted to 0 (flush enabled), but AINIC always overrode it to 1 at runtime - the default didn't match the intended AINIC behavior.
Technical Details
Fix
Memory layout (structs): fifo/fifo_inline in ncclIbSendComm and elems/elems_cts_inline in ncclIbRemFifo are now wrapped in a named union u, so they overlay the same memory - halving the FIFO footprint.
FIFO remote address math: rocmIbPostFifo() now branches on rcclCtsInlineData to compute remote_addr using sizeof(ncclIbSendFifoCtsInline) (32B) vs sizeof(ncclIbSendFifo) (64B), so the RDMA write targets the correct offset.
Send-path struct dispatch: ncclIbMultiSend() and rocmIbIsend() now maintain a separate slots_inline pointer alongside slots, and branch on rcclCtsInlineData to read nreqs, addr, rkeys, tag, size, idx from the correct struct type.
maxRecvs clamped: rocmIbGetPhysProperties() now returns maxRecvs=1 when CTS inline is active, matching the hardware/protocol constraint.
rkey simplification: rocmIbPostFifo() inline path now writes only rkeys[0] directly instead of looping over all devices.
Env var consolidation: Removed the standalone RCCL_CTS_INLINE_DATA env param. rcclCtsInlineData is now derived from ncclIbUseInline. When CTS offload is enabled and IB inline is not already on, it is auto-enabled with a log message.
GDR flush default: GDR_FLUSH_DISABLE default changed from 0 to 1, matching AINIC intent.
Alignment assertions: Added static_assert for ncclIbSendFifoCtsInline <= 32B. Updated offsetof assert to use u.fifo.
Simplify FIFO MR registration and fix memset for inline mode. Simplify send-side and recv-side ibv_reg_mr to a single unconditional call using the union's larger member (ncclIbSendFifo, 64B). Since fifo and fifo_inline share the same base address in the union, registering the larger size covers both layouts.
Fix memset after send completion to clear the correct struct size: ncclIbSendFifoCtsInline (32B) for inline mode vs ncclIbSendFifo (64B) for standard mode, preventing a 64B zeroing of a 32B-entry region.
CTS inline idx comparison truncation and rkeys out-of-bounds read. Fix uint32_t vs uint64_t idx comparison in rocmIbIsend() inline path: cast idx to uint32_t to match ncclIbSendFifoCtsInline::idx field width, preventing sender stall/spin after fifoHead exceeds 2^32-1.
Fix out-of-bounds rkeys read in ncclIbMultiSend() inline path: use rkeys[0] instead of rkeys[qp->remDevIdx] since the CTS inline struct only has rkeys[1] (single element).
Issue Tracking
JIRA ID: AICOMRCCL-1558This also fixes JIRA ROCM-29123
Test Plan
Test Result
PASS.
Attached report:
RCCL_comparison_tot_vs_inline-change_8N_20260807.html