Repository navigation
rccl-tests/gin-sdma: cap A2A GIN put to 128 MiB to fix >1 GiB SDMA hang #9927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
4df1c45
d24881b
fbc6ba7
852d20d
2401bde
3db8170
73e0a47
579861c
51c2af8
92b25f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,10 +248,10 @@ __global__ void GinAlltoAllKernel(ncclWindow_t sendwin, size_t sendoffset, ncclW | |
| int tid = threadIdx.x + blockIdx.x * blockDim.x; | ||
| int nthreads = blockDim.x * gridDim.x; | ||
|
|
||
| /* send to all peers via GIN */ | ||
| /* send to all peers via GIN (chunked to <=128 MiB/put; see common.h::ginPutChunked) */ | ||
| const size_t size = count * sizeof(T); | ||
| for (int r=tid; r<devComm.nRanks; r+=nthreads) { | ||
| gin.put(ncclTeamWorld(devComm), r, | ||
| ginPutChunked(gin, ncclTeamWorld(devComm), r, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will applications call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and that's exactly what 92b25f7 does. There's no |
||
| recvwin, recvoffset + devComm.rank * size, | ||
| sendwin, sendoffset + r * size, | ||
| size, ncclGin_SignalInc{signalIndex}); | ||
|
|
@@ -298,13 +298,13 @@ __global__ void HybridAlltoAllKernel(ncclWindow_t sendwin, size_t sendoffset, nc | |
| int nthreads = blockDim.x; | ||
|
|
||
| for (int r = tid; r < startLsa; r += nthreads) { | ||
| gin.put(world, r, | ||
| ginPutChunked(gin, world, r, | ||
| recvwin, recvoffset + world.rank * size, | ||
| sendwin, sendoffset + r * size, | ||
| size, ncclGin_SignalInc{signalIndex}); | ||
| } | ||
| for (int r = startLsa + lsaSize + tid; r < world.nRanks; r += nthreads) { | ||
| gin.put(world, r, | ||
| ginPutChunked(gin, world, r, | ||
| recvwin, recvoffset + world.rank * size, | ||
| sendwin, sendoffset + r * size, | ||
| size, ncclGin_SignalInc{signalIndex}); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,12 @@ | |||||||||||||||||||||
| #define NCCL_TESTS_VERSION "2.18.3" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #include "rccl/rccl.h" | ||||||||||||||||||||||
| // GIN-SDMA shared policy constants (single source of truth for the put-chunk | ||||||||||||||||||||||
| // cap that ginPutChunked below clamps to). Cherry-picked as a standalone, | ||||||||||||||||||||||
| // namespaced (gin_sdma::), no-GPU/no-RCCL header from | ||||||||||||||||||||||
| // users/dondai/gin-stage2j-sdma-A1457-a2av-wip to fix A2A >1 GiB WITHOUT | ||||||||||||||||||||||
| // pulling in the other GIN-SDMA collective kernel designs. | ||||||||||||||||||||||
| #include "gin_sdma_collective_policy.h" | ||||||||||||||||||||||
| // nccl_device.h provides the device-API public types referenced below | ||||||||||||||||||||||
| // (ncclCommProperties_t, full ncclDevCommRequirements, NCCL_GIN_TYPE_NONE, ...). | ||||||||||||||||||||||
| // As of NCCL 2.29 these types are referenced unconditionally by the test engine | ||||||||||||||||||||||
|
|
@@ -412,6 +418,64 @@ extern int is_main_proc; | |||||||||||||||||||||
| extern thread_local int is_main_thread; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #if defined(ENABLE_DEVICE_API) && NCCL_VERSION_CODE >= NCCL_VERSION(2,28,0) | ||||||||||||||||||||||
| // Overflow-safe AND hang-safe gin.put for the Anvil-SDMA backend. Two distinct | ||||||||||||||||||||||
| // single-descriptor limits apply, and we clamp to the smaller of the two: | ||||||||||||||||||||||
| // 1) Correctness: the SDMA linear-copy count field is 30 bits and 1-based | ||||||||||||||||||||||
| // (count = bytes-1), so a put of >1 GiB (kGinPutMaxBytes) silently | ||||||||||||||||||||||
| // truncates and corrupts data (a 2 GiB transfer copies only 1 GiB). | ||||||||||||||||||||||
| // 2) Reliability: on MI355X + ROCm 7.13 a single copy descriptor at/above | ||||||||||||||||||||||
| // 256 MiB stalls the SDMA engine on the fused copy+signal packet, so the | ||||||||||||||||||||||
| // copy never lands and the SignalInc never fires -> waitSignal hangs. A | ||||||||||||||||||||||
| // 128 MiB cap (gin_sdma::kGinSdmaSafeCopyBytes) is measured hang-free with | ||||||||||||||||||||||
| // no bandwidth loss; see that constant for the evidence. | ||||||||||||||||||||||
| // Split the transfer into <=kGinSdmaSafeCopyBytes segments and carry the | ||||||||||||||||||||||
| // caller's remote action (e.g. SignalInc) ONLY on the final segment. A | ||||||||||||||||||||||
| // <=128 MiB message is a single put with no extra overhead. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // ORDERING PRECONDITION (why the trailing signal is safe): the Anvil-SDMA | ||||||||||||||||||||||
| // backend selects the hardware SDMA queue per *issuer*, as | ||||||||||||||||||||||
| // effectiveChannel = (blockId*warpsPerBlock + warpId)*stride % numChannels | ||||||||||||||||||||||
| // (gin_anvil_sdma.h) -- NOT per peer. A single hardware queue is a strict | ||||||||||||||||||||||
| // in-order FIFO; distinct channels are independent completion domains with NO | ||||||||||||||||||||||
| // cross-queue ordering (only ncclGinApi_Flush drains all of a peer's channels). | ||||||||||||||||||||||
| // Because this loop runs entirely within ONE thread, every segment of this | ||||||||||||||||||||||
| // message -- data segments AND the final signal-carrying put -- resolves to the | ||||||||||||||||||||||
| // same (blockId, warpId) and therefore the same in-order queue, even when | ||||||||||||||||||||||
| // NCCL_GIN_ANVIL_SDMA_NUM_CHANNELS > 1. So the final SignalInc is FIFO-ordered | ||||||||||||||||||||||
| // behind all prior segments of this message and "signal fired" still means "the | ||||||||||||||||||||||
| // whole message landed"; per-message waitSignal accounting is unchanged. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // CALLER CONTRACT: issue an entire per-peer message (all segments + the | ||||||||||||||||||||||
| // signal-carrying final put) from a SINGLE thread/warp, as the AllToAll GIN | ||||||||||||||||||||||
| // sites do (one thread owns each disjoint (peer, offset) tuple). Do NOT split a | ||||||||||||||||||||||
| // single peer's message across multiple warps/CTAs while carrying the signal on | ||||||||||||||||||||||
| // only one of them: under NUM_CHANNELS > 1 the segments could land on different | ||||||||||||||||||||||
| // queues and the SignalInc could fire before an earlier segment completes | ||||||||||||||||||||||
| // (silent data corruption). If you ever need that pattern, drain with a Flush | ||||||||||||||||||||||
| // (or force NUM_CHANNELS=1 / SPREAD_CHANNELS=0) before signaling. | ||||||||||||||||||||||
| template <typename RemoteAction> | ||||||||||||||||||||||
| __device__ __forceinline__ void ginPutChunked( | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no regression test for the chunking here. Could we add the following test cases?
Today there's no AllToAll test in rccl-tests and the pytest
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I will add my test code into the "rccl-tests/" directory.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added all three in 73e0a47.
The two GPU tests need an 8x MI355X-class node + MPI + a GIN-SDMA build, so the module skips unless
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ran both test tiers on hardware: Host unit test — built googletest 1.14.0 and compiled GPU AllToAll (8x MI355X, ROCm 7.13, NCCL_GIN_TYPE=5, force-single-channel) —
Running the suite on-hardware surfaced two gaps I fixed in 51c2af8:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added and relocated. The host unit test is now |
||||||||||||||||||||||
| ncclGin& gin, ncclTeam team, int peer, | ||||||||||||||||||||||
| ncclWindow_t dstWin, size_t dstOff, | ||||||||||||||||||||||
| ncclWindow_t srcWin, size_t srcOff, | ||||||||||||||||||||||
| size_t bytes, RemoteAction finalAction) { | ||||||||||||||||||||||
| const size_t kMax = gin_sdma::kGinSdmaSafeCopyBytes < gin_sdma::kGinPutMaxBytes | ||||||||||||||||||||||
|
dlamd1dai marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||
| ? gin_sdma::kGinSdmaSafeCopyBytes | ||||||||||||||||||||||
| : gin_sdma::kGinPutMaxBytes; | ||||||||||||||||||||||
| size_t off = 0; | ||||||||||||||||||||||
| do { | ||||||||||||||||||||||
| const size_t rem = bytes - off; | ||||||||||||||||||||||
| const size_t seg = rem > kMax ? kMax : rem; | ||||||||||||||||||||||
| if (off + seg >= bytes) { | ||||||||||||||||||||||
| // Final (or only) segment carries the signal / remote action. | ||||||||||||||||||||||
| gin.put(team, peer, dstWin, dstOff + off, srcWin, srcOff + off, seg, finalAction); | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| gin.put(team, peer, dstWin, dstOff + off, srcWin, srcOff + off, seg); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| off += seg; | ||||||||||||||||||||||
| } while (off < bytes); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
wenkaidu marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||
| template <typename F> | ||||||||||||||||||||||
| testResult_t testLaunchDeviceKernel(F kernel, void* sendbuff, size_t sendoffset, void* recvbuff, size_t recvoffset, size_t count, ncclDataType_t type, ncclRedOp_t op, int root, ncclComm_t comm, cudaStream_t stream) { | ||||||||||||||||||||||
| if (kernel == nullptr) return testNotImplemented; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GinAlltoAllKernelandHybridAlltoAllKernelare the entry points for the other GIN backends as well, not just SDMA, so this will cap puts for proxy, and rocshmem gda tooIt would be good to validate with NCCL_GIN_TYPE=2 and 4
Would it make sense to put the chunking in
ncclGinApi_Put<NCCL_NET_DEVICE_GIN_ANVIL_SDMA>instead, the way the proxy backend already does?That would fix the hang for all GIN SDMA callers and not just alltoall_perf
rocm-systems/projects/rccl/src/include/nccl_device/gin/proxy/gin_proxy.h
Lines 214 to 242 in ea5a1ab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GDA gin has the same limitation (1GB message size), but I agree with Nusrat that this should not surface in user code.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — you're right that
GinAlltoAllKernel/HybridAlltoAllKernelare shared by all GIN backends, so capping there was not necessary for proxy and GDA. Fixed in 92b25f7: the size cap now lives inside the SDMA backend's Put API (ncclGinApi_Put<NCCL_NET_DEVICE_GIN_ANVIL_SDMA>), and the kernels are back to a plaingin.put(). Proxy (type 2) and GDA (type 4) are no longer capped by construction — they don't touch the SDMA segmentation path at all.I haven't been able to get a clean type 2/4 datapoint yet (the RDMA host I have, dell300x, has a broken
bnxt_reABI in-container), but since the kernel path for those types is now byte-for-byte the pre-PRgin.put(), there's no behavioral change to validate. SDMA (type 5) is validated on 8x MI355X up to 2 GiB — no hang, 0 wrong, ~426 GB/s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on both points. As of 92b25f7 the chunking is entirely inside the SDMA backend
gin.put()and no longer in user/test code. On the GDA 1 GiB limit: this PR scopes its fix to SDMA only (GDA's kernel path is unchanged here). The same internal-segmentation pattern could be applied to the GDA backend's Put as a follow-up if we wantgin.put()to be size-agnostic there too — happy to file that separately.