fix(kv-cache): size pages via get_block_range in _get_num_alloced_blocks - #452
Open
SuperMarioYL wants to merge 2 commits into
Open
fix(kv-cache): size pages via get_block_range in _get_num_alloced_blocks#452SuperMarioYL wants to merge 2 commits into
SuperMarioYL wants to merge 2 commits into
Conversation
When block_mem_size does not evenly divide page_size (HYBRID_LINEAR / Mamba GDN per-block state), InternalPage.get_block_range drops blocks straddling the page boundary. Some page ids yield zero usable blocks while get_num_blocks reports one or more; _alloc parks those 0-block pages in full_pages (kv_cache_manager.py:335), but _get_num_alloced_blocks still counted them with the theoretical get_num_blocks, inflating the lazy-shrink completion gate _get_num_alloced_blocks() <= target_num_blocks and stalling the operator kvctl limit shrink. Factor the boundary-aware capacity into a module-level _page_capacity helper (the abstraction get_page_occupancy already uses) and sum it per page id in _get_num_alloced_blocks. available_size keeps the theoretical upper bound for its free-page count term: the allocator exposes only a count of free pages, not their ids, so a precise fix needs page-id enumeration (a C++ change, out of scope); an inline note documents this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Draft — Round 1
Title:
fix(kv-cache): size pages via get_block_range in _get_num_alloced_blocksBase:
main← Branch:fix/page-capacity-get-block-rangeSummary
KVCacheManager._get_num_alloced_blocksandavailable_sizesized every page withInternalPage.get_num_blocks(page_size, block_mem_size)— the theoreticalpage_size // block_mem_size. Butget_page_occupancyalready uses theboundary-aware
InternalPage.get_block_range(page_id, page_size, block_mem_size),and the comment there explains why: blocks straddling a page boundary belong to
neither page, so a page's capacity comes from its own
get_block_range, not frompage_size // block_mem_size.When
block_mem_sizedoes not evenly dividepage_size(HYBRID_LINEAR / Mamba GDNper-block state — the exact case the
_allocparking comment atkv_cache_manager.py:335names), some page ids yield zero usable blocks whileget_num_blocksreports one or more. Those 0-block pages are parked infull_pages(so they're not re-handed-out) but were still counted byget_num_blocks, inflating_get_num_alloced_blocks.That inflation is observable, not cosmetic:
free()'s lazy-shrink completion gate_get_num_alloced_blocks() <= target_num_blocksreads the inflated count, so anoperator's
kvctl limitshrink never crosses the threshold and physical GPU memoryis not returned (the elastic-shrink-stalls failure).
Fix: add a module-level
_page_capacity(page_id)helper that mirrorsget_page_occupancy'sget_block_rangeusage, and use it in_get_num_alloced_blocks(per tracked page id infull_pages/avail_pages) so aparked 0-block page contributes 0 instead of the theoretical count. The helper is
module-level (not a staticmethod) so it is unit-testable without the compiled
kvcached.vmm_opsextension or a GPU, matching the_get_max_cached_blocks/_make_cache_keyidiom.Scope:
available_size'sfree_pagesterm could not be made per-id precise —the C++
PageAllocatorexposes only a count of free pages, not their ids, sothe boundary-aware
_page_capacity(which depends onpage_id) can't be appliedthere without a C++ change. The
avail_blocksterm inavailable_sizewas alreadyboundary-aware (
num_avail_blocksis maintained frompage.num_free_blocks(), whichInternalPage::initpopulates fromget_block_range). An inline note documents theremaining upper-bound term and the precise-fix prerequisite.
Test
tests/test_block_count_consistency.py(registered intests/manifests/cpu.txt)builds a real
KVCacheManagerviaobject.__new__(pertest_resize_reserved_order.py)with
page_size=4, block_mem_size=3(3 does not divide 4) and a stubInternalPageexposing the real static
get_num_blocks/get_block_rangeformulas. It asserts_get_num_alloced_blocks()returns theget_block_range-based count, not theinflated
get_num_blockscount. The test is red onmain(concrete valuefailures
assert 1 == 0/assert 2 == 1, not import errors, and does not mock thefunction under fix) and green on this branch.
CPU suite:
142 passed(was 138; +4 from the new test); the 10 errors arepre-existing macOS
/dev/shmenvironment failures in unrelated files.Why not just
available_sizeAddressed above —
available_size's count-based free-page term is an upper bounduntil the allocator exposes free page ids; the load-bearing accounting
(
_get_num_alloced_blocks, used by the shrink gate) is fully fixed here.