Skip to content

ggml : fix race-condition in ggml-rpc #13600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gkpln3
Copy link

@gkpln3 gkpln3 commented May 17, 2025

No description provided.

@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label May 17, 2025
@gkpln3 gkpln3 changed the title ggml : Fixed race-condition that leads to crashes when using ggml-rpc ggml : fix race-condition that leads to crashes when using ggml-rpc May 17, 2025
@gkpln3 gkpln3 changed the title ggml : fix race-condition that leads to crashes when using ggml-rpc ggml : fix race-condition in ggml-rpc May 17, 2025
@rgerganov
Copy link
Collaborator

The RPC backend is always called in a single thread. What kind of crashes do you observe?

@gkpln3
Copy link
Author

gkpln3 commented May 17, 2025

I'm adding RPC support to Ollama, which depends on this feature. In Ollama, it's called from multiple Go threads, leading to race conditions and crashes. I assumed ggml-rpc.cpp was intended to be thread-safe, given the use of mutexes elsewhere in the file.

@rgerganov
Copy link
Collaborator

While we use std::mutex in few places, the RPC backend is not considered thread-safe. I think that ggml backends in general are not required to be thread-safe by design, @slaren please correct me if I am wrong.

I'd suggest to add proper synchronization in the Go code.

@slaren
Copy link
Member

slaren commented May 17, 2025

Most ggml-backend objects are not intended to be thread safe, however it should still be possible to use different objects in different threads simultaneously. The RPC client reuses the same socket for multiple objects, so the synchronization here seems necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants