Repository navigation
[HIP] Fix ar_ll128_unroll2 hang on consecutive calls of different sizes - #6446
Conversation
ar_ll128_unroll2 derived its per-rank slot stride and bank offset from the current message size. When a rank ran ahead into the next call with a different size, its writes into the peer's scratch could land in slots the peer had not read yet from the previous call, overwriting payload and flag words, so the peer spun forever on a flag that never matched its epoch. Use a fixed per-rank stride sized for kLL128PocCapBytes so both banks and all rank slots have the same layout for every message size. The scratch footprint equals the previous worst case, which the reused tmp region already has to hold. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
Run back-to-back LL128 all-reduces of different sizes, with three sizes per round so each size alternates scratch banks, and check every result exactly. Rank 1 forces queue evictions so it falls behind inside the kernel, as on a busy host, and a watchdog turns a deadlock into a failure. Covers TP2 and TP4 under --regression. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
One backend per PR: PR title tags & labels: |
|
This was the second time that I m editing all reduce so I thought to adda unit test this time. |
|
The bank offset and the per-rank slot both used the current message size, so a faster rank could overwrite a slower rank's unread slots when the next call had a different size. The stride is now fixed for the 4 MiB cap, so every size stays in its own slot. gfx942 and gfx950 share this kernel. The new test returns at tp 8. Multi-GPU CI runs this file with no |
…ocation Multi-GPU CI runs the test file with no arguments, so the LL128 regression got the default TP8, which has no LL128 range, and returned without running. Run it at each of TP2 and TP4 that fits in the requested TP size instead. Also update the --regression help and the dispatch comment's worst-case scratch size, which the fixed stride moved to TP4. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
@ChuanLi1101 Done. With no On gfx942: agreed, it dispatches the same kernel with the same thresholds, so it gets the same fix. I only tested on gfx950, and multi-GPU CI runs on MI350X only, but I think it should be enough for the purposes of this PR unless the reviewers also would like gfx942 testing. |
ChuanLi1101
left a comment
ChuanLi1101
left a comment
There was a problem hiding this comment.
Thanks, this addresses my concern. The default multi-GPU CI invocation now runs both TP2 and TP4 regression cases. The shared gfx942/gfx950 kernel gets the same fix. LGTM.
…hang The regression forces rank skew by repeatedly evicting rank 1's queues. On a host where restoring the queues is slow, the evictions alone can starve a rank past the 30 s watchdog with no deadlock. When a window stalls, any rank now stops the evictions through a shared event and waits 60 s more; only a window that still does not finish is reported as a hang. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
Thanks for the fix. The PR is currently blocked by the required The important failure appears directly related to this PR: the newly added Could you please investigate why the fix is not taking effect on the CI runner (or whether the regression test needs adjustment)? A simple rerun may not be sufficient because this is the exact scenario the PR is intended to fix. There is also a separate |
The evictions that force rank skew behave differently with the host's KFD driver: on the multi-GPU CI runner they wedged the evicted rank itself, so the test failed with the fixed kernel. By default the regression now runs the mixed-size LL128 sequence with exact checks and a plain hang watchdog; AITER_TEST_LL128_EVICT=1 turns the evictions back on for local stress runs. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
@zufayu I hope now test is in a better state (I tested it locally 10 times in a loop) |
|
The CI failure is caused by a missing or incompatible MORI CCO installation on the MI350X runner, not by PR #6446; its custom all-reduce tests pass, while the gate only propagates the unrelated comm-fused MoE failure. |
|
FYI, #6571 has now been merged into Please update/rebase the branch and rerun CI. The remaining result to focus on is |
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
Merged main (including #6571). On the last run, |
|
@zufayu I think now it should be good. The failure seems to be some sort of log file not collected for some reason which should be resolvable by a retry or something |
4 participants
ar_ll128_unroll2, the gfx9 LL128 all-reduce from #5272, can deadlock when consecutive calls have different message sizes. We hit it in vLLM CI on MI355 (Kimi-K3 at TP2) as an RCCL watchdog timeout. This PR fixes the scratch layout and adds a regression test.Root cause and fix
The bank offset and per-rank slot stride are both derived from the current message size (
total_group * 128 * 2 / 4). Consecutive calls alternate banks, but those banks only stay disjoint when both calls have the same size. A rank that has all its peers' data for call k moves on to call k+1 while a slower peer is still reading call k. With a different size, its call-(k+1) stores can land on the peer's unread slots. That leaves the peer polling forever for the call-k flag, and every other rank then waits on the peer.A word-level model of the store and poll addresses predicts the overwritten range of every hang we captured: 16 at TP2 and 1 at TP4, all decoded with rocgdb. In the TP4 case, rank 2 is stuck in a 32768-element call. 136 of the 274 groups it polls in rank 3's slot, words
[61440, 70144), already hold the next epoch's flag, written by rank 0's 28800-element call. That is exactly the predicted range. Whether a size sequence is exposed depends on which bank each size lands in: at TP4,[32768, 28800]repeated never overlaps, while[32768, 32768, 28800]does.The fix uses a fixed per-rank stride,
kLL128SlotStride, sized forkLL128PocCapBytes. Every allowed size then fits within one slot, so a call-(k+1) store can never reach a call-k slot. The scratch now always spans the 4 MiB-cap worst case that the existing comment already reserves: 22.1, 40.0 and 75.8 MB at TP2, TP4 and TP8. That fits in the tmp region of both vLLM (256 MiB) and the AITER default (2 GiB). The LL128 region still starts atkLLPocRegionBytes, so this does not interact with the two-stage kernel, including the tail path from #5807. Launch configuration and dispatch are unchanged.Test
test_allreduce_ll128_mixed_size_regressionruns back-to-back LL128 all-reduces of three sizes per round and checks the results exactly. Rank 1 forces KFD queue evictions so it falls behind inside the kernel, as on a busy host, and a watchdog turns a deadlock into a failure. It runs under--regressionat TP2 and TP4; TP8 never dispatches LL128.MI355X (gfx950), ROCm 7.2.3. "Unpatched" is the shipped
v0.1.24.post1build; a local rebuild of the same source hangs the same way.[114688, 114688, 57344][32768, 32768, 28800]Performance
Unpatched and patched builds alternated on the same GPUs, 5 runs each. Values are mean ± sd.
ar_ll128_unroll2kernel time (rocprofv3)bench serve, Kimi-K3-8L dummy, TP2 (4096 in / 256 out, conc 16)The
test_allreduce_customsweep (CUDA graph, bf16, 3 runs) shows no change at any size beyond launch-overhead noise, which affects calls under about 12 µs. Above that, every size agrees within 0.6%.test_allreduce_custom sweep, TP2 and TP4
TP2
TP4
#5616 (gfx1250 LL128) uses the same size-dependent stride and would need the same change.
AI assistance (Claude Code) was used to investigate and draft this change. Every line was reviewed by the submitter.