Repository navigation
fix(mtp): keep side-stream D2H sources alive until the copy runs - #2654
Conversation
tokenIDProcessor copies the sampled ids, the MTP verdict (num_rejected / num_bonus) and the draft ids to the host on async_copy_stream. The sources are fresh default-stream allocations that the caller drops right after queuing the copy, so the caching allocator can hand their block to the next default-stream allocation before the side-stream copy has run. A late copy (work queued on the copy stream, or on another stream sharing its hardware queue) then reads the next step's tensor. Under MTP the stale num_bonus becomes num_accepted_tokens, which the GDN conv update kernel adds to the conv-state offset unchecked: an illegal memory access. Record each source on async_copy_stream so its block is not reused until the copy has executed. Co-authored-by: Cursor <cursoragent@cursor.com>
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
|
Looks correct — I traced the three |
Conflict in atom/model_engine/model_runner.py with #2670 (perf: reuse speculative output buffers and combine status D2H), which replaced send_to_cpu_async_draft / send_mtp_status_to_cpu_async with D2HQueue-backed send_draft_token_ids_to_cpu and spec_status_queue.send(spec_counts, ...). Resolution: take main's queue-based paths and move the hold into D2HQueue.send, which now records each CUDA source on its stream after queuing the copy. That covers both queue users on async_copy_stream (the spec status, a view of the sampler's fresh verdict, and the draft ids). send_to_cpu_async keeps _hold_for_copy_stream for the sampled ids and logprobs. The GPU test is ported to the new API; its positive control now disables Tensor.record_stream and runs for every path. Co-authored-by: Cursor <cursoragent@cursor.com>
VerifyScheduler.record_ell queues the ell D2H on the runner's async_copy_stream, the same stream and the same race as the tokenIDProcessor copies: ell is a fresh default-stream tensor that is dropped after the copy is queued, so its block can be reused before the copy has run. Hold it with tokenIDProcessor._hold_for_copy_stream, and cover the path in the GPU test. Suggested by @yitingw1 in review. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Merged Conflict: @yitingw1 thanks for spotting GPU test: Verified (MI355X,
|
Replace `_hold_for_copy_stream` / `record_stream` with explicit references kept next to the completion events that already exist, released only after that event has been waited on. No new CUDA events. - `send_to_cpu_async`: the sampled ids and logprobs go into the `cpu_tensor_handle` entry with `cpu_tensor` and `copy_done`; `recv_async_output` waits on `copy_done`, then pops the entry. - `D2HQueue` (spec status, draft ids): each slot holds its source; `receive` clears it after `copy_done.synchronize()`. New `drain()` waits for and drops every pending transfer. - DSpark `record_ell`: `ell` is held with its ring slot's event and released when `_ell_stage` reuses the slot after that event; a one-off buffer keeps `ell` with the copy's own event until it completes. `_ell_pending` eviction no longer decides the source's lifetime. - `tokenIDProcessor.clean()` waits for every queued copy before dropping the handles and queues. Tests: the GPU lifetime test covers spec status, sampled ids, logprobs, draft ids, DSpark ell (ring slot and one-off buffer) and clean() with copies in flight; its positive control strips the holds (for clean(), skips the wait) and requires every path to race. CPU tests check each source is released only after its event has been waited on. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@yitingw1 Changed it as you suggested, in
Verification (MI355X, CI aiter wheel):
The PR description is updated. Since the mechanism changed after your approval, could you take another look? |
|
@valarLip could you take a look at this one? Short version: side-stream D2H copies on This revision follows the earlier review suggestion: no Validation is in the comment above. On unpatched main, the deterministic lifetime test fails on all paths (20/20 trials read the next step's tensor); on this branch it passes in 5 of 5 runs. GSM8K with real MTP-3 verification is 0.978, and the CPU suite matches main. |
G1 carries two real citations pointing in opposite directions (#1369, H2D/compute-side; f08cd7f/#2654, D2H-side) and could never fire -- confirmed by re-running fetch.sh on ATOM#1369, which derived rules=5/25 with G1 absent. A rule with real citations that can never fire is the exact disease this project keeps finding. Add a `stream-lifetime` family: added lines that create/hand off a stream or make a cross-stream lifetime promise explicit (cuda.Stream(, .record_stream(, .wait_stream(, .set_stream(, .to(..., non_blocking=True)) surface both G1 (sync-ordering) and G1b (the record_stream-vs-wait_stream criterion) together. Move both out of UNREACHABLE_BY_DESIGN into ALL_RULES. Demonstrated firing on ATOM#2654 (f08cd7f): rules=7/27, [stream-lifetime] G1 G1b. Still does not fire on #1369 itself -- that PR's diff has no stream API call at all, just a bare tensor handoff -- reported honestly in rules.md rather than re-tuned to force a match on the PR that motivated the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Keep the GPU sources of the side-stream D2H copies on
tokenIDProcessor.async_copy_streamalive until those copies are known to have completed. Each source is held next to the completion event that already exists for its copy, and released only after that event has been waited on. Without this, the caching allocator can hand the block to the next step's allocation on the default stream while the copy is still queued, and the host reads the next step's bytes as this step's MTP status / sampled ids / draft ids / DSparkell.Under MTP this is what produces the GDN
_causal_conv1d_update_kernelillegal memory access seen with in-process LMCache offload (#2636): a stalenum_bonusof 16384 becomesnum_accepted_tokens = 16385, which the conv kernel adds to the state offset unchecked.No
record_stream, no new CUDA events: the earlier_hold_for_copy_stream/record_streamversion is gone, and every hold reuses an event the code already had.Motivation
https://github.com/wuhuikx/LLM-Booster/issues/157 — with
lmcache_offload+ MTP 3 on Qwen3.8-Flash-Next (#2636), AgentX conc 32/64 crashed within 13–54 min with an illegal memory access in the GDN conv update kernel. The faulting offset (slot * 6 + num_accepted_tokens - 1 = 16487,16487 mod 6 = 5) is impossible for any validnum_accepted_tokens(1..4), so the bad value isnum_accepted_tokens, not the slot.num_accepted_tokenscomes fromnum_bonus + 1, andnum_bonuscomes from the spec-status D2H queued onasync_copy_stream(nowSpecStatusQueue, aD2HQueue). Its source is a view of the rejection sampler'sverdict, a freshtorch.emptyon the default stream every step, and nothing holds it after postprocess: once the last reference is dropped the block is free for the default stream's next allocation. If the side-stream copy runs after that allocation is written, the host gets the new contents.The copy runs late whenever
async_copy_stream, or a HIP hardware queue it shares with other streams (defaultGPU_MAX_HW_QUEUES=4), has work in front of it. In-process LMCache adds its own pack/copy streams with kernels on them, which is why this only showed up with offload on; without MTP there is no status D2H, which is why spec 0 never crashed. The bug itself is onmain, so the fix is proposed againstmain.Caught in production with host-side checks added only to a debug build (not in this PR), c32 + 160 GiB CPU pool, #2636 + #2606:
16384 is
--max-num-batched-tokens: the next step's prefill metadata landed in the freedverdictblock. The step was a decode step immediately followed by a prefill step while LMCache was storing 32k tokens on its own stream. The request's state load had succeeded, so offload correctness is not involved.A micro-benchmark (bs 32, the same send/recv code path) reproduced it deterministically (measured with the first,
record_streamversion of this fix):async_copy_streamGPU_MAX_HW_QUEUES=4)GPU_MAX_HW_QUEUES=1Changes
Contract (commented at
async_copy_stream): every D2H queued on that stream keeps its GPU source referenced until the copy is known to have completed. The five sources on that stream:cpu_tensor_handleentry(cpu_tensor, copy_done, (gpu_tensor, gpu_logprobs))recv_async_outputwaits oncopy_done, then pops the entrycopy_done)verdict[:2], the crash path)_D2HSlot.sourceofSpecStatusQueueD2HQueue.receiveaftercopy_done.synchronize()_D2HSlot.sourceof the draft-idD2HQueueell(record_ell)_ell_slot_source[slot], next to_ell_slot_event[slot]; one-off buffers:_ell_oneoff_inflightas(event, ell)with the copy's own event_ell_stagereuses the slot after its event completed; one-off entries are pruned once their event has completedellis deliberately not held through_ell_pending: that list evicts entries whose copy may still be in flight (del self._ell_pending[:-_MAX_ELL_INFLIGHT]), andell_by_req = {}clears it outright.D2HQueue.drain()waits for every pending transfer, then drops its output and source.tokenIDProcessor.clean()waits on every queuedcopy_doneand drains both queues before it rebuilds them, so dropping the handles cannot free a source under an in-flight copy.tokenIDProcessor._hold_for_copy_streamand bothrecord_streamcalls.Tests
tests/test_side_stream_d2h_source_lifetime.py(GPU only, skipped on CPU): parks the copy stream with atorch.cuda._sleepspin on a stream that does not share the default stream's hardware queue, queues the copy, drops the source, allocates and writes the next step's tensors, and checks the host got this step's values. Paths: spec status, sampled ids, logprobs, draft ids, DSparkell(ring slot), DSparkell(one-off buffer), andclean()with all copies still in flight. The positive control monkeypatches the holds away (forclean(): skips its wait, holds intact) and requires every path to race, so the test cannot pass vacuously.test_output_transfer.py(a slot's source is alive at itscopy_donewait and released afterreceive/drain),test_dspark_scheduler.py(elloutlives its_ell_pendingentry and the setter, and is released only when its slot is reused after the event; one-offellheld until its own event lands),test_output_transfer_runner.py(handle owns ids and logprobs untilrecv_async_outputhas waited;clean()waits on every queued copy before dropping it; needs AITER to import the runner, like the rest of that file).Accuracy
GSM8K (
shared/scripts/gsm8k.sh: 5-shot, chat,max_tokens=12288,num_concurrent=64, temperature 0), MTP 3 with real verification (no--spec-decode-acceptance-length):40db24c2+ this PR (hold-refs version)5fea029d+ this PR (record_streamversion)5fea029d(control)record_streamversion)lmcache_offload(160 GiB CPU)Performance
Measured with the first,
record_streamversion of this fix; the hold-refs version keeps the same blocks out of the allocator for about the same time (until the host waits on the copy, which it does the next step anyway), so no difference is expected.AgentX (
inferencex-agentx-mvp, seed 42, 3600 s profiling, 1 GPU per point, MTP 3 with--spec-decode-acceptance-length 2.32as in the original crash runs),effective_total_throughputper GPU:"#2636" here includes its
183cfec2(one LMCache lookup socket per server); before that commit, servers started together in one container could query each other's LMCache and those numbers are not comparable.Without this PR, 4 of 5 one-hour offload runs on the first machine crashed 13–54 min in (c32 + 160 GiB at about 70.8k tok/s/GPU before the crash, c64 + 288 GiB at about 55.7k; c32 + 96 GiB crashed twice), and on this machine the debug check caught
num_accepted_tokens = 1638512 min (c32 + 160 GiB) and 17 min (c64 + 288 GiB) into profiling. With it, every point above ran the full 3600 s. The two c64 runs differ by 8%. The first overlapped with up to 6 other servers on the host and the repeat ran its last 30 min with only one, so host-side LMCache copy bandwidth is the likely cause (not measured); treat c64 as ~57–62k.How to use
No flags. Nothing changes for users other than the crash going away.
Test plan
Hold-refs version (
88a00dd9, main40db24c2merged), MI355X,rocm/atom-dev:latest+ CI aiter wheelamd_aiter-0.1.1.dev1+g508736740:black26.10.1 /ruff0.17.2 clean on changed files.tests/test_side_stream_d2h_source_lifetime.py: 14 passed, 5/5 runs on one GPU plus one run on a second GPU. Control counts, 50 trials per path: with the holds 0/50 stale on every path; without them 50/50 stale on every path (spec status, sampled ids, logprobs, draft ids, ell slot, ell one-off,clean()).main40db24c2: all 7 paths fail, 20/20 trials each read the next step's tensor.test_output_transfer.py,test_output_transfer_runner.py,test_spec_deferred_status_queue.py,test_dspark_scheduler.py,test_gdn_num_accepted_tokens.py: 111 passed, 2 skipped (opt-in) vs 103 passed, 2 skipped onmain; the 8 extra are the new CPU tests..github/scripts/run_unit_tests.sh, CPU torch, no aiter): 3 failed / 9814 passed / 1007 skipped vs 3 / 9808 / 1006 onmain40db24c2. Same 3 failures (test_graph_markerx2,test_sparse_indexer_side_effect_op); the per-test diff is exactly the 6 new CPU tests and the GPU-only file skipping.Earlier (
record_streamversion, same paths held):lmcache_offload+ MTP 3 real verification: 0.978, acceptance 72.1%.Limitations
num_accepted_tokens; a bounds assert there would have turned a silent illegal access into a clear error, but it costs a sync per step and is left out.Retrieved, the pair still fails and recomputes). That is a capacity issue in [Qwen 3.8] LMCache offload for QSA + GDN + PLE state #2636 and is tracked there, not here.