[BugFix][DSA] Harden top-k v1/v2 kernels against negative padded seq_lens - #31123
[BugFix][DSA] Harden top-k v1/v2 kernels against negative padded seq_lens#31123zkyue wants to merge 1 commit into
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
seq_len < 0 is sick. In this case I would recommend fix the upstream (NOTE that, not only topk kernel consumers seq_len, other kernels may suffer from the same bug). Another workaround with 0 overhead is to change the trivial path into signed integer comparison (fill all indices with -1, since non-negative number >= -1) |
Negative per-row lengths (DP-padded / idle-companion rows, e.g. -4 from GLM 5.2 MTP draft-extend metadata) are reinterpreted as ~4e9-token rows by the unsigned device-side reads in both top-k kernels. Depending on how the unsigned chunk arithmetic wraps, this is either an illegal memory access (v1: any negative; v2: e.g. -1048576 on every dispatch shape) or a silently garbage output row (v2: -1/-4), all reproduced deterministically on B200. sgl-project#30378 clamped two DSA triton producers, but the kernels stayed exposed to every other caller (the DSv4 DP-attention path has no clamp). Per review feedback, handle this with signed comparisons instead of an arithmetic clamp: negative rows take the documented trivial all-(-1) path (a non-negative candidate index never qualifies against a negative length). v1 keeps seq_len signed through the trivial dispatch and naive_transform; v2 dispatches via a signed is_trivial() in both the main and fused small-batch kernels; the plan kernel counts signed and proves rows positive before the (unsigned) pool-routing compare, so a full-uint32 static_cluster_threshold keeps its meaning. Zero cost: paired timing on all six dispatch shapes is unchanged within noise.
05d65e5 to
f6345c9
Compare
|
Thanks — agreed, and adopted in the latest push. Signed comparison adopted: no arithmetic clamp remains anywhere; negative rows are routed by comparisons alone:
Re-validated the full battery on B200: 262/262 registered tests (the 18 negative-length regressions stay green, incl. the plan-metadata pin), the illegal-address/garbage repros, CUDA-graph replay, compute-sanitizer memcheck + synccheck (0 errors), and paired timing on all six dispatch shapes (unchanged within noise). On fixing upstream: fully agreed the producer is the right long-term fix — a padded row should never carry a negative length in the first place. #30378 clamped two GLM DSA triton producers ( On other consumers: your warning matches what we saw while auditing. The same unsigned-length pattern exists elsewhere in the DSv4 JIT family, e.g. |
|
Closing this one out. Since it was opened, the concrete hazards in this kernel have been addressed directly upstream — #30645 (tie overflow / inf scores → invalid indices) and #34167 (CUDA 13.1+ non-primary-rank output drop) — and #24654's Closing to keep the queue clean; happy to revive with a concrete reproducer if one shows up. |
Motivation
Related to #25574 (
topk_transform_512_v2illegal memory access on SM100 under DP-attention). We set out to reproduce and fix that issue on B200 (SM100) and found the following:The code quoted in DSv4 topk_v2 fused-cluster kernel crashes on B300/SM100 with DP-attn + DeepEP #25574 no longer exists. [JIT Kernel] DeepSeek-V4 DSA indexer: faster top-k + page-table transform (runtime k <= 2048) #26788 rewrote
topk_v2.cuh; the oldtopk_fused_transformSMALL/TRIVIAL early-return branches are gone, and the rewritten fused small-batch kernel already has cluster-uniform fall-through control flow. Note the branch condition is per-blockIdx.xand all 8 blocks of a cluster shareblockIdx.x, so even in the old kernel every block of a cluster took the same branch — there was never a divergent cluster barrier (compute-sanitizer --tool synccheckon the pre-[JIT Kernel] DeepSeek-V4 DSA indexer: faster top-k + page-table transform (runtime k <= 2048) #26788 kernel reports 0 errors, and 24k+ stress iterations over mixed TRIVIAL/SMALL/LARGE batches, eager + CUDA-graph replay, never faulted on B200). The B300-specific crash could not be reproduced at kernel level on B200, and its proposed patch (fix(dsv4 topk_v2): honor cluster contract in fused kernel SMALL/TRIVIAL branches #25575) targets the deleted code.What does reproduce — deterministically, at current
main— is the negative-seq_lenscrash class. Negative per-row lengths (DP-padded / idle-companion rows; [DSA] Re-enable fused top-k v2 for MTP: clamp padded-row seq_lens to >= 0 #30378 observed-4from GLM 5.2 MTP draft-extend metadata, and DP-attention idle rows are the same class) are read through unsigned conversions in both top-k kernels and become ~4e9-token rows:topk_v1.cuh): illegal memory access for any negative length (notably, theSGLANG_OPT_USE_TOPK_V2=0fallback is equally exposed);topk_v2.cuh): illegal memory access (e.g.-1048576, on every dispatch shape: fused small-batch cluster, persistent pool, streaming) or — worse — a silently garbage top-k row (-1/-4, where the unsigned chunk arithmetic happens to wrap to a zero-length chunk andhandle_tiepads the row with bogus indices).[DSA] Re-enable fused top-k v2 for MTP: clamp padded-row seq_lens to >= 0 #30378 clamped two DSA triton producers caller-side, but the kernels stay exposed to every other caller — e.g. the DSv4 DP-attention path (
dsv4/metadata.pyfeedsc4_seq_lenstoplan_topk_v2unclamped).Modifications
Per review feedback, negative lengths are handled with signed comparisons (no arithmetic clamp) so a negative row takes the documented trivial all-
(-1)"no tokens" path -- every non-negative candidate index compares>= -1, so none qualifies:topk_v1.cuh:seq_lenstaysint32_t; the signed trivial dispatch (seq_len <= (int32_t)kTopK) routes negative rows intonaive_transform, whose fill comparison is signed; the radix path casts touint32_tonly after the> kTopKguard proves the value positive;topk_v2.cuh: a signedis_trivial()((int32_t)seq_len <= (int32_t)topk) used by the main and fused small-batch kernels;trivial_transform's per-element comparison is signed;topk_v2.cuhtopk_plan(parameter type fixed toconst int32_t*to match the tensor dtype): the threshold-count loop compares signed against the candidate thresholds (all int32-exact); the pool-compaction predicate proves a row positive (signed) before the unsigned threshold compare, so negative rows are never routed to the persistent pool as ~4e9-token items while a full-uint32static_cluster_thresholdkeeps its meaning.Python docstrings updated (producers should still clamp to 0 themselves; the kernel no longer turns a producer bug into an illegal address or silent corruption). Cost is unchanged -- paired timing below.
New tests:
test_topk_v2_negative_lengths(8 configs x k in {512, 2048}, one config per dispatch shape: level 0/1/2, fused small-batch cluster with-1/-4/-1048576, persistent pool),test_topk_v2_plan_negative_lengths(pins the plan kernel's counting loop via a candidate-cap-boundary distribution and the plan metadata, plus a full-uint32 static-threshold boundary check), andtest_topk_v1_negative_lengths. Output buffers are poison-prefilled; each test asserts negative rows come back all-(-1)(bothoutandraw) and non-negative rows in the same batch still matchtorch.topk.Accuracy Tests
On B200 (CUDA 13.3), before this patch (current
main):[40000]*16 + [-1048576, 400](streaming),[131072, 9000, 9000, -1048576](fused cluster),[131072]*20 + [400]*10 + [-1048576](persistent pool):CUDA error: an illegal memory access was encountered— deterministic;[131072, 9000, 9000, -4]/[..., -1]: padded row returns 512 garbage indices (silent corruption);[9000, 9000, 9000, -4]: illegal memory access;[131072, 9000, 9000, -4]reproduces the exact reported failure signature (illegal address surfacing at the fused-kernel launch).After this patch:
(-1);test/registered/jit/deepseek_v4/test_topk_v2.py: 262 passed (244 pre-existing + 18 new);compute-sanitizermemcheck + synccheck on negative-row and mixed-branch cluster scenarios: 0 errors.Speed Tests and Profiling
Paired A/B timing (median of 5x50 launches, 4 interleaved rounds, B200),
mainvs this PR, us/launch:All deltas within run-to-run noise on a shared machine.
Checklist
CI States
Latest PR Test (Base): ❌ Run #29328239541
Latest PR Test (Extra): ❌ Run #29328239210