rpc: cherry-pick upstream #18626 and #26500 (real async + event APIs, RPC_PROTO 6.0) - #181
rpc: cherry-pick upstream #18626 and #26500 (real async + event APIs, RPC_PROTO 6.0)#181danielhanchen wants to merge 2 commits into
Conversation
* rpc : implement event and async backend APIs * cache responses from RPC_CMD_GET_ALLOC_SIZE
* rpc: avoid serializing buffers from other servers Only include remote buffer pointers when the buffer belongs to the RPC dispatcher receiving the graph. Add a two-server regression test for cross-server tensor serialization. Assisted-by: Codex * cont : add ref --------- Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec2378870
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bool status = send_rpc_cmd(sock, msg_ptr->cmd, msg_ptr->input.get(), msg_ptr->input_size); | ||
| RPC_STATUS_ASSERT(status); | ||
| } | ||
| } | ||
| msg_ptr->completion.set_value(); |
There was a problem hiding this comment.
For output-less commands such as RPC_CMD_GRAPH_COMPUTE, send_rpc_cmd returns immediately after flushing the request, and this fulfills the promise without receiving any server acknowledgment. Therefore ggml_backend_rpc_synchronize and recorded events can report completion while the server is still computing, allowing the scheduler to reuse buffers or consume results on another backend prematurely; the barrier needs a round trip that completes only after the server finishes all preceding commands.
Useful? React with 👍 / 👎.
| // Cache key for calls to read the alloc_size. | ||
| // We deliberately exclude src tensor dimensions from the key because: | ||
| // 1. For CPU backends, alloc_size = ggml_nbytes(output) regardless of src shapes | ||
| // 2. For GPU backends, the reservation graph uses max dimensions, so the | ||
| // cached value from reservation is always >= any subsequent request | ||
| // 3. Including src dims causes cache misses per-ubatch (e.g. growing KV cache) | ||
| // which blocks the main thread behind in-flight GRAPH_COMPUTE commands |
There was a problem hiding this comment.
When a FLASH_ATTN_EXT output with unchanged dimensions and op parameters is reused while its K/V source shapes grow, this cache returns the size calculated for the earlier sources. CUDA's ggml_cuda_flash_attn_ext_get_alloc_size explicitly adds conversion storage based on ggml_nelements(K) and ggml_nelements(V) for non-F16 sources, so a smaller first lookup can make the RPC allocator under-allocate a later decode graph and permit the conversion kernel to write past the allocation.
Useful? React with 👍 / 👎.
| void rpc_dispatcher::event_free(ggml_backend_event_t event) { | ||
| rpc_event * ev = (rpc_event *)event->context; | ||
| delete ev; | ||
| } |
There was a problem hiding this comment.
Every event is created with both a heap-allocated rpc_event context and a heap-allocated ggml_backend_event, but this free path deletes only the context. Consequently each event owned by a parallel scheduler leaks its wrapper when ggml_backend_event_free is called, accumulating across repeated scheduler or context creation; delete event after releasing its context as the other backend implementations do.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Closing this is correct, and here is the evidence so nobody reopens it by mistake. The prebuilt release Verified on the shipped linux-arm64 cuda13 bundle on a pair of DGX Sparks:
One thing worth knowing when reading this repository: |
3 participants
rpc: cherry-pick upstream ggml-org#18626 (real async + event APIs, RPC_PROTO 6.0)
Cherry-picks upstream
d0132a68-- ggml-org/llama.cpp#18626 "rpc : implement event andasync backend APIs", merged 2026-08-26 --
onto
master. It applies cleanly with no conflicts: 2 files, +442/-165, no fork code touched.Why
Our fork is on
RPC_PROTO 5.1. Upstream is on6.0. The difference is not cosmetic: ggml-org#18626gives the RPC backend a real
rpc_dispatcherper endpoint with a background I/O thread,heap-allocated commands passed as
shared_ptr, and genuine event semantics, then advertisescaps.asyncandcaps.eventsunconditionally.That is what lets
ggml_backend_schedactually overlap work across RPC devices. Without it,llama-server --device CUDA0,RPC0 -sm layersplits a model across two machines but runs thetwo halves strictly one after the other, so a two-node split costs slightly more than it
saves. With it, the stages overlap and multi-node prefill roughly doubles.
Measured
Two DGX Sparks (GB10, aarch64) over ConnectX-7, Qwen3.8-27B-UD-Q4_K_XL (16.4 GiB),
llama-batched-bench -c 36864 -b 2048 -ub 512 -ntg 128, both GPUs clock-capped identically,binaries md5-verified identical on both nodes. "1 Spark" and "2 Sparks" use the same binary,
so the only variable is whether the model is split.
Prompt processing, tok/s:
-sm layerToken generation is unchanged, as expected -- a layer split moves the same number of weight
bytes per token, so decode cannot speed up and the only question is how much the hop costs:
End to end, which is what a user waits for (4096-token prompts, 8 concurrent, 128 tokens each):
60.24 s on one Spark, 41.50 s split across two -- 1.45x.
Before this cherry-pick, on the same hardware and the same model, the same split was
0.95x-0.99x, i.e. a small loss. The commit is the entire difference.
Correctness
Checked through
llama-serverwith the split on and off, and diffed:Every generated string is byte-identical between the single node and the split, including a
deliberately knife-edge short prompt that the unsplit node itself answers differently between
serial and batched execution. Prefix reuse is demonstrably live rather than silently disabled:
2399 ms cold versus 199 ms warm on the split.
This is unsurprising and worth saying explicitly: an RPC split has one
llama_context, one KVcache and one control plane, with the split below all of that at tensor placement. There is no
second scheduler to disagree with about
seq_id-to-slot mapping.Notes
against a 5.1
ggml-rpc-serverwill refuse to connect rather than misbehave.freedeltas of +10.9 GiB and +11.3 GiB for a 16.4 GiBmodel, i.e. one copy spread over the pair.
master(CPU and CUDA), and the CUDA build was used for everynumber above.
Also cherry-picked: ggml-org#26500
d0132a68alone leaves the fork one commit short for multi-worker setups. Upstream#26500
rpc: avoid serializing buffers from other servers(merged 2026-08-30) makes
serialize_tensordispatcher-aware, so a buffer pointer is onlyincluded when the buffer belongs to the RPC socket receiving the graph. Without it, a client
driving two or more
ggml-rpc-serverinstances serializes pointers owned by a different serverand the receiver fails with
[create_node] invalid data ptr/[graph_compute] failed to create graph node 0. That is open issue ggml-org#28047.It applies cleanly and brings the regression test upstream added with it. Run here:
Two Sparks only need one RPC worker, so this is not what produced the numbers above. It is
included because three or more machines do need it, and picking one of a pair of related
upstream commits is how a fork acquires a bug nobody can reproduce upstream.
After both picks,
ggml/include/ggml-rpc.his byte-identical to upstream master andggml/src/ggml-rpc/ggml-rpc.cppdiffers by 6 lines, all from an unrelated later ggml refactor(
ggml_backend_op_alloc_size_may_expand) that is not RPC-specific.