Repository navigation
[unified-memory] Mark the write loc physical and check it at every write door (4/7) - #40328
Merged
Merged
Conversation
caihuali95
requested review from
BBuf,
DarkSharpness,
Fridge003,
HaiShaw,
HydraQYH,
Qiaolin-Yu,
Ying1123,
alphabetc1,
celve,
hanming-lu,
hebiao064,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
kpham-sgl,
merrymercy,
pyc96,
xiezhq-hermann,
yizhang2077 and
yuan-luo
as code owners
September 19, 2026 09:55
This was referenced Sep 19, 2026
Merged
ch-wan
force-pushed
the
mainline/write-loc-id-space
branch
from
September 28, 2026 19:53
3e068f0 to
548f1d3
Compare
ch-wan
requested review from
JustinTong0323,
sogalin,
wisclmy0611 and
zijiexia
as code owners
September 28, 2026 19:53
ch-wan
force-pushed
the
mainline/write-loc-id-space
branch
2 times, most recently
from
September 29, 2026 01:33
1d0454d to
7a62b39
Compare
ch-wan
force-pushed
the
mainline/write-loc-id-space
branch
from
October 1, 2026 06:21
503098a to
b9e8a96
Compare
A pool's write door takes ids in one of two spaces: VIRTUAL ids, which the
allocator hands out and the radix / accept / free paths key on, and
KERNEL-FACING ids, which address the pool's buffers. A unified pool needs the
second; every other pool's allocation is already physical, so the two coincide.
Nothing said which one a given loc was. Handing a unified pool a virtual loc
was therefore silent: virtual ids sit inside the OOB probe's bounds, so the
write landed on the wrong slots and only the output was wrong.
Make the space explicit, at the three places that know it:
KVWriteLoc.id_space "virtual" (the default) or "kernel".
KVWriteLoc.for_batch(fb,..) builds one from a batch, inheriting the
batch's space -- so a producer cannot forget.
ForwardBatch.out_cache_loc_id_space
the batch's own space, stamped "kernel" by
KVIndexTranslator.rebind_write_loc.
KVCache._check_write_loc_space
a pool that sets requires_translated_write_loc
refuses anything not marked kernel-facing.
Gated on SGLANG_ENABLE_ASYNC_ASSERT, like the
OOB probe next to it.
rebind_write_loc now runs on non-translating pools too, to stamp the space.
It still rebinds -- and still records out_cache_loc_virtual -- only when the
pool actually translates, so nothing on the plain path changes shape.
HybridLinearKVPool's two write doors take `loc_info` rather than `loc`: they
forward to a sub-pool, so they have to pass the marker along instead of
unwrapping to a bare tensor. The MLA branch's duplicated write_loc derivation
is hoisted to the one place both branches use.
The producers are stamped in the next commit; until then the check has nothing
marked to accept, so it is inert outside the tests.
…cers
Adopt the contract at every site that produces a write loc. Two shapes, and
nothing else:
KVWriteLoc.for_batch(forward_batch, loc, ...) at the attention backends'
write doors, replacing the bare tensor or KVWriteLoc(...) they passed.
The loc still comes from the same expression; for_batch only attaches
the batch's id space, so a door cannot forget it.
out_cache_loc_id_space="kernel" at the capture-time batch
constructors. These build a ForwardBatch outside `init_new`, so no
rebind runs over them; they fill out_cache_loc with ids the runner
already translated (or with sink ids), and must say so themselves.
With the producers stamped, a unified pool's write door now rejects a loc
that reached it without a rebind, instead of writing to the slots the
untranslated ids happen to name.
aiter's doors are stamped too. Its loc is `forward_batch.out_cache_loc`,
the same expression the other backends pass, so the ids were already correct
-- what it lacked was the mark, which would have made the unified door refuse
a healthy write under SGLANG_ENABLE_ASYNC_ASSERT and left its doors outside
the guard. It is not reachable on this hardware, so it joins the list below.
The DCP verify case in test_trtllm_mla_family_dcp_metadata.py stubs the
batch without an id space, which the trtllm_mla write now reads through
`KVWriteLoc.for_batch`; the stub carries the ForwardBatch default, "virtual".
aiter, fa4, trtllm_mla, cutedsl_mla and tokenspeed_mla are admitted under
--enable-unified-memory and supported by this change, whose stamped write
doors include theirs, but not yet validated: every validation run was on H20
(SM90), where their kernels do not run. The same holds for trtllm_mha's
SM100 kernels (trtllm-gen context and decode) and for decode context
parallelism (--dcp-size > 1). Reviewers with SM100-class hardware: please
run them before merging.
The write loc's id space was a two-valued string ("kernel" / "virtual"): a
typo read as "not kernel", and "kernel-facing" is the vocabulary this stack
retires now that a kernel-facing id is the physical token id. Name the fact
instead: `ForwardBatch.out_cache_loc_is_physical`, `KVWriteLoc.physical`,
`write_loc_is_physical()` and `requires_physical_write_loc`. Behaviour is
unchanged: `rebind_write_loc` still marks the batch's loc on every pool,
translating or not.
Also correct the docstrings this touches: a unified pool's write door, not
the kernel, refuses an unmarked loc, and unified pools set the flag rather
than reset it.
fa3 on the unified MLA allowlist wrote its latent rows with a bare `cache_loc`, so a unified MLA pool read the write as unmarked. The mark was also stamped by the batch on tensors it does not describe: `for_batch` took any `loc`, and backends passed `encoder_out_cache_loc` for cross-attention layers, trtllm_mla its capture buffer, and triton its DCP rank-local loc through it or bare. - `KVWriteLoc.for_batch` now wraps `out_cache_loc` only; `for_layer` picks the cross-attention `encoder_out_cache_loc` (never translated, so never marked) or the batch's loc. - fa3 / flashinfer / aiter / zigzag write through `for_layer`; the MLA and trtllm_mha backends drop the `cache_loc` alias that repeated `out_cache_loc`. - trtllm_mla's `_kv_write_loc` returns the `KVWriteLoc`: its capture buffer is filled by `fill_capture_write_loc`, so it is marked physical itself. - triton's DCP loc (`out_cache_loc // dcp_size`) keeps the batch's mark; dspark's and dflash's context writes into their static draft pools mark their slot ids physical. The ratchet that checked source text in three directories becomes an AST census over every allowlisted attention backend (tied to kv_cache_hook's allowlists) and the model, CP, speculative, executor and disaggregation code: every write-door call's loc must resolve, through local names, parameters and helper returns, to a marked `KVWriteLoc`.
The unified write doors checked the physical mark only under SGLANG_ENABLE_ASYNC_ASSERT, a gate the probe it replaced needed for its device-side value check. This check reads a flag on a dataclass, with no device sync, and runs at capture time rather than per replay, so it now runs on every write and raises a ValueError. With every producer that can reach a unified pool marked, production is guarded against a loc that skipped `rebind_write_loc`, which is otherwise in range and writes the wrong slots.
Producers now hand a KVWriteLoc to every pool, static pools included, but five overrides still indexed with their loc argument directly: MLATokenToKVPoolFP4.set_mla_kv_buffer (so an MLA model with an FP4 KV cache failed on its first write), HiSparseDSATokenToKVPool's two doors, LayerSplitDSATokenToKVPool.set_mla_kv_buffer and PageInterleaveMLATokenToKVPool.set_mla_kv_buffer. Each now takes loc_info and unwraps it, like its siblings. A census in test_physical_write_loc.py requires every pool write door under srt/ to unwrap its loc argument, forward it untouched to another door, take only *args, or only raise. It flags all five overrides on the previous head.
The DCP branch of TritonAttnBackend._set_kv_buffer wraps the rank-local loc in a KVWriteLoc and then compared positions.numel() to loc.numel(). KVWriteLoc has no numel(), so every triton run with --dcp-size > 1 raised AttributeError on its first KV write. Compare against the wrapped tensor instead. test_dcp_layout_unit.py now drives the DCP write with a stub pool and checks the rank-local loc, its physical mark and the position mask.
KVWriteLoc.for_batch and for_layer read forward_batch.out_cache_loc_is_physical, so batch stubs that drive a write path need the field. The zigzag materialize test (test_cp_strategy_unit.py) and the aiter fused-fp8 write tests (test_fused_fp8_kv_write.py) built theirs without it and now raised AttributeError. Also move the plan-marker assertions in test_tbo_filter_batch_marker.py back into the plan-marker test; the new mark tests had been inserted in the middle of it.
KVWriteLoc.for_batch also carries swa_loc / full_loc under the batch's mark, which holds only because both are derived from the rebound out_cache_loc; say so. The physical check runs on every write-door call, not on fused writers that scatter through get_kv_buffer or on a replayed cuda graph; say that too.
ch-wan
force-pushed
the
mainline/write-loc-id-space
branch
from
October 2, 2026 00:10
b9e8a96 to
7f8e884
Compare
This was referenced Oct 2, 2026
5 tasks done
This was referenced Oct 11, 2026
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.
4/7 — mark the write loc physical, and check the mark at every write door
Base: #38592 (3/7). 9 commits, 45 files, +935/−132.
Why
A unified pool's write door needs physical token ids. The allocator hands out virtual ids,
and
KVIndexTranslator.rebind_write_loctranslates the batch's write loc atForwardBatchconstruction; every other pool allocates physically, so the two coincide. Under 3/7's
token-major views a virtual id is in range and, by value, the same kind of integer as a
physical one, so a loc that skipped the rebind writes to the wrong slots without any error.
3/7 deletes the only probe that could tell them apart, because the new layout makes it
vacuous. This PR replaces it with an explicit mark, checked at every write door.
The contract
ForwardBatch.out_cache_loc_is_physicalrebind_write_locsets it on every pool, translating or not; the capture-time and draft-runner constructors that build a batch outsideinit_newset it themselvesKVWriteLoc.physicalFalseunless statedKVWriteLoc.for_batch(forward_batch, *, swa_loc, full_loc)out_cache_locwith the batch's mark. It wraps nothing else, because the mark describes that tensor onlyKVWriteLoc.for_layer(forward_batch, layer, …)encoder_out_cache_loc, which nothing translates and so is never markedKVCache.requires_physical_write_locValueErroron a loc not marked physical. The check is a flag read with no device sync, so it runs on every write-door call. Fused writers that scatter throughget_kv_buffer, and a replayed cuda graph, do not pass through a doorA loc produced separately states its own mark with
KVWriteLoc(loc, physical=…):trtllm_mla's capture-stable decode buffer (the runner translates into it), triton's
rank-local DCP slot of a physical loc, and the dspark / dflash draft writes (draft pools are
always static).
HybridLinearKVPooland the SWA composites forward theKVWriteLocto theirsub-pools instead of unwrapping it, so the mark reaches the door that checks it.
Completeness is enforced
test_physical_write_loc.pyparses every file that can hand a write loc to a pool under--enable-unified-memory— the attention backends on the unified allowlists (tied tokv_cache_hook's allowed sets, so a newly admitted backend joins the scan) plusmodels/,layers/cp/,speculative/,model_executor/anddisaggregation/— and requires the locargument of every write-door call to be
KVWriteLoc.for_batch/for_layeror aKVWriteLoc(...)with an explicitphysical=. It follows local names, parameters and helperreturns, and a self-test shows it flags a bare tensor and an unmarked
KVWriteLoc. Theadoption covers 44 write-door calls, including fa3's two MLA writes, triton's DCP writes and
the draft-model writes.
The pool side is enforced the same way. Producers now hand a
KVWriteLocto every pool,static ones included, so the same file also requires every
set_kv_buffer/set_mla_kv_bufferoverride undersrt/to unwrap its loc argument, forward it untouched toanother door, take only
*args, or only raise. It covers 30 doors.When a new backend must wrap
If the backend can receive a unified pool, its write doors pass
KVWriteLoc.for_batch(forward_batch, …), orfor_layerwhen it serves cross-attention. Ifit translates the ids itself, it passes
KVWriteLoc(loc, physical=True). The ratchet abovefails otherwise.
Static-pool paths the new loc type reaches
Wrapping the loc changes what static pools and backends receive, not just unified ones:
--dcp-size > 1comparespositions.numel()with the rank-local loc's length;it now reads the wrapped tensor (
loc.loc.numel()).test_dcp_layout_unit.pydrives theDCP write with a stub pool.
MLATokenToKVPoolFP4.set_mla_kv_buffer,HiSparseDSATokenToKVPool's two doors,LayerSplitDSATokenToKVPool.set_mla_kv_bufferandPageInterleaveMLATokenToKVPool.set_mla_kv_buffertakeloc_infoand unwrap it, liketheir siblings.
out_cache_loc_is_physical(
test_cp_strategy_unit.py,test_fused_fp8_kv_write.py).On the H200, before the stack was rebased onto current main, each against the base it then had
(0318a8d) (GSM8K 5-shot, n=200):
AttributeErrorat the first KV writefp4_mx_block16KV cache, flashinfer,--max-total-tokens 262144AttributeErrorin graph captureDeepSeek-V2-Lite is a base model: with a bf16 KV cache on the same setup it scores
0.325, so the FP4 rows are to be read against each other. The token cap keeps
flashinfer's FP4 decode, which dequantizes the whole pool, inside memory at both commits.
Validation
Model families exercised — a hybrid of every pool shape the unified pool supports, plus a
non-hybrid control. Perf covers the gated-deltanet hybrid (Qwen3.5-9B), the MLA hybrid
(Kimi-Linear-48B-A3B), the sliding-window hybrid (gpt-oss-20b), the tri-pool hybrid
(Inkling and Inkling-Small), and dense Llama-3.1-8B as a control. Accuracy covers the same
set and additionally the Mamba hybrid, Falcon-H1-7B. Each model is run unified against a
baseline on the same model, backend and configuration, so every delta isolates the pool.
Median inter-token latency, unified vs baseline: +1.03% (measured run-to-run noise floor
for this metric: 1.9%). Median GSM8K delta at n=200: +0.50 pt (binomial delta-sigma ≈ 3.0 pt).
Re-validated on the previous base (0318a8d) after review feedback: the stack tip (7/7) gives +0.95% median inter-token latency over 40 paired configurations (median per-configuration shift against the previous run of the same 40: +0.12 pp) and −0.25 pt median GSM8K over 26, with the full unit scope green.
Against the preceding PR's run on the same matrix the median per-model shift is −0.11 pp —
this PR moves nothing.
End to end with the always-on check
Each run below boots with
--enable-unified-memoryand writes through the checked doors;none refuses a write (GSM8K 5-shot, n=100). The unified-vs-baseline comparison for these
backends is in #38592.
aiter's write doors are marked, but aiter is not admitted under
--enable-unified-memory, sothe ratchet does not scan it, and its doors have not been run on ROCm.
CI States
Latest PR Test (Base): ⏳ Run #36944620308
Latest PR Test (Extra): ⏳ Run #36944620104
Latest PR Test (AMD ROCm 10): ⏳ Run #36944620296