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
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. |
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