Repository navigation
Guard get_kv_cache_manager against num_blocks beyond created FTensor capacity - #469
Conversation
|
great! i'll give this a review when i can. |
| f"alloc_kv_cache() in this process for group {group_id}; " | ||
| "no allocation is recorded to derive the capacity from." | ||
| ) | ||
| return num_blocks |
There was a problem hiding this comment.
This still misses the supported manager-first order. If the manager is created for 65,536 blocks before alloc_kv_cache() clamps the tensors to 45,056, it is never revalidated and the #437 mismatch remains. Could alloc_kv_cache() check any existing manager's requested capacity before the tensors become available for mapping?
There was a problem hiding this comment.
This still misses the supported manager-first order. If the manager is created for 65,536 blocks before
alloc_kv_cache()clamps the tensors to 45,056, it is never revalidated and the #437 mismatch remains. Couldalloc_kv_cache()check any existing manager's requested capacity before the tensors become available for mapping?
Done in 519c763. alloc_kv_cache() now looks up any manager already registered for its group_id and validates it before create_kv_tensors(), so the tensors never become mappable for a manager that cannot address them. The check has to precede creation: the manager's _post_init thread polls kv_tensors_created() every 1 ms and starts mapping as soon as it flips.
kvcached/integration/vllm/interfaces.py:435-456: the per-group geometry record is built first,_validate_registered_managers(group_id, created_capacity)runs, thencreate_kv_tensors(), then the record is stored.interfaces.py:670_validate_registered_managers(): iteratesget_registered_kv_cache_pools(integration="vllm")fromkvcached.pool_registry, whereget_kv_cache_manager()already registers every manager it builds (weak references, so a collected manager from a torn-down engine cannot veto a new allocation), filters ongroup_id, and runs the shared validator onmanager.num_blocks/block_mem_size/num_layers/num_kv_buffers.interfaces.py:610_validate_manager_against_created_tensors(): the one validator both orders use. Same ValueError naming both numbers (65,536 requested vs 45,056 created); nothing is shrunk silently.
Tests in tests/test_vllm_ftensor_capacity_guard.py, TestManagerFirstOrder (L311): a manager for 65,536 followed by an alloc whose device budget clamps to 45,056 raises with both numbers, create_kv_tensors is never called and no record is written (L314); a matching manager lets the alloc proceed and records the geometry (L353); alloc-first end to end with num_blocks=None (L369); a manager for another group and a collected manager are not compared (L383, L392).
|
|
||
| block_mem_size = block_size * cell_size | ||
| capacity_num_blocks = ( | ||
| record["ftensor_bytes_per_layer"] // num_kv_buffers // block_mem_size |
There was a problem hiding this comment.
Could we also check that num_layers and num_kv_buffers match the allocation? In contiguous mode, these values determine the mapping stride. Right now num_layers is recorded but never checked, so the block count can pass while the mapping offsets are still invalid.
There was a problem hiding this comment.
Could we also check that
num_layersandnum_kv_buffersmatch the allocation? In contiguous mode, these values determine the mapping stride. Right nownum_layersis recorded but never checked, so the block count can pass while the mapping offsets are still invalid.
Done in 519c763. The record now also carries the num_kv_buffers the FTensors were created with (compound_num_kv_buffers: 2 for MHA/GQA, 1 for MLA and HYBRID_LINEAR), and both values are checked in both orders:
interfaces.py:640-652in_validate_manager_against_created_tensors():num_layersandnum_kv_buffersmust equal the record exactly, checked before the capacity check. The ValueError names both values, e.g.num_layers=32 does not match num_layers=16 of the KV cache tensors alloc_kv_cache() creates for group 0, and states why:FTensorAllocator::create_kv_tensorssizes the compound page askPageSize * num_layers * num_kv_buffers(csrc/allocator.cpp:142) andPageAllocator::map_pagesstrides offsets bypage_size_ * num_layers_ * num_kv_buffers_(csrc/page_allocator.cpp:677), so a mismatch maps at the wrong offsets even when the block count fits.interfaces.py:692_resolve_manager_num_blocks()now takesnum_layersand runs the validator forget_kv_cache_manager()(alloc-first order, call at L751).num_blocks=Nonederivation goes through the same check, so it cannot bypass a layout mismatch.- The manager-first order gets the same checks through
_validate_registered_managers(), which readsmanager.num_layers/manager.num_kv_buffers.
The check is unconditional rather than contiguous-only: in the per-layer layout the two values still size the PageAllocator's physical-memory accounting, and every in-tree caller already passes the same values to both calls (coordinator path: group_size and _get_kv_cache_params; v0.8 path: layer count and spec-derived buffer count), so no working configuration trips it.
Tests: num_layers and num_kv_buffers mismatches at the resolver (L232, L239), through get_kv_cache_manager() before construction (L280, L289), through alloc_kv_cache() before create_kv_tensors (L332, L341), and None derivation still checking layout (L247).
|
Addressed both review comments in 519c763, a second commit on top of 933b2ab (no rebase; upstream/main is unchanged at 60cad94).
Tests: 12 new in |
alloc_kv_cache() sizes the KV cache tensors from device memory and can create fewer blocks than requested (it warns and clamps). A KVCacheManager configured with the original, larger num_blocks exposes page ids beyond the FTensor's reserved virtual range; the first map past the reservation fails cuMemUnmap/cuMemMap and aborts the process inside FTensor::map (issue ovg-project#437, diagnosed by @rob-9: 65,536 requested vs 45,056 created on an L4, SIGABRT after the first bulk free/realloc cycle). Record the per-group capacity actually reserved by alloc_kv_cache() and validate get_kv_cache_manager()'s num_blocks against it, raising a ValueError that names both numbers instead of aborting later. num_blocks=None now derives the created capacity directly. When no allocation is recorded in the process (vLLM's engine/worker split), an explicit num_blocks is trusted unchanged, so integration flows are unaffected. Fixes ovg-project#437 Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu>
…kv_buffers alloc_kv_cache() now looks up any KVCacheManager already registered for its group_id in kvcached.pool_registry and validates it against the geometry about to be created, before create_kv_tensors() makes the tensors available for mapping (the manager's _post_init thread starts mapping as soon as kv_tensors_created() flips). A manager configured beyond the clamped capacity raises the same ValueError naming both numbers; nothing is silently shrunk. The per-group record now also carries the num_kv_buffers the FTensors were created with, and both orders require the manager's num_layers and num_kv_buffers to match it: in the contiguous layout the FTensor and the PageAllocator both stride compound pages by page_size * num_layers * num_kv_buffers, so a mismatch maps pages at the wrong offsets even when the block count fits. The ValueError names the mismatched values. Tests: manager-first then clamped alloc, num_layers and num_kv_buffers mismatches in both orders, happy path in both orders, per-group and weak-reference registry semantics (12 new, 24 total). Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
519c763 to
281ec39
Compare
shipiyouniao
left a comment
There was a problem hiding this comment.
The manager-first check and the layer/buffer geometry checks address rob-9's findings. I did not find another blocking issue in this pass.
Validation:
- All 24 capacity-guard tests passed on head
281ec39. - I also tested the clean merge with main
73fa57con one NVIDIA T4, with both contiguous and non-contiguous layouts. The native sources were unchanged from the already-built main backend. - Requesting 131,072 blocks over tensors with capacity 29,824 was rejected before manager construction; incorrect layer/buffer counts were also rejected.
num_blocks=Nonederived 29,824 correctly. - Each layout completed 400 allocation/free cycles with CUDA readback. Each also hit 10 injected native physical-allocation failures and recovered on all 10 retries, then shut down cleanly. These are single-device allocator tests, not TP/MPS serving tests.
One non-blocking description update: #418 has merged, and the test count is now 24 rather than 12. Please refresh the old dependency/validation paragraphs so readers do not think this still waits on an open #418 or that the teardown behavior was verified by that older run.
LGTM for this Python-side guard. @RixinLiu, could you take the final look and handle the merge?
Fixes #437. Validate
get_kv_cache_manager()'snum_blocksagainst the capacity actually reserved byalloc_kv_cache(), raising a ValueError that names both numbers instead of aborting the process later insideFTensor::map.num_blocks=Nonenow derives the created capacity.Credit to @rob-9 for the diagnosis on #437:
alloc_kv_cache()warned and clamped the created tensors (65,536 requested -> 45,056 on his L4) whileget_kv_cache_manager()was configured independently with the original 65,536, so the manager exposed 512 page ids over an FTensor holding 352 compound pages, and the first map past the reservation failedcuMemUnmapand hitCHECK_GPU'sstd::abort. His control run with the manager at 45,056 completed all 400 churn iterations.Changes
alloc_kv_cache()records pergroup_idthe created capacity;get_kv_cache_manager()refusesnum_blocksbeyond it (ValueError naming both numbers) and derives it fornum_blocks=None. The per-layer capacity is computed in the caller's block geometry (ftensor_bytes_per_layer // num_kv_buffers // (block_size * cell_size)), consistent for MHA, MLA, and HYBRID_LINEAR units.alloc_kv_cacheruns in the worker), an explicitnum_blocksis trusted unchanged, so the integration flow is unaffected.tests/test_vllm_ftensor_capacity_guard.py), including the SIGABRT in FTensor::map() — mem_unmap fails with "invalid argument" under repeated large alloc/free churn #437 reproducer geometry and MLA single-buffer units. The stub fixture restores the kvcached module graph on teardown; the earlier revision that didn't broke later ElasticBlockPool tests on the CI 3.10 job when the file ran early in the manifest.Scope: no csrc changes, deliberately
The C++ half of #437 (bounds-check page ids in
FTensor::map/unmap, stop aborting) is left to open PR #418, which already addsFTensor::validate_offset_()on those paths and converts theCHECK_GPUaborts intostd::runtime_error. Duplicating that here would conflict with #418/#455/#465, which all touch the same csrc region. This PR is the Python-side guard that stops the invalid configuration before any page id is handed out; it composes with #418.Re-checked 2026-08-27 with
git merge-tree --write-treeagainst the current heads: merges clean with #455 and #465; with #418 the only conflict is both PRs appending test filenames totests/manifests/cpu.txt(trivial union; #418'sinterfaces.pyhunks are confined toinit_kvcached()and don't overlap this PR's).Validation
GPU (this PR's target failure), 1x NVIDIA L40S (driver 570.124.06), kvcached editable install, torch 2.11.0+cu130: reproducer from #437 scaled to this GPU's capacity (46GB creates 90,880 blocks for the issue's 16-layer fp16 geometry, so the issue's 65,536 doesn't clamp here; 131,072 keeps the same structure of one number passed to both calls).
num_blocks=None: derives 90,880 and all 400 churn iterations complete (COMPLETED WITHOUT ABORT).CPU: 12 new tests pass; full CPU manifest 188 passed under a CI-matching env (3.10, CPU torch, pytest 8.4).
tools/check_test_classification.pyvalid; ruff/isort clean;mypy --python-version 3.10introduces no new errors vs main.One unrelated observation from the same runs: at interpreter exit every configuration, including main with a valid
num_blocks=90880control, core-dumps in FTensor/Page static destructors withunknown CUDA driver error (4)(driver already deinitialized; the standalone reproducer never callsshutdown_kvcached()). Pre-existing teardown behavior, orthogonal to this fix; #418's abort-to-exception conversion likely covers it.