feat(vllm): avoid CUDA context in EngineCore - #416
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “control-only” mode for kvcached in vLLM EngineCore to avoid creating a CUDA context in the EngineCore process, while still maintaining correct allocation capacity by querying CUDA memory info from TP worker processes.
Changes:
- Introduces a worker-based CUDA meminfo query path (Unix-socket IPC) and a retrying
meminfo_provider. - Extends
KVCacheManager/PageAllocatorto support a non-CUDA control plane by caching meminfo and accounting for map/unmap deltas. - Wires vLLM integration to initialize kvcached in control-only mode when
KVCACHED_ENGINECORE_NO_CUDA=true, and adds targeted tests for the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tp_ipc_meminfo.py | Adds tests for worker meminfo IPC behavior and disconnect resilience. |
| tests/test_enginecore_no_cuda.py | Adds tests for control-only initialization and fail-closed meminfo refresh semantics. |
| kvcached/utils.py | Adds env-config flags/timeouts for EngineCore no-CUDA mode and meminfo refresh behavior. |
| kvcached/tp_ipc_util.py | Adds cuda_mem_get_info IPC command + client query helper. |
| kvcached/meminfo_provider.py | Adds worker-meminfo snapshot acquisition with bounded retry loop. |
| kvcached/kv_cache_manager.py | Adds control-plane mode, disables prealloc in control-only, and refreshes meminfo for capacity checks. |
| kvcached/integration/vllm/patches.py | Passes control_only into init_kvcached for EngineCore/coordinator setup. |
| kvcached/integration/vllm/interfaces.py | Implements control_only init path and propagates cuda_control_plane into KVCacheManager. |
| csrc/torch_bindings.cpp | Extends PageAllocator binding with cuda_control_plane and exposes set_mem_info. |
| csrc/page_allocator.cpp | Implements cached meminfo path + map/unmap accounting when not acting as CUDA control plane. |
| csrc/inc/page_allocator.hpp | Updates PageAllocator API/state for cached meminfo and accounting hooks. |
Comments suppressed due to low confidence (1)
kvcached/integration/vllm/interfaces.py:101
- In the main
is_workerinitialization path, the listener is also started withdevice_index=int(torch.cuda.current_device()). Ifinit_kvcached()is called with an explicitdevicestring that isn't the current CUDA device, the listener will report memory info for the wrong device.
Use the (normalized) device string to choose the device_index, falling back to torch.cuda.current_device() only when the string has no index (e.g. "cuda").
if is_worker:
# start the listener thread for kv cache management regardless of TP size
# because the vLLM EngineCore might need to reach this worker if PP > 1
start_worker_listener_thread(
tp_rank, pp_rank,
device_index=int(torch.cuda.current_device()))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a0eeb7c to
c39758c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
tests/test_tp_ipc_meminfo.py:74
- The socket-wait loop should assert that the worker socket file exists after the deadline. Otherwise, a listener startup/bind failure can surface later as an unrelated connection error, which is harder to debug and can make the test appear flaky.
socket_path = tmp_path / "w0.sock"
deadline = time.monotonic() + 2.0
while not socket_path.exists() and time.monotonic() < deadline:
time.sleep(0.01)
tests/test_tp_ipc_meminfo.py:46
- The socket-wait loop has no assertion that the worker socket was actually created. If the listener thread fails to bind/start, the test will fail later with a less-informative connection error or timeout, making it harder to diagnose.
Add an explicit assert after the deadline so failures point directly at listener startup.
This issue also appears on line 70 of the same file.
socket_path = tmp_path / "w0.sock"
deadline = time.monotonic() + 2.0
while not socket_path.exists() and time.monotonic() < deadline:
time.sleep(0.01)
ccc1799 to
121ff42
Compare
121ff42 to
6c2f4af
Compare
b7f729a to
390ebcc
Compare
|
Hi, although the cuda context memory is real gain, but i dont think this trade is worth. Because i think this pr will bring stale data into the critical path, which is the allocation path. Then the fail-then-rollback will be triggered even more. |
|
Thanks, I think this concern is valid. The current implementation fails closed when a worker query fails, but a successful snapshot is cached for one second and adjusted only for mappings performed by this process. A colocated peer can change physical usage during that interval, so the control process can still over-admit and increase allocation misses and rollback frequency. Avoiding the EngineCore CUDA context has a measurable memory benefit, but that does not justify adding this consistency tradeoff to the allocator critical path upstream. I will close the current design. If this optimization is revisited, it should use a worker-owned admission/accounting contract rather than polling and caching whole-device free memory in the control process. |
|
Thanks for the feedback on the cost and consistency trade-off. I have reopened this PR as a draft; the current cached-meminfo implementation is not intended to merge as-is. I would like to check whether the following ownership boundary matches the direction maintainers would accept:
This keeps the roughly 300 MiB EngineCore context saving, but moves final physical admission back to the CUDA-owning workers and avoids making a stale IPC snapshot part of the scheduler's critical path. It would rely on the transactional map/rollback contract from #418. Would this design be acceptable? In particular, would you prefer admission to be fused into the existing map request, or exposed as an explicit prepare/commit protocol across TP workers? Before marking the PR ready, I would also validate high-concurrency serving and injected worker-IPC delay, since even without a separate meminfo fan-out the worker-side admission/map path must not introduce a measurable TTFT/TPOT regression. |
390ebcc to
62c940d
Compare
|
I updated the Draft implementation to use worker-owned physical admission:
The cost is controlled by The exact candidate passed the 193-test CPU manifest and a real-GPU vLLM smoke. I am keeping the PR in Draft until the #418 dependency and the combined failure path are validated. |
|
Follow-up from a higher-concurrency real-GPU validation of a combined candidate containing this worker-owned admission design and its required transactional failure handling. The correctness result was good: both colocated TP=4 instances completed 390/390 requests, with no request failures, process restarts, CUDA OOM, illegal memory access, or EngineCore startup failure. The previous permanent first-page wait also did not reproduce. However, the performance result rejects the current design:
During the run, the two instances recorded 2,182/2,176 headroom rejections and 1,102/1,096 allocation misses. Repeating the same workload with the EngineCore-no-CUDA worker-admission path removed restored average throughput to within about 1.4% of the established result. Headroom contention itself was not eliminated and remained asymmetric, but it no longer turned the physical-growth path into the same throughput bottleneck. The current implementation no longer performs a separate cached-meminfo query in So I do not think this PR should merge in its current form, even as an opt-in implementation. Avoiding the extra CUDA context remains a useful goal, but the worker-owned path needs a different design, likely including persistent/batched control transport and explicit retry backoff or event-driven admission, followed by the same high-concurrency validation. |
Summary
Avoid creating a redundant CUDA context in vLLM's EngineCore process by
moving physical KV admission to the CUDA-owning workers.
The behavior is opt-in through
KVCACHED_ENGINECORE_NO_CUDA=true. The defaultvLLM and SGLang paths are unchanged.
Why
In vLLM V1, the GPU workers already own model execution and KV tensor mapping,
but kvcached also initialized CUDA/VMM state in EngineCore. That created an
additional CUDA context even though EngineCore only needs to maintain logical
KV state.
On a 4x RTX 4090 TP4 deployment, the redundant EngineCore context occupied
386 MiB. Removing it increased idle free memory on rank 0 by 409 MiB.
Design
CUDA/VMM state.
periodically query worker meminfo.
TP worker in the current PP group. Each CUDA-owning worker performs admission
and mapping for its own device.
background thread cannot re-enter Python to reach remote workers.
before serving map/unmap operations because CUDA's current device is
thread-local.
This PR deliberately does not add a representative-TP meminfo cache, a
per-allocation meminfo RPC, or a cross-process file lock. CUDA remains the
final physical admission authority. A failed multi-worker mapping must be
reported as an allocation miss after transactional rollback, so this design
depends on #418 (and uses the scheduling-miss behavior from #453).
Cost and fallback
The runtime cost is bounded by an explicit feature flag:
KVCACHED_ENGINECORE_NO_CUDA=false(the default), no runtime behaviorchanges.
IPC. Only the existing worker map/unmap IPC is used when physical pages grow.
code rollback.
Validation
62c940d057f49297e6c9711b128ad53af53e8d8a193 passed13 passedand Linux-platform MyPy passed.
autopatch loaded, control-only initialization and the worker listener were
observed, the server reached healthy state, and an OpenAI-compatible request
returned exactly
OK.The PR remains Draft until the worker-side transactional rollback dependency
is available and the combined path is validated.