feat: add revisioned instance memory limits - #414
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
1c89792 to
db9ccfe
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an instance-level, revisioned physical-GPU memory cap that can be assigned by an external integration and then enforced locally by each live KV pool (both in Python and in the native page allocator), including support for shrinking without revoking in-use mappings.
Changes:
- Added a provider-owned, revisioned per-pool physical mapping limit in
KVCacheManager, and updatedavailable_size()to treat an active limit as the allocator-local “fast path” (no GPU free-memory query). - Introduced a new
kvcached.controlregistry (weakrefs) to split/apply an instance-wide cap across all registered pools, and wired vLLM/SGLang integrations to register/clear pools on lifecycle events. - Extended the C++
PageAllocator+ PyTorch bindings to support physical page limits, remaining-capacity queries, and a native per-GPU “growth guard” using a PCI-bus-ID file lock and a final free-memory headroom check.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_physical_memory_limit.py | Adds focused unit coverage for revisioning, alignment, deferred shrink semantics, and control-plane split behavior. |
| kvcached/kv_cache_manager.py | Implements per-pool physical limit state, revision handling, and available_size() behavior under active limits. |
| kvcached/integration/vllm/interfaces.py | Registers KV managers as physical-memory pools and clears them on shutdown. |
| kvcached/integration/sglang/interfaces.py | Registers KV managers as physical-memory pools and clears them on shutdown. |
| kvcached/control.py | New control-plane module for weakref pool registry + deterministic limit splitting and application. |
| csrc/torch_bindings.cpp | Exposes new PageAllocator limit and accounting APIs to Python via pybind. |
| csrc/page_allocator.cpp | Enforces a physical page limit in foreground alloc and background prealloc; adds limit/accounting helpers. |
| csrc/inc/page_allocator.hpp | Declares the new physical-limit APIs and internal helpers on PageAllocator. |
| csrc/inc/gpu_vmm.hpp | Adds a portable (CUDA/HIP) PCI bus ID query wrapper for per-GPU locking. |
| csrc/inc/allocator.hpp | Extends allocator state to track bytes-per-offset for guarded mapping. |
| csrc/allocator.cpp | Adds the cross-process “physical growth guard” and overflow-checked transaction sizing for VMM mappings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
77a68db to
529dccb
Compare
529dccb to
c5c934d
Compare
|
Thanks! Before comparing designs I walked the current What happens today when the limit is loweredTwo notes on this path: "Never revoke active mappings" is structural, the shrink path only ever draws from "Block additional physical growth until mapped usage falls below the cap" is also achieved by One correction on page statesThere is no "unused" page category distinct from reserved. A page is in exactly one of four states:
The allocator's own accounting says the same ( Gap 1: the limit is only polled from the allocation pathAn instance with no traffic never notices that its limit changed, and "an idle instance hands memory back" is the main thing this feature is for. Gap 2: no revision, no acknowledgement (You mentioned in the comment)On hard limit vs immediate hard limitAgreed, and I think that framing is right: a hard limit on new physical growth, not an immediate on. CPU offloading as a separate follow-up for the stronger convergence case sounds right too. QuestionCould you enumerate what the proposed cap covers that |
|
Thanks for tracing the existing path end to end. I agree with your conclusion about the downward mechanics: The remaining distinction is the contract being limited. After your walkthrough, I think the focused delta should be:
The cross-process physical-growth guard is orthogonal and materially broadens the map path, so I will remove it from this PR and treat it separately. I will revise the implementation around that narrower boundary after the allocator counter/snapshot prerequisite in #443 is available. Thanks, this was a useful correction to the current shape of the PR. |
c5c934d to
891921a
Compare
|
Reworked in The revised implementation is now stacked on #443, keeps virtual pool capacity unchanged, limits mapped physical residency in both foreground allocation and preallocation, and reuses the existing free/trim behavior for downward convergence. The cross-process growth guard and the parallel shrink design have been removed. Validation on the revised head:
The PR description has been updated with the current contract and dependency. |
891921a to
1ca3c6c
Compare
|
Hi @shipiyouniao , After reviewing this pr, i still think we don't need a new mechanism, please see my below reasons: 1. The reservation is made once in Your claim "reduces logical/virtual pool capacity by changing To me, it feels kind of inconsistent, if we have two mechanisms, one limits on the num of virtual pages, and the other limits on the num of physical pages. I think a very detailed case that explicitly illustrate why we must have a new mechanism instead of just enhancing current 2. What I do want out of this PR, pointed at
|
|
Thanks, I agree with this correction. I had conflated allocator page capacity with CUDA virtual-address reservation. Since the address space is reserved by I will revise this PR to enhance the existing
The separate physical-page limit, pending-map accounting, and duplicate allocation/preallocation gates will be removed. I will also add focused coverage for idle shrink, deferred shrink without revoking active mappings, regrowth, and multi-pool budget splitting. Thanks for walking through the existing semantics in detail. |
1ca3c6c to
7ca2977
Compare
7ca2977 to
69697d4
Compare
|
Reworked in The separate physical-page limit and all duplicate C++ allocation/preallocation gates have been removed. The PR now contains only the revisioned control protocol around the existing
The final diff is one commit touching four Python/test files, with no C++ map or preallocation hot-path changes. All 11 GitHub CPU, MyPy, and pre-commit checks passed. |
|
LGTM, thanks. I will merge it now, but perhaps you can add some document/instruction to tell people (who wanna build their own production layer upon these) how to use them. |
|
Added the requested production integration guidance in #458. It documents the process-local control-handler boundary, revision and acknowledgement semantics, deferred convergence polling, multi-pool splitting, and the ownership boundary between kvcached and the external controller. |
Baseline
Built on current
main, including the coherent allocator accounting from #443.Summary
KVCacheManager.resize()path, including while the instance is idleapplied,deferred,stale,conflict, orunavailablestatus together with revision and byte accountingin_shrink, free, trim, and reclaimed-page behavior for downward convergence and later regrowthRelationship to resize
This PR no longer introduces a separate physical-page limit or a second shrink state machine.
create_kv_tensors()owns the CUDA virtual-address reservation.resize()only changes allocator page bookkeeping and physical backing: shrinking moves page ids to the reclaimed list and unmaps releasable pages, while growing reuses those ids without rebuilding the KV tensors. In the current allocator,num_total_pages_already bounds maximum physical residency.The new code therefore adds the missing control protocol around the existing mechanism:
Ownership boundary
The caller owns quota policy, fairness, admission, and cross-instance arbitration. kvcached stores and applies the assigned limit next to the existing resize state machine.
Validation
The focused tests cover immediate idle shrink, deferred shrink without active-page revocation, convergence after usage drops, regrowth within the original reservation, maximum-capacity clamping, zero-limit drain, revision semantics, and multi-pool splitting.
All 11 GitHub CPU, MyPy, and pre-commit checks passed on the rewritten head.
Related design discussion: #375.