Repository navigation
kv-cache : initialize per-stream views after buffer allocation (#23737) - #27738
ByungHyun21 wants to merge 1 commit into
Conversation
The k_stream/v_stream views are created before the k/v tensors are allocated, so on allocation paths that skip ggml_backend_view_init they keep data == NULL while their storage lives on view_src. Direct IO (draft-mtp checkpoint serialization, stream-to-stream copies) then aborts in ggml_backend_tensor_get/set on backends that require an initialized tensor — reproducible on Vulkan (GGML_ASSERT(tensor->data != NULL), #23737) since 6c4cbdc. Initialize any still-uninitialized stream view after the KV buffers are allocated. Co-authored-by: Yoshi4470 <Yoshi4470@users.noreply.github.com>
|
Hi @ByungHyun21, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
Closing in favor of #25584. After re-checking the cross-references on #23737 I found @Yoshi4470's open PR, which fixes this at the allocator level — a view-only tail skipping the final Thanks @Yoshi4470 for the root-cause work, and apologies for the duplicate noise. |
Root cause and fix for a separate --parallel>1 draft-mtp SIGSEGV/abort crash: GGML_ASSERT(tensor->data != NULL && "tensor not allocated") aborting the whole process on periodic checkpoint creation (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> llama_state_seq_get_data_ext -> ggml_backend_tensor_get), reproduced live on Qwen3.8-27B-UD-Q4_K_XL (Vulkan/gfx1151) with --parallel 3 and draft-mtp speculative decoding, via direct kubectl port-forward + log capture. A live diagnostic build confirmed the crashing tensor is a per-stream KV-cache view (layer 64 MTP k_stream) with data=(nil) buffer=(nil) view_src=<valid, allocated> -- i.e. the VIEW itself was never initialized even though its backing storage was. Note: this is a distinct bug from the empty-draft-sequence crash fixed by the preceding commit -- that one is a genuinely never-touched sequence being read at all; this one is a sequence whose backing view was never initialized even though it had been used and has real data. This is a confirmed, independently-reported-upstream bug, not something specific to this fleet's config: - Issue ggml-org/llama.cpp#23737 ("Eval bug: GGML_ASSERT(tensor->data != NULL) on Vulkan since b9318") -- exact assert match, multiple independent reporters on different hardware (RADEON AI PRO r9700) and different models (gemma-4-31B-it, gemma-4-26B-A4B), confirming this is a general Vulkan-backend multi-stream/view-allocation bug, not MTP- or model-specific. - PR ggml-org/llama.cpp#27738 (closed, not merged, by its own author in favor of the PR below) describes the exact same reproduction: model Qwen3.8-27B-UD-Q4_K_XL, MTP/nextn layer 64, same call stack (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> pre_decode). Root cause per that PR's own body: "The k_stream/v_stream views are created before the k/v tensors are allocated. On allocation paths that skip ggml_backend_view_init ... they keep tensor->data == NULL forever while their storage lives on view_src." - PR ggml-org/llama.cpp#25584 ("ggml: fix view init skipped after buft max_size split", open/unmerged as of vendoring) is the fix applied here -- named by #27738's own closing comment as "the better fix," fixing it at the ggml allocator level rather than the KV-cache call site, predating #27738 by six weeks, and additionally covering two related issues (#19839, #21762). ROOT CAUSE: when ggml_backend_alloc_ctx_tensors_from_buft splits a context's allocation across multiple buffers because a tensor would exceed the backend buft max_size (Vulkan's default is 1 GiB -- exactly the class of split a ~27B model's KV-cache tensors can trigger), a view-only tail at the end of a split range can skip alloc_tensor_range entirely, so any persistent view landing in that tail (this fleet's layer-64 MTP k_stream/v_stream) never gets ggml_backend_view_init and keeps data == NULL indefinitely, even though its view_src is fully allocated. FIX: instead of trying to allocate-or-view-init each tensor inline during a single pass over one split range (the old alloc_tensor_range logic, exactly what a view-only tail could skip), allocate ONLY real (non-view) tensors during the per-range pass, then run one final pass over the WHOLE context after all splits complete, initializing any tensor whose view_src is set but whose buffer is still NULL. This guarantees every view gets initialized regardless of which split range its parent tensor's allocation landed in. TEST FILE NOT PORTED: this PR's own tests/test-alloc.cpp addition (test_view_init_after_max_size_split) does not apply cleanly against this commit (context drift from the PR's own last rebase point, same class of drift as patch 0001's grammar fix) and was not hand-re-ported here. Low priority to re-port since the fleet's own build disables LLAMA_BUILD_TESTS anyway; worth doing if this repo's tests are ever turned back on. VERIFIED: applies clean (git apply --check) against this exact pinned commit with zero context drift, and gcc -fsyntax-only clean on the modified file. Live-verified end-to-end on real fleet hardware: the exact original crash repro (>16K-token prompt, --parallel 3, draft-mtp, kv_unified=false, mid-prefill checkpoint) completed cleanly (HTTP 200, 0 pod restarts) against an image built from this commit, where the prior image reliably aborted on an identical repro. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017R3QNP4GznFWhhf11QY18j
Root cause and fix for a separate --parallel>1 draft-mtp SIGSEGV/abort crash: GGML_ASSERT(tensor->data != NULL && "tensor not allocated") aborting the whole process on periodic checkpoint creation (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> llama_state_seq_get_data_ext -> ggml_backend_tensor_get), reproduced live on Qwen3.8-27B-UD-Q4_K_XL (Vulkan/gfx1151) with --parallel 3 and draft-mtp speculative decoding, via direct kubectl port-forward + log capture. A live diagnostic build confirmed the crashing tensor is a per-stream KV-cache view (layer 64 MTP k_stream) with data=(nil) buffer=(nil) view_src=<valid, allocated> -- i.e. the VIEW itself was never initialized even though its backing storage was. Note: this is a distinct bug from the empty-draft-sequence crash fixed by the preceding commit -- that one is a genuinely never-touched sequence being read at all; this one is a sequence whose backing view was never initialized even though it had been used and has real data. This is a confirmed, independently-reported-upstream bug, not something specific to this fleet's config: - Issue ggml-org/llama.cpp#23737 ("Eval bug: GGML_ASSERT(tensor->data != NULL) on Vulkan since b9318") -- exact assert match, multiple independent reporters on different hardware (RADEON AI PRO r9700) and different models (gemma-4-31B-it, gemma-4-26B-A4B), confirming this is a general Vulkan-backend multi-stream/view-allocation bug, not MTP- or model-specific. - PR ggml-org/llama.cpp#27738 (closed, not merged, by its own author in favor of the PR below) describes the exact same reproduction: model Qwen3.8-27B-UD-Q4_K_XL, MTP/nextn layer 64, same call stack (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> pre_decode). Root cause per that PR's own body: "The k_stream/v_stream views are created before the k/v tensors are allocated. On allocation paths that skip ggml_backend_view_init ... they keep tensor->data == NULL forever while their storage lives on view_src." - PR ggml-org/llama.cpp#25584 ("ggml: fix view init skipped after buft max_size split", open/unmerged as of vendoring) is the fix applied here -- named by #27738's own closing comment as "the better fix," fixing it at the ggml allocator level rather than the KV-cache call site, predating #27738 by six weeks, and additionally covering two related issues (#19839, #21762). ROOT CAUSE: when ggml_backend_alloc_ctx_tensors_from_buft splits a context's allocation across multiple buffers because a tensor would exceed the backend buft max_size (Vulkan's default is 1 GiB -- exactly the class of split a ~27B model's KV-cache tensors can trigger), a view-only tail at the end of a split range can skip alloc_tensor_range entirely, so any persistent view landing in that tail (this fleet's layer-64 MTP k_stream/v_stream) never gets ggml_backend_view_init and keeps data == NULL indefinitely, even though its view_src is fully allocated. FIX: instead of trying to allocate-or-view-init each tensor inline during a single pass over one split range (the old alloc_tensor_range logic, exactly what a view-only tail could skip), allocate ONLY real (non-view) tensors during the per-range pass, then run one final pass over the WHOLE context after all splits complete, initializing any tensor whose view_src is set but whose buffer is still NULL. This guarantees every view gets initialized regardless of which split range its parent tensor's allocation landed in. TEST FILE NOT PORTED: this PR's own tests/test-alloc.cpp addition (test_view_init_after_max_size_split) does not apply cleanly against this commit (context drift from the PR's own last rebase point, same class of drift as patch 0001's grammar fix) and was not hand-re-ported here. Low priority to re-port since the fleet's own build disables LLAMA_BUILD_TESTS anyway; worth doing if this repo's tests are ever turned back on. VERIFIED: applies clean (git apply --check) against this exact pinned commit with zero context drift, and gcc -fsyntax-only clean on the modified file. Live-verified end-to-end on real fleet hardware: the exact original crash repro (>16K-token prompt, --parallel 3, draft-mtp, kv_unified=false, mid-prefill checkpoint) completed cleanly (HTTP 200, 0 pod restarts) against an image built from this commit, where the prior image reliably aborted on an identical repro. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017R3QNP4GznFWhhf11QY18j
Root cause and fix for a separate --parallel>1 draft-mtp SIGSEGV/abort crash: GGML_ASSERT(tensor->data != NULL && "tensor not allocated") aborting the whole process on periodic checkpoint creation (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> llama_state_seq_get_data_ext -> ggml_backend_tensor_get), reproduced live on Qwen3.8-27B-UD-Q4_K_XL (Vulkan/gfx1151) with --parallel 3 and draft-mtp speculative decoding, via direct kubectl port-forward + log capture. A live diagnostic build confirmed the crashing tensor is a per-stream KV-cache view (layer 64 MTP k_stream) with data=(nil) buffer=(nil) view_src=<valid, allocated> -- i.e. the VIEW itself was never initialized even though its backing storage was. Note: this is a distinct bug from the empty-draft-sequence crash fixed by the preceding commit -- that one is a genuinely never-touched sequence being read at all; this one is a sequence whose backing view was never initialized even though it had been used and has real data. This is a confirmed, independently-reported-upstream bug, not something specific to this fleet's config: - Issue ggml-org/llama.cpp#23737 ("Eval bug: GGML_ASSERT(tensor->data != NULL) on Vulkan since b9318") -- exact assert match, multiple independent reporters on different hardware (RADEON AI PRO r9700) and different models (gemma-4-31B-it, gemma-4-26B-A4B), confirming this is a general Vulkan-backend multi-stream/view-allocation bug, not MTP- or model-specific. - PR ggml-org/llama.cpp#27738 (closed, not merged, by its own author in favor of the PR below) describes the exact same reproduction: model Qwen3.8-27B-UD-Q4_K_XL, MTP/nextn layer 64, same call stack (common_prompt_checkpoint::update_dft -> server_context_impl::create_checkpoint -> pre_decode). Root cause per that PR's own body: "The k_stream/v_stream views are created before the k/v tensors are allocated. On allocation paths that skip ggml_backend_view_init ... they keep tensor->data == NULL forever while their storage lives on view_src." - PR ggml-org/llama.cpp#25584 ("ggml: fix view init skipped after buft max_size split", open/unmerged as of vendoring) is the fix applied here -- named by #27738's own closing comment as "the better fix," fixing it at the ggml allocator level rather than the KV-cache call site, predating #27738 by six weeks, and additionally covering two related issues (#19839, #21762). ROOT CAUSE: when ggml_backend_alloc_ctx_tensors_from_buft splits a context's allocation across multiple buffers because a tensor would exceed the backend buft max_size (Vulkan's default is 1 GiB -- exactly the class of split a ~27B model's KV-cache tensors can trigger), a view-only tail at the end of a split range can skip alloc_tensor_range entirely, so any persistent view landing in that tail (this fleet's layer-64 MTP k_stream/v_stream) never gets ggml_backend_view_init and keeps data == NULL indefinitely, even though its view_src is fully allocated. FIX: instead of trying to allocate-or-view-init each tensor inline during a single pass over one split range (the old alloc_tensor_range logic, exactly what a view-only tail could skip), allocate ONLY real (non-view) tensors during the per-range pass, then run one final pass over the WHOLE context after all splits complete, initializing any tensor whose view_src is set but whose buffer is still NULL. This guarantees every view gets initialized regardless of which split range its parent tensor's allocation landed in. TEST FILE NOT PORTED: this PR's own tests/test-alloc.cpp addition (test_view_init_after_max_size_split) does not apply cleanly against this commit (context drift from the PR's own last rebase point, same class of drift as patch 0001's grammar fix) and was not hand-re-ported here. Low priority to re-port since the fleet's own build disables LLAMA_BUILD_TESTS anyway; worth doing if this repo's tests are ever turned back on. VERIFIED: applies clean (git apply --check) against this exact pinned commit with zero context drift, and gcc -fsyntax-only clean on the modified file. Live-verified end-to-end on real fleet hardware: the exact original crash repro (>16K-token prompt, --parallel 3, draft-mtp, kv_unified=false, mid-prefill checkpoint) completed cleanly (HTTP 200, 0 pod restarts) against an image built from this commit, where the prior image reliably aborted on an identical repro. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017R3QNP4GznFWhhf11QY18j
Overview
llama-server/llama-cliwith--spec-type draft-mtpaborts on the Vulkan backend (#23737, open since May 26):backtrace:
common_prompt_checkpoint::update_dft → server_context_impl::create_checkpoint → pre_decode. First bad commit: 6c4cbdc (#23646).The k_stream/v_stream views are created before the k/v tensors are allocated. On allocation paths that skip
ggml_backend_view_init(observed: the draft-MTP KV on Vulkan — debugged down tocache_k_l64 (view)/cache_v_l64 (view), layer 64 being the nextn/draft layer), they keeptensor->data == NULLforever while their storage lives onview_src. Direct IO — the draft-mtp checkpoint serialization and stream-to-stream copies (llama-kv-cache.cpp:846) — then feeds those views toggml_backend_tensor_get/set, which require an initialized tensor on every backend that addresses viatensor->data(CPU memcpy; CUDA/HIP analogous).Fix: after the KV buffers are allocated, initialize any still-uninitialized stream view via
ggml_backend_view_init. 20 lines, guarded to!hparams.no_alloc, and only touches views that are actually uninitialized (data == NULL, backing allocated).The root-cause analysis originates with @Yoshi4470 (issue thread, June 6, fork commit 09c1af8). This PR fixes the same bug at the source rather than relaxing the ggml-backend asserts: we verified that assert relaxation alone leaves the CPU backend reading through a NULL
tensor->data(segfault) — only backends whose get/set address viaview_src(Vulkan) survive it.Reproduction / verification (gfx1151 / RADV 25.2.8, ROCm 7.2.2 for the HIP control, Qwen3.8-27B-UD-Q4_K_XL + mmproj)
Before (stock fc35562): abort in 14 s at first inference.
After (this branch @ 67e1862, rebuilt from the pushed commit):
-np 4 -c 1048576 --spec-type draft-mtp/slotssave/restore (same IO family)A unit test would need a full
llama_kv_cacheconstruction (model + device context); the CLI command above is the reproducible regression check, matching the issue's reporter environment.Additional information
The issue has four independent reporters (Strix Halo / RADV, Radeon AI PRO r9700); the most recent crash report in the thread is from July 20. cc @cdanis @artisticMink @Andy4OS — verification on your machines welcome.
Performance (cross-backend data taken while verifying this fix)
Measured on a tree combining #27311 (the scheduler input-ring fix — required for valid HIP numbers under
--parallel, see #27572) and this fix; both backends built from the same source. Qwen3.8-27B-UD-Q4_K_XL,-fa on -ctk/-ctv q4_0 -b 1024 -ub 512, per-slot ctx 262144, realistic corpus, temp 0, 3-run medians; server-process CPU from /proc deltas.Server tg t/s (draft-mtp n4 / plain), with draft acceptance medians:
VK MTP tg tracks its acceptance, not length: the 7404 dip is a less draft-predictable corpus chunk (acc 0.44 vs ~0.50 elsewhere), while VK plain decode is position-flat — HIP MTP declines on both counts (position + acceptance).
Server pp t/s: HIP 266–315, VK 127–233 across the same sizes.
Position scaling (llama-bench tg32, depth fill, r1, no-warmup):
Vulkan decode is position-invariant (~9.2–10.3 flat) while HIP degrades linearly (-57% over 98K); crossover ~20–30K tokens. This is independent of this fix but is the reason it matters: long-context draft-mtp on Vulkan becomes reachable for the first time on this hardware class, and it is the faster backend there.
Server-process CPU: VK 0.6–1.0% avg (p95 2–3%) vs HIP 3.8–5.1% (p95 12–18%).
Vision (mmproj) image encode, 640×480: HIP 1.64 s, VK 3.60 s.
No measurable delta from this fix itself: the change only initializes views that were previously uninitialized (and aborting) — it adds no work to any steady-state path.
Requirements