[KVOffload] Document CPU eviction behavior as reference (control branch) - #47234
Open
Alex-ai-future wants to merge 4 commits into
Open
Alex-ai-future wants to merge 4 commits into
Alex-ai-future wants to merge 4 commits into
Conversation
The FIXME claimed num_stored_blocks cursor could become stale when CPU LRU evicts blocks in the middle of a request. Analysis shows this scenario cannot occur: 1. Active request blocks (ref_cnt > 0) are not in the free queue, so LRU cannot evict them. 2. Finished request blocks (ref_cnt == 0) can be evicted, but their offload state is already cleaned up, so the scan loop skips them. Replace the FIXME with a NOTE explaining the safety argument, and add test_active_request_blocks_not_evicted which proves that blocks with ref_cnt > 0 survive LRU eviction while ref_cnt == 0 blocks do not. Signed-off-by: jihuihuang Signed-off-by: Alex <alex.tech.lab@outlook.com>
The original NOTE claimed active request blocks (ref_cnt > 0) cannot be evicted, but conflated 'active request' with 'ref_cnt > 0'. After _process_store_completion() frees blocks, ref_cnt drops to 0 and blocks become LRU eviction candidates even for active requests. Rewrite the NOTE with accurate reasoning: 1. In-flight blocks (ref_cnt=1 during async DMA) leave free queue; when num_free=0 the store loop defers (out_of_space) rather than evicting. 2. Post-completion blocks (ref_cnt=0) can be evicted; the load path handles this gracefully via shorter cache hits. 3. Preemption resets the cursor, causing re-scan of evicted blocks. 4. Finished requests are skipped (state is None or finished). Replace the broken test (touch() on req_b was a no-op because req_a was already at the LRU head) with a real in-flight store test that proves the out_of_space deferral mechanism. Signed-off-by: Alex <alex.tech.lab@outlook.com>
The FIXME worried that evicted blocks below the cursor would be silently lost. Rewrite the NOTE to explain why this is an intentional O(1) vs O(N) trade-off: re-scanning from block 0 every step would waste bandwidth on redundant store attempts. Key clarification: active request blocks CAN be evicted after store completion (ref_cnt drops to 0). They are protected by LRU ordering (MRU tail, evicted last) but not by ref_cnt. The load path handles eviction gracefully via shorter cache hits and GPU recomputation. Add test_active_request_blocks_can_be_evicted to prove the FIXME's scenario: an active (not finished) request's stored blocks are evicted by a subsequent store. This validates NOTE claim vllm-project#2. Signed-off-by: Alex <alex.tech.lab@outlook.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Signed-off-by: Alex <alex.tech.lab@outlook.com>
Contributor
|
This pull request has merge conflicts that must be resolved before it can be |
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
1 participant
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.
Purpose
Document and test the current
SimpleCPUOffloadSchedulereviction behavioras a reference for the FIXME:
num_stored_blocks can be stale and omit evicted blocks in the middle of the request.This branch is a control/reference. It does not change the eviction
algorithm. Instead it explains why the current behavior is safe and provides
tests that prove the documented properties.
Problem
After
_process_store_completion()frees CPU blocks (ref_cnt → 0), theyjoin the free queue tail (MRU). A subsequent
get_new_blocks()call canre-allocate these blocks, removing their cache entries via
_maybe_evict_cached_block(). This can happen while the original request isstill active — its
num_stored_blockscursor has advanced past the evictedblocks and they will never be re-stored.
Why this is safe
The trade-off is intentional: re-scanning from block 0 every step would turn
this loop from O(new blocks) into O(total blocks), wasting bandwidth on
redundant store attempts.
find_longest_cache_hit()returns a shorter match; missing tokens are recomputed from GPU — no data lossref_cnt=1during async DMA leave the free queue entirelynum_stored_blocksresets to 0 on preemption, causing evicted blocks to be re-scannedChanges
vllm/v1/simple_kv_offload/manager.pytests/v1/simple_kv_offload/test_scheduler.pyTest Plan
test_in_flight_store_protectedref_cnt=1) leave the free queue; whennum_free=0the store loop defers (out_of_space) rather than evictingtest_active_request_blocks_can_be_evictedTest Result
ruff: All checks passed.
Limitations
documents the current behavior as a reference point for evaluating the
fix-cpu-eviction-trackingbranch which adds re-store logic.NOTE explains why the trade-off (O(1) cursor vs. re-scan cost) is acceptable.