[Bugfix] Record aux-stream MoE allocations on the consuming stream - #1
Closed
rchalamala wants to merge 1 commit into
Closed
rchalamala wants to merge 1 commit into
rchalamala wants to merge 1 commit into
Conversation
Tensors produced inside `with torch.cuda.stream(aux)` belong to the caching allocator's aux-stream pool. Events and wait_stream order the consumer after the producer, but they do not stop the allocator from handing the block to a later aux-stream allocation while the main stream is still reading it, so the consumer can read freed-and-reused memory. Add `record_stream_if_safe()` and apply it to the aux-stream allocations that escape to the main stream: `maybe_execute_in_parallel`'s `result1`, `execute_in_parallel`'s aux results, `SharedExperts`'s output, and the all-reduce output in `LatentMoERunner`, which rebinds `shared_output` to a new aux-stream tensor after the input was tagged. The helper no-ops during CUDA graph capture, where allocations come from the graph-private pool and are not recycled. Multi-stream overlap is unchanged -- captured graphs replay their stream DAG concurrently, so suppressing it there would cost decode latency. Co-authored-by: Devin Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Rahul Chalamala <22563365+rchalamala@users.noreply.github.com> Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Purpose
Aux-stream MoE allocations that escape to the main stream are never handed to
Tensor.record_stream(), so the caching allocator can recycle a block while the main stream is still reading it.The sites all look correctly synchronised, and for execution order they are — events and
wait_streamdo make the consumer run after the producer. But ordering is not lifetime. A tensor allocated insidewith torch.cuda.stream(aux)is owned by the allocator'sauxpool; when its Python reference dies, the block is returned to that pool with onlyaux's completion tracked. The nextauxallocation can take the block while the main-stream consumer's kernel is still reading it.record_streamis the mechanism that defers reuse until the consuming stream is done, and this repo already documents exactly that invllm/v1/worker/gpu/spec_decode/utils.py:40-42:Four escaping allocations were missing it:
SharedExperts.forward(shared_experts.py) andexecute_in_parallelhave the same shape.shared_experts.pyalso carried a comment asserting the tag was unnecessary, which conflates the two guarantees:Symptom class: intermittent, scheduling-dependent corruption or unmapped reads (XID 31) in a bystander kernel downstream of the MoE — the faulting kernel is whichever one happens to consume the recycled block, not the buggy one. Every site is normally rescued by the next iteration's
wait_stream/event.record, which is why it is rare rather than deterministic.Scope, and what this deliberately does not do. The helper no-ops during CUDA graph capture (graph-private pool allocations are static, not recycled) so this is an eager-path fix. An earlier draft also suppressed multi-stream overlap during capture on the theory that captured side-stream work gets no overlap at replay. That theory is wrong and was dropped — measured on B300 (2×4096² bf16 GEMMs, 16 per graph, 20 replays):
A graph captured from two streams replays its DAG concurrently, so suppressing overlap under capture would cost real decode latency on small-batch decode. Behaviour under capture is therefore unchanged.
Affects every model using shared experts / the multi-stream helpers (Kimi K3 latent-MoE, DeepSeek shared experts, the DeepSeek-V4 attention path via
execute_in_parallel). Found while investigating a K3 DFlash warmup crash on 8×B300 — see the honesty note under Test Result.Test Plan
Test Result
pre-commit run: all hooks pass (ruff, mypy-3.10, SPDX,check-torch-cuda-call, lazy-import, docstring/config validators).Standalone harness on B300, exercising the patched
multi_stream_utils.pyagainst real capture and replay — 8/8:Also verified directly that an unguarded
record_stream()inside capture does not raise on torch 2.11 — so the guard is for correctness-of-meaning, not to avoid an exception.Pending:
pytest tests/utils_/test_multi_stream_utils.pyhas not run yet — it needs a compiled tree (vllm._C_stable_libtorch) and the source build of this branch is still in progress. The logic under test is what the standalone harness above already covers on the same GPUs. I will post the pytest output when the build lands.Honesty note on the motivating crash. This came out of a source review of a K3 DFlash k=16 warmup crash (
XID 31MMU fault →CUBLAS_STATUS_EXECUTION_FAILEDin the drafter'sfcGEMM). I have not reproduced that crash against this patch, so I am not claiming this fixes it. The reported producer/consumer pair was not confirmed either:vllm/v1/worker/gpu/spec_decode/dflash/andqwen3_dflash.pycontain no stream or event operations at all, so the DFlashcombine_hidden_statespath is single-stream. What is defensible from source is the lifetime hazard fixed here, and the fact that warmup is the phase most exposed to it — all four multi-stream gates are upper-bound token thresholds (256/256/512), so multi-stream is ON for warmup's 1-2 request batches, andwarmup.py:329-341deliberately enumerates mixed spec/non-spec batches whose shapes miss the captured uniform-decode graphs and run eager.Not a duplicate. Checked open PRs (
gh pr list --repo vllm-project/vllm --state open --search "record_stream" / "shared experts stream" / "multi_stream_utils"). Nearest neighbours are vllm-project#33225 (replaceswith torch.cuda.stream()by thetorch.Streamcontext-manager form — pure syntax, no lifetime change, but it edits adjacent lines in the olderfused_moe/layer.pyshared-experts block, so expect a trivial conflict if both land) and vllm-project#45452 (opt-in K2.6-NVFP4 two-stream perf flags, default-off, adds overlap rather than fixing tagging). Neither addsrecord_streamto an escaping aux allocation.AI assistance was used for this change: the source review, patch, tests and measurements were produced by Devin. A human must review every line and defend it before this goes upstream.
Upstreaming. This branch targets
k3-pr50000. Two of the three files —vllm/utils/multi_stream_utils.pyand.../runner/shared_experts.py— are byte-identical onvllm-project/vllmmain (checked at15d65f8) and carry the bug verbatim, so those two hunks are directly upstreamable as a standalone PR.latent_moe_runner.pydoes not exist on main and arrives with the K3 drop, so that hunk belongs on PR vllm-project#50000.Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Link to Devin session: https://modal.devinenterprise.com/sessions/857595e915f9407db96cd4e8140caa24
Requested by: @rchalamala