[Bugfix] Fall back to T1 when ARC cannot reclaim enough entries from T2 - #55461
Conversation
Keep CPU offload stores progressing when T2 has no eligible victims, while preserving pin protection, atomic failure, and single-pass scans. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wang Zupeng <zupenwang@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe ARC eviction policy now falls back to T1 when T2 cannot provide an evictable candidate. A parametrized test covers pinned keys, protected keys, eviction results, iterator bounds, and failed-store atomicity. ChangesARC eviction fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to ARC eviction now reclaims eligible T1 entries when T2 cannot satisfy a store, while preserving protected-key and failed-store behavior. Targeted regression coverage and the reported suite results support merging without an outstanding current-head risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Manny7717
left a comment
Manny7717
left a comment
There was a problem hiding this comment.
APPROVE — verified locally against head 2d7a48f and base 7fbd44c.
Bug is real. ARCCachePolicy.evict() preferred T2 whenever T1 was below its adaptive target, and returned None (store failure → prepare_store fails) whenever T2 had no eligible candidates — even when T1 held plenty of idle, unprotected blocks. The manager's own _num_evictable_cache_blocks pre-check (manager.py:204) can pass while the policy still returns None, so the store fails despite sufficient reclaimable capacity. This also bites mid-batch and whenever T2 entries are pinned/protected.
Fix verified. After the T2 scan yields nothing, the code now continues the (already-positioned, monotonic) T1 iterator and evicts from T1, decrementing virtual_t1_size so the batch simulation stays consistent. Scans stay single-pass — no quadratic restart.
Regression-proven: the new parametrized test test_store_falls_back_to_t1_when_t2_cannot_satisfy_eviction (transplanted onto base 7fbd44c) fails in 5/6 cases exactly as expected — the store is rejected (output is None) where head now evicts T1 and completes the store. The 6th case (expected_evicted=None) passes on both — correct atomic-failure control. Pinned/protected blocks survive in every case. Full tests/v1/kv_offload/cpu/test_manager.py: 34/34 pass at head (the 5 failures on base are precisely the new cases — zero collateral).
Design note: evicting from T1 when it is below its adaptive target deviates from the pure-ARC 'never shrink T1 under target' rule, but that rule exists to prefer T2 — when T2 cannot satisfy, failing the store is strictly worse, and this matches canonical ARC's empty-T2 fallback. The counting asserts (items_yielded <= len) lock in the no-rescan property.
Non-blocking: the eviction-failure path still returns None when both lists are exhausted of eligible entries, which is correct; nothing further needed here.
|
/ci run |
|
✅ @zupengwang, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #87432 for commit |
…T2 (vllm-project#55461) Signed-off-by: Wang Zupeng <zupenwang@gmail.com> Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
3 participants
Purpose
CPU KV offload stores can fail even when ARC has enough evictable entries. When T1 is below its adaptive target, eviction prefers T2 but currently returns
Noneonce T2 has no eligible candidates. This also occurs partway through a batch, or when T2 entries are pinned or protected.Fall back to eligible T1 entries after exhausting T2. Preserve the existing preference order, monotonic queue scans, pin/protection rules, and atomic failure when the total candidate count is insufficient. Add six parameterized cases to the existing manager suite using normal stores and ghost hits to reproduce the adaptive state.
Duplicate-work checks on 2026-09-05 found no open PR implementing this CPU ARC fallback. #50422 adds session-aware policy context without fixing this selection path; #40270 targets GPU BlockPool ARC; #51787 concerns LRU transfer ordering. This change preserves the single-pass optimization merged in #50992.
AI assistance was used to investigate, implement, and test this change.
Test Plan
Tested against upstream
7fbd44cbe0a90b9c8fd3a94a0f0401ac4b1bc719on Linux with Python 3.12.13 and PyTorch 2.13.0+cu130. The isolated checkout imports its own upstream Python source and reuses dependencies and binary extensions from an existing vLLM 0.28.0 installation.Test Result
git diff --checkpassed.These are CPU unit tests. No model evaluation, GPU test, or end-to-end offload throughput benchmark was run; no performance or model-quality claim is made.