Repository navigation
CUDA: size MMQ ids-path tail padding from the flattened row count, not ne11 - #27044
glennneuber wants to merge 1 commit into
Conversation
|
Hi @glennneuber, 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. |
The ids branch of ggml_cuda_mul_mat_q() sizes the src1_q8_1 data term from ne12*n_expert_used but the tail-padding term from ne11. The correct row count is computed just below as ne11_flat. For MoE gate/up the activations are broadcast, so ne11 == 1, and ggml_cuda_mmq_get_J_max() returns 0 for that. The buffer then has no tail padding while MMQ reads in tiles of up to 512 rows past the end.
e9dd2c5 to
5d958f7
Compare
|
This is a regression imho. I was able to pin it to a commit. It was introduced by 6eddde0 ("CUDA: refactor MMQ kernel configuration", #24127), build b9992. That commit changed the padding term in both allocation branches: - get_mmq_x_max_host(cc)*sizeof(block_q8_1_mmq);
+ ggml_cuda_mmq_get_J_max(src0->type, fallback, cc, ne11) * sizeof(block_q8_1_mmq);
- get_mmq_x_max_host(cc)*sizeof(block_q8_1_mmq);
+ ggml_cuda_mmq_get_J_max(src0->type, fallback, cc, ne11) * sizeof(block_q8_1_mmq);That is correct for the The difference matters because the two functions have different failure modes. Last clean build is b9990 (259ae1d), first affected is b9992. b9991 is not tagged and the only other commit in that range is Vulkan-only. Bisected by source, then checked at runtime on both sides of the boundary. Same GPU, same request, cold server, first request each time,
For anyone hitting this through ollama: v0.32.1 is the last release on a clean llama.cpp, v0.32.2 is the first affected. |
docs(tasks): the regression comment to post on ggml-org/llama.cpp#27044
…2.15 sync The upstream sync moved llama.cpp b10434 -> b10488, so payload_pin failed on the merged build: expected 7e4c0a968, actual 9d77fa172. That is the check working -- it refuses to let ladders measured on one payload imply a pass on another. Re-measured before changing the pin, not after. measure_ladder.py against 0.32.14-dynres-108-g76918a7 returned byte-identical rows for all three arches: nemotron_h_omni [266, 266, 578, 2306, 3270] budgets 256/3328 stride 32 gemma4 [1102 x 5] budgets 70/1120 stride 48 qwen35 [1034, 1034, 1034, 2306, 4082] budgets 1024/4096 stride 32 Same budgets, same pixel windows, same strides as the b10434 rows. So no expected value is edited here -- only the payload identity, plus the provenance explaining why. The prose in the expect blocks (ADR 0011 rule 4 discussion on qwen35, pinned_not_applicable, the B8 prefix notes) is left intact rather than overwritten with generator output that does not carry it. Not a new profile: the README's "add a new [profiles.<id>]" path is for a changed patchset or a new platform. The patchset (001/002/004/005/903) still describes this build exactly, and resolve_profile keys only on (platform, version) -- two cuda profiles sharing the 0.32.x-dynres pattern would resolve by file order, which is worse than useless. 903 is still required: ggml-org/llama.cpp#27044 re-checked 2026-08-21, still open, so b10488 carries the MMQ ids-path defect exactly as b10434 did. Verified: test_verdicts.py 43/43, and the full preflight (including the pinned-budget probe) on the merged build is now PASS=18 SKIP=2 FAIL=0, against FAIL=1 before this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I ran the ids path under compute-sanitizer on four architectures to see what this patch covers, since whether the over-read shows up depends on the allocator. Build at b96806d with a diagnostic pool (every P100 (sm_60) and T4 (sm_75) on Kaggle, A100 (sm_80) and L4 (sm_89) on Colab, CUDA 12.8:
Three things fall out.
The FAIL counts under |
|
Confirming this fix resolves a reproducible Setup: Windows, llama.cpp master Before this patch, It killed the server about 1,100 generated tokens into the second task of a benchmark run, having completed the first task and a 1,380-token generation before that, which matches the "depends on how much slack the pool happens to have" description in #27792. With this one-line change and no other difference (same build tree, same flags, same model), the identical benchmark runs to completion: ten tasks, 9.34/10, including the task that previously crashed the process. The default Worth noting for anyone hitting this: the same configuration is also a meaningful speedup, so the workaround of staying at |
|
Follow-up with a correction, and a better argument for this patch than the one I gave. Above I cited 140.4 tok/s at Separated by build, same model, same placement (
So this is not only a crash fix. It is worth about +47% prefill at That shape matches the mechanism: the mis-sized One caveat so this is not overclaimed. Page-cache warmth is not perfectly controlled between those two runs, because the expert weights are host resident and mmap'd. Each run was preceded by a multi-minute run over the same weights, and the |
|
Retracting the performance claim in my previous comment. This patch is performance-neutral on my setup; the +47% I reported was my own benchmarking error. The correctness finding stands and is unaffected. What went wrong: the run I quoted at 269.8 tok/s was the only run in my whole series that executed a multi-minute generation workload against the same server before the throughput probe. With ~34 GB of expert weights host-resident on a 63 GB box, that pre-run is what makes the difference, not the patch. I attributed it to the rebuild because the rebuild happened between the two runs. I have since measured the same configuration ( Unpatched cold 183.8 against patched cold ~187 is within run-to-run spread. The honest reading is no measurable throughput effect either way. Every arm above 195 tok/s in my data has the warm-up confound and no arm without it exceeded 194. The related claim in my first comment, that staying at Sorry for the noise. The reason I am still glad this landed: without it, |
|
Tested on Turing: v0.5.0 + this patch fixes a deterministic illegal memory access with MiMo-V2.6-Flash (host-resident experts, |
|
Independent confirmation from another Windows / Blackwell setup.
|
… (ne12*n_expert_used), not ne11 (ggml-org ggml-org#27044): illegal memory access with host-resident experts at -ub 2048 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ArcDEkPAw7vGSYJzi35Ng
|
Same bug as #29847, which comes with a test-backend-ops reproducer: On a GB10 (sm_121a, CUDA 13.0, master 11fe021) that case aborts with an illegal memory access without this change, and memcheck reports 485 errors. With the same change (ne12*n_expert_used for the padding) both cases pass and memcheck reports 0 errors. The full MUL_MAT_ID (933 cases, these two included) and MUL_MAT (1304) suites pass. Maybe add those two cases here. They fault without the sanitizer on this box and on the reporter's, so test-backend-ops would catch it on those setups. |
|
Sorry for the long radio silence, I took a break from llama.cpp and didn't see this in my backlog. The fix in this PR looks 90% correct to me, can you check whether #29941 also works? |
… (ne12*n_expert_used), not ne11 (ggml-org ggml-org#27044): illegal memory access with host-resident experts at -ub 2048
|
Yes, #29941 works here. On a GB10 (sm_121a, CUDA 13.0) with ne12 in the J_max call, both reproducer cases pass and memcheck reports 0 errors (without the change: abort, 485 errors). MUL_MAT_ID 933/933 and MUL_MAT 1304/1304 pass. Same result as the ne12*n_expert_used version. |
|
I'm closing this PR in favor of the other one, assuming the issue is now fixed. |
|
@JohannesGaessler, thanks for looking into this. Here are our test results: Table for #27044 (sm_120, RTX PRO 6000 Blackwell, CUDA 13.0, llama.cpp master 0504396; compute-sanitizer --tool memcheck, src1 buffer in its own exact-size allocation; memcheck errors, ✓ test passed, ✗ aborted):
Host-only check, 2,439,360 shapes on sm_75/86/89/120: #29941 short in 268,440 (all below 128 tokens), #27044 in none. |
|
I'm not seeing any for (bool b : {false, true}) {
test_cases.emplace_back(new test_mul_mat_id(GGML_TYPE_Q4_0, GGML_TYPE_F32, 512, 10, b, 640, 100, 2560));
} |
|
Reproduced on a GB10 (sm_121a) with src1_q8_1 in its own exact-size cudaMalloc, memcheck, master dd26678 (includes #29941). q4_0, 512 experts, 10 used, m=640, k=2560:
q4_K, 256 experts, 8 used, n=65 and n=100: 0 with both here, I did not hit the numbers in the table above for those shapes. Cause for n=100: mul_mat_q_switch_J picks J=112 (104 is skipped), while ggml_cuda_mmq_get_J_max(ne12=100) returns 96, so the padding is 16 columns short. J_max rounds down to a valid J, the picker rounds up. With ne_get_rows it is 128. Also 0 errors on all 8 cases with J_max(..., 128), a flat max tile width that does not depend on ne12. GGML_PAD(ne12, 8) is not enough, J_max stays at 96. |
|
Yes, looking at the code in |
|
Since I am unable to reproduce the issue locally, can either one of you please check #29953 ? |
Overview
The
idsbranch ofggml_cuda_mul_mat_q()sizes thesrc1_q8_1allocation from two different row counts. The data term usesne12*n_expert_used. The tail-padding term usesne11. The correct value is already computed a few lines below asne11_flat.For MoE gate/up projections the activations are broadcast across experts, so
ne11 == 1.ggml_cuda_mmq_get_J_max()withne11 == 1computesmin(1,512) = 1, then1 - 1%8 = 0, skips its loop and returns 0. The buffer gets no tail padding at all, while MMQ reads in tiles of up toJ_max = 512rows and runs past the end.ffn_downis affected too but less: it passesne11 = 8, so padding is sized for 8 rows instead ofne12*n_expert_used.The
!idsbranch above is correct, because therene11really is the buffer's row count.This patch uses
ne12*n_expert_usedfor the padding term.Additional information
Hit this on a MoE vision model (256 experts, 8 used, q4_K gate/up) on sm_120 / CUDA 13.0 when a whole image was submitted as one 2040-token ubatch:
The failing node is
ffn_moe_gate, withne2=2040 ne02=256 ne11=1 ne12=2040, type q4_K, taking the mmq branch. Thefind_slotline also shows up on runs that do not crash, so it marks the path rather than the fault.Notes for testing:
test-backend-opsdoes not catch it. The over-read lands in padding rows the kernel discards, so output is unchanged and NMSE is unaffected.compute-sanitizer --tool memcheckdoes flag it.512 * sizeof(block_q8_1_mmq)= 72 KB per call, and does not grow withne12.Tested with 4 cold runs on the patched build (no fault) against 2 unpatched controls from the same tree (both fault).
May be related to #24399, #19705 and #18331. I have not reproduced those configurations, so this is a guess, but the
GGML_CUDA_FORCE_CUBLAS=ONworkaround in #24399 skips MMQ entirely and requantising changes the allocation size, which would both hide this.Requirements