ggml: fix backend split scheduler race condition - #26040
Conversation
splits without input were running concurrently with other splits, while potentially reusing memory the other split is accessing
github-actions
Bot
added
the
ggml
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
As of right now ggml backend events are used in unsafe ways. They are being typecast unconditionally so I think this patch will result in segfaults if multiple different backends (e.g. CUDA + Vulkan) are used together.
|
I can't trigger any crash with this on my system with CUDA + Vulkan, but I don't know if there are edge cases. Are there plans to fix the way events are used? We could also just use synchronize here until then. |
|
@aendk for backend scheduling related matters |
|
There is a PR open to do alignment on a backend's expected beahvior: #25319 Unfortunately I have not had the time to follow-up on this |
|
For reference, I was able to reproduce this extremly rarely on underclocked NVIDIA GPUs. To fix this, several options spring to mind:
As a hotfix, I think 1 is ok; I have not tested performance though. |
|
I would like to get this fix merged in some way. @JohannesGaessler should it avoid using events entirely for now, or is it okay like this? |
I don't see a problematic use case atm that would lead an event from one backend ending up handled by another backend. We can add asserts in the backends. Though this change should be good as is. |
ggerganov
added
the
merge ready
|
Shouldn't this one also be superseeded by #27311? Adding a ring-buffer will allow overlap/concurrency 🤔 |
|
Currently in the final stages of testing. The question if this overlaps with #27311 is IMO moot. This is a good hotfix, and our medium-term plans move away from explicit syncs anyways (#25319) as outlined above in #26040 (comment) |
|
Ok, the change looks good to me. |
|
No performance regression for ggml-hexagon, verified on Snapdragon 8 Gen 4 hardware. Qualcomm's ggml-hexagon: JZ's ggml-hexagon: |
* ggml: fix backend split scheduler race condition splits without input were running concurrently with other splits, while potentially reusing memory the other split is accessing * only sync when split has no inputs
The spine sat idle for the whole worker round trip on all 48 layers. Three things had to change together for the shared-expert FFN to actually run during the RPC: - ggml-backend scheduler: a split with no inputs used to force a full sync against the previous backend before starting, because gallocr may reuse buffer regions across splits (ggml-org#26040). The expert-dispatch WAIT is MAP_CUSTOM2 on CPU with only CPU sources, a different buffer type from the GPU shexp FFN, so that reuse cannot happen. Defer the sync for that one shape and drain the backend before the next split that really uses it, which HIP graph capture still needs. - llm_graph_context::shexp_after_issue(): pin the shared expert's INPUT to the issue node instead of its output. Pinning the output made the GPU FFN a split that completed before the wait, so the deferred sync only overlapped a one-element acc with recv. The input is scaled by 1 first so acc_inplace cannot alias the activations that issue reads. - complete_moe_dispatch(): build_wait() no longer takes shexp, and shexp is added first so graph expansion visits the GPU FFN split ahead of the CPU wait. Also in this commit: - A sidecar MTP/DFlash GGUF ships its own routed experts in-file but inherits the target's --expert-dispatch string and ctx_other, so it dispatched trunk layers to workers that do not hold them. Dispatch is now ignored unless the model itself sets weight_pager.routed_experts_external. - wp-expert-worker: per-class arena placement fixes, and the worker library is now linked into llama-server. - llama-context: LLM_ARCH_QWEN4EXP added to the large-graph node budget (rides along here; belongs with the upstream sync in the next commit). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjfM4FxGPEhoApv6yvWAzD
* ggml: fix backend split scheduler race condition splits without input were running concurrently with other splits, while potentially reusing memory the other split is accessing * only sync when split has no inputs
5 participants
Overview
Fixes #23321
Splits without input were running concurrently with other splits, while potentially reusing memory the other split is accessing. In this case, for Qwen models with
-nkvo, one Vulkan split containsmodel.input_embedas input, while a following CPU split without inputs reuses the same memory. The Vulkan backend runs fully asynchronously, it just schedules the copies and the graph execution and returns, so the CPU backend was able to run immediately and overwrite the memory area used for themodel.input_embedtensor, before the Vulkan backend actually read it.The solution I chose here is to make sure all splits run sequentially, since currently the allocator assumes it can reuse memory in following splits. Potentially faster may be if the allocator took concurrency into account and didn't reuse memory for these cases, but that would be much more complicated.
I'm not that familiar with the ggml-backend.cpp code, so let me know if this is not the right way to handle it.
This did not affect CUDA because CUDA cpy_tensor_async does not run for CPU->GPU copies, it falls back to a synchronous copy.
Requirements