fix: decouple virtual KV capacity from shared-GPU profiling - #448
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds cross-process startup serialization for vLLM’s shared-GPU memory profiling window when kvcached is enabled, preventing concurrent vLLM instances from corrupting each other’s CUDA-graph-based KV cache budgeting (related to #193).
Changes:
- Introduces a per-visible-GPU
flock-based lock (configurable via env vars) to serialize vLLM’s profile-to-CUDA-graph capture window. - Updates the vLLM EngineCore patch to lock
_initialize_kv_cacheswhen available, with a compatibility fallback to locking the full EngineCore init. - Adds unit tests validating lock resource selection, patch scoping, and POSIX cross-process serialization; includes the new test in the CPU manifest.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
kvcached/integration/vllm/patches.py |
Adds startup-locking primitives and integrates them into the EngineCore patching flow. |
tests/test_vllm_startup_serialization.py |
Adds unit tests covering resource selection and cross-process locking behavior (POSIX-only where needed). |
tests/test_vllm_tp_world_size.py |
Disables startup locking in this unit-test module to keep Windows test runs working. |
tests/manifests/cpu.txt |
Ensures the new serialization test runs in the CPU test manifest. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d1581a3 to
055518f
Compare
055518f to
c892296
Compare
|
I replaced the previous startup-serialization approach with the lock-free implementation now in this PR. The lock prevented two initializers from profiling at the same time, but it did not make vLLM's whole-device profiling assumption valid. After engine A released the lock, it could begin serving and change physical KV mappings while engine B profiled; the same problem remained for a later engine C. A fully correct lock would require quiescing every running peer, which would stall serving traffic and make cold starts increasingly expensive. The new implementation instead separates logical KV sizing from transient whole-device free memory:
On a Tesla T4 with vLLM 0.18.0 and Qwen2.5-0.5B-Instruct, two C256 engines started simultaneously without a startup lock, received the same logical KV capacity, reached health, and both returned The branch also passes 22 focused tests and the full 157-test Linux CPU manifest. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
kvcached/integration/vllm/patches.py:1958
- patch_worker_determine_available_memory() temporarily sets cache_config.kv_cache_memory_bytes, but the finally block always restores it to None. Even though this branch is entered only when it was None, restoring the prior value is safer in case vLLM mutates/normalizes the field internally or a future config type changes the default value.
cache_config.kv_cache_memory_bytes = virtual_budget
try:
available_memory = int(
original_determine(self, *args, **kwargs)
)
c892296 to
ab52706
Compare
|
One limitation of the current null-block retry should be explicit: if physical KV capacity never becomes available, post-initialization remains waiting instead of failing fast. Concretely, the null block is block 0, which the engine reserves as a sentinel before normal KV allocations begin. The upstream code attempts this reservation once; if With this PR, logical KV sizing can complete even while the physical page allocator has no page available. During
The side effect appears when the configuration is permanently infeasible, for example when colocated engines retain all physical KV pages and none can ever be released for the new engine. In that case the process does not busy-spin or crash, but it also never becomes usable: post-initialization keeps retrying, and callers waiting on A fixed timeout would turn temporary pressure back into a timing-dependent startup failure, so this PR does not invent an arbitrary one. A durable follow-up should make the wait lifecycle-aware and cancellable, or connect it to an explicit startup deadline supplied by the caller, and expose the waiting state clearly for operators. The indefinite-wait behavior is therefore a real tradeoff that should be considered when reviewing this change. |
|
I added another observed failure mode to the PR description. An unfixed colocated startup recorded free memory increasing from This is the inverse of a peer allocating memory during profiling: a peer released about |
ab52706 to
8d163bb
Compare
8d163bb to
861e9ef
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_sglang_autopatch_order.py:24
- The test derives patch ordering via
ast.walk(tree), butast.walkdoes not guarantee a source-order traversal. That makes this ordering assertion potentially unreliable across Python versions / AST changes, and it could accidentally include unrelated*Patch()calls elsewhere in the file. Parse the specificregister_patches_with_versions([...])list literal and read its elements in-order instead.
patch_names = [
node.func.id
for node in ast.walk(tree)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id.endswith("Patch")
]
861e9ef to
24a3f2d
Compare
24a3f2d to
d693adc
Compare
|
LGTM. Two suggestions. Take the direct path on the vLLM side too, like SGLang. The SGLang patch replaces
Measured on an A100,
|
d693adc to
24b136d
Compare
|
Good catch. I changed the vLLM path to profile directly and calculate:
I also reran the simultaneous-start test on a T4 with two vLLM 0.18.0 C256 engines. Both produced the same values:
Both reached health and returned exactly |
fa5af1f to
e920fc7
Compare
e920fc7 to
e225acf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
kvcached/integration/vllm/patches.py:1946
- The direct profile path omits vLLM's process-local CUDA-graph estimate. Supported vLLM 0.18/0.22 releases call
profile_cudagraph_memory()and subtract it whenVLLM_MEMORY_PROFILER_ESTIMATE_CUDAGRAPHSis enabled; this calculation subtracts only weights and the forward-pass torch peak, so that opt-in configuration overstates KV capacity and can leave no room for graph capture. Preserve and deduct the enabled graph estimate without reintroducing whole-device deltas.
torch_peak_increase = int(profile_result.torch_peak_increase)
available_memory = (
virtual_budget - weights_memory - torch_peak_increase
)
029e2e1 to
b224bf1
Compare
|
One safety boundary is worth making explicit before merge. The new vLLM formula is intentionally a per-engine logical KV capacity. It subtracts process-local weights, torch peak, and the applicable CUDA Graph estimate, but it does not subtract peer engines'' physical costs. With large colocated models, peer weights alone can be tens of GiB, so the aggregate logical capacities may overcommit physical memory by a large margin. That overcommit is acceptable only if kvcached''s runtime physical admission remains authoritative and atomic across processes. The current scheduling path does re-read We observed that this is not merely a theoretical cold-start path in an 80B shared-GPU overload run. That build included a per-physical-GPU growth guard covering the headroom check and mapping transaction. Under load it recorded:
This does not mean peer weights should be subtracted from the logical capacity: doing so would make capacity order-dependent again and defeat elastic overcommit. It means this PR increases the importance of a separate physical-growth transaction boundary. The required lock is also different from the startup/profiling lock removed earlier. It would serialize only: It would not serialize model loading, profiling, normal inference, or already-backed KV access. I suggest documenting atomic runtime physical admission as a safety dependency/known limitation of logical overcommit, and handling the per-GPU growth transaction in a focused follow-up rather than mixing it into this profiling fix. |
|
Both changes look right. Two things. Why subtract the CUDA Graph estimate? It comes from the same device-wide Could this run on the versions we support? Validation used vLLM 0.18 / SGLang 0.5.13.post1 on a T4; README and CI target 0.24.0 / 0.5.15. |
b224bf1 to
aba5125
Compare
|
Validated the currently supported versions and updated the PR accordingly.
The vLLM 0.24 run also exposed its The final commit is |
|
Thanks! It looks better a lot! |
Summary
Fix the shared-GPU startup capacity failures described in #193 without serializing colocated vLLM or SGLang engines.
When kvcached is enabled, logical KV capacity is derived from stable device geometry and process-local memory costs. It is no longer derived from whole-device free-memory deltas that can change while another process is starting, serving, or exiting.
The known startup free-memory guard becomes a targeted warning in this mode. Explicit
kv_cache_memory_bytesremains authoritative, kvcached-disabled behavior is unchanged, and unrelated model or CUDA failures still propagate.Fixes #193.
Root cause
kvcached reserves CUDA virtual address space for KV tensors, but model weights, CUDA contexts, profiling workloads, activations, communication buffers, CUDA Graph capture, and physical KV page mappings still consume physical GPU memory.
vLLM computes a requested budget from the whole device:
It then profiles non-KV costs and calculates the KV remainder:
This calculation explicitly assumes that other processes sharing the GPU do not change their memory use during profiling:
That assumption does not hold for colocated engines.
MemorySnapshotobserves whole-device free memory, while PyTorch allocator counters are process-local. A peer's allocation or release can therefore be attributed to the worker being profiled.Observed failure modes
Initial free-memory guard
If a peer has already consumed memory before the initial snapshot, startup can fail with:
The previous kvcached patch bypassed this guard but replaced
requested_memorywith current free memory. The subsequent profiling calculation remained sensitive to peer activity.Peer allocation during profiling
If another process allocates memory during profiling, that growth can be counted as the current worker's
non_torch_increase. The resulting KV capacity may become zero or negative:This can produce
No available memory for the cache blocks, or a positive capacity too small for one request atmax_model_len.Peer release during profiling
The inverse failure is also possible. An unfixed colocated startup observed free memory increase from
8.57 GiBto12.91 GiBduringdetermine_available_memory(). That violated vLLM's assertion:EngineCore then terminated with
Error in memory profiling. The snapshot is device-wide, so the peer does not need to be in the same process or container to invalidate it. See the vLLM 0.22.1 check.Why startup serialization is insufficient
Serializing two initializers prevents simultaneous profiling, but it does not make whole-device deltas process-local:
The same issue remains when engines A and B are serving and engine C starts later. A fully correct lock would require quiescing every running peer, which would stall traffic and make cold-start latency grow with the number of instances.
Implementation
vLLM
For supported vLLM releases, the patch:
Mirrors vLLM's
ceil(total_memory * gpu_memory_utilization)formula to establish a stable logical budget.Downgrades only the known whole-device startup guard to a warning when kvcached is enabled.
Reuses the budget stored in
self.requested_memoryinstead of recomputing it from mutable free memory.Runs the required profile workload, but sizes KV from process-local costs:
Ignores device-wide
non_torch_increase,after_profile.free_memory, the cross-process free-memory assertion, and the CUDA Graph estimate for KV sizing.Still runs
profile_cudagraph_memory()when supported and recordsself.cudagraph_memory_estimatefor vLLM's later comparison/logging, but does not fold the estimate intopeak_activation_memoryor subtract it from logical KV capacity.Uses runtime feature detection for older supported workers that predate
init_snapshot: afterinit_device(), it stores the same logical budget and measures the reset process-local PyTorch peak.Leaves explicit
kv_cache_memory_bytesand kvcached-disabled paths unchanged.SGLang
For SGLang 0.5.11 and later, the patch wraps
ModelRunner._profile_available_bytes():The rank-local result is reduced with
MINacross the current SGLang world group before pool sizing, preserving identical TP geometry. If any rank cannot query its local capacity, a failure sentinel makes every rank fall back to SGLang's original distributed profiler, so collective order cannot diverge.Mamba reservation is applied after synchronization. Non-GPU devices and kvcached-disabled runs retain the original profiler. The patch is installed after elastic memory-pool aliases because importing
ModelRunnercaches those pool classes in module globals.Temporary physical pressure
The vLLM and SGLang integrations reserve block 0 as a null block during post-initialization. If physical KV capacity is temporarily unavailable, reservation now sleeps and retries instead of terminating EngineCore. It also retries the race where capacity is observed but another engine takes the page before allocation.
The expected block identity remains fail-loud. CUDA allocation failures, unsupported layouts, invalid geometry, and unrelated runtime failures are not converted into warnings.
Validation
Hardware validation used one NVIDIA Tesla T4.
OKsglang-kernel0.4.4+cu129, Qwen2.5-3B-InstructOKThe vLLM 0.24.0 run also verifies the worker contract required by
compile_or_warm_up_model():self.non_torch_memoryis initialized without reintroducing the device-wide delta. Separate regressions verify that a large peer-drivennon_torch_increaseand the CUDA Graph estimate do not change logical KV capacity.Known limitation
This patch does not make a permanently infeasible configuration runnable. At least one physical page and sufficient non-KV headroom must eventually become available.
While reserving the null block,
_post_init_doneis not set until block 0 is allocated. Temporary pressure can recover, but if no physical KV page ever becomes available, post-initialization keeps waiting instead of failing fast. The retry sleeps and does not busy-spin, but the engine does not become usable.This PR does not introduce an arbitrary timeout because that would turn temporary pressure back into a timing-dependent startup failure. A follow-up can make the wait lifecycle-aware and cancellable, or connect it to an explicit startup deadline supplied by the caller.
Design boundary
This is a compatibility fix for the current kvcached integrations. It does not replace vLLM's complete memory profiler or define a shared-GPU physical-memory accounting model.
The selected boundary, rejected alternatives, known limitation, and criteria for reopening the larger design are documented in Discussion #454.