Repository navigation
[Bugfix][ROCm] Size the extensible KV cache from exact VRAM use, not 2 MB-rounded free memory - #63401
GodRishUniverse wants to merge 5 commits into
Conversation
|
This pull request has merge conflicts that must be resolved before it can be |
c67e642 to
12e41f4
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
Documentation preview: https://vllm--63401.org.readthedocs.build/en/63401/ |
|
This pull request has merge conflicts that must be resolved before it can be |
9530ade to
a686f3c
Compare
njhill
left a comment
There was a problem hiding this comment.
The approach and math look sound: the correction is capped at what's physically free, the erase and read-order races I traced at the final reading only shrink it, and non-ROCm platforms return early. This review is from reading the code only; I haven't run it. Main requests, with details inline:
- Move the ROCm logic out of common files. 129 of the 254 source lines are in common files but only do anything on ROCm.
- Fix the race at the final reading, then report results across several starts.
- Widen validation beyond R9700, TP1 and one model.
- Rework the tests to follow
tests/AGENTS.md.
Other points:
- Multi-instance limitation: the description says a later instance's start-up check uses rounded free memory, but
request_memoryskips that check when the extensible cache is on. From reading the code, the first instance's rounding is already in the second's baseline and never gets added back, so the second instance silently gets a smaller cache, depending on start order. Please correct the description.--gpu-memory-utilizationnow meaning exact use on ROCm probably deserves a docs note too. - Overlapping PRs: #65413 rewrites the same
get_memory_infolines and adds its owndevice_memory_infocorrection, and #55828 changes thenon_kv_usedcalculation this correction feeds into. Please agree a merge order and check whether the free-memory corrections can share one mechanism, though #65413 corrects every reading and this PR only the final one. - PR description: please use the template sections (Overview, Claims, Validation, Details) and give the run count and variability for the benchmark table.
| gc.collect() | ||
| torch.accelerator.empty_cache() | ||
| free_memory, _ = torch.accelerator.get_memory_info(device) | ||
| free_memory, total_memory = torch.accelerator.get_memory_info(device) |
There was a problem hiding this comment.
The baseline samples for 200 ms to wait out erases, but this reading comes straight after empty_cache(), just as warmup's cached memory is freed. Memory still being erased counts as used, so the correction shrinks and can reach 0, silently falling back to main's size. _min_across_ranks then applies the unluckiest rank's result to every rank.
Could both readings share one sampling helper? If that lives behind the platform hook, free memory needs re-reading after sampling, so the hook may be better off returning the corrected value. Please also report KV cache size across several starts, including multi-GPU.
There was a problem hiding this comment.
I am running tests on R9700 TP4 right now, I don't have access to MI355x or MI300 right now @njhill to test it on CDNA.
|
|
||
|
|
||
| @pytest.mark.optional | ||
| def test_corrected_free_memory_can_be_allocated(exact_count, code_objects): |
There was a problem hiding this comment.
Two validation gaps:
- Single allocation: this only allocates 64 MB and 2 MB chunks.
defragment=True(registers_kv_cache_memory) remaps each segment as one driver allocation, which is the case kernel commit 1ac354beecfd's rounding is meant to help. Could you cover it? - CDNA and multi-GPU: only R9700 TP1 is tested so far, and [Bugfix][ROCm] Raise the extensible KV cache margin floor to 3 GiB #65418 just fixed OOMs on MI355 spec decode. This removes about 3.4 GB of unintended slack wherever it activates. The 3 GiB floor should still cover that since KFD checks exact sizes, but it hasn't been shown on CDNA. Could you do an unpartitioned MI300 or MI355 run with spec decode? Otherwise, consider limiting the correction to validated GPUs.
Also, the description says AMD CI falls back because it's partitioned. The worker tests do use mi355_dpx, but .buildkite/test-amd.yaml also has mi355_1 and mi300_1 pools. If those are unpartitioned, this path runs in CI on hardware that hasn't been tested. Do you know which mode they use?
There was a problem hiding this comment.
@njhill I don't have access to MI300 or MI355 at the moment. If you have access, then I'd appreciate if you can run it. I'll cc @dllehr-amd just in case, if he is able to run on MI.
I have not run CI
Also, the description says AMD CI falls back because it's partitioned.
This is my agent's assumption
| child.stdin.close() | ||
| child.wait(timeout=60) | ||
| assert free_alone - free_shared >= 1 << 30 | ||
| assert abs(added_shared - added_alone) < 32 * MB |
There was a problem hiding this comment.
Possible flake: the child process's own rounding also counts toward the correction, so 32 MB may be tight.
There was a problem hiding this comment.
Made the upper limit as 256 MB, does that work @njhill ?
|
This pull request has merge conflicts that must be resolved before it can be |
|
Re the merge order with #55828 (mine): the two PRs overlap in the diff but not at runtime.
So either order works for me, and whichever lands second resolves a small conflict. #55828 is already rebased onto #65413. On sharing one mechanism: #55828 isn't a free-memory correction. It only changes which memory counts as this process's own use and leaves the free-memory reading alone, so I don't think it has anything to share with this PR or #65413. |
|
Converting to draft while I rebase and move the ROCm logic behind platform hooks. Will re-request review when ready. |
Signed-off-by: GodRishUniverse <risagarw@amd.com>
Signed-off-by: GodRishUniverse <risagarw@amd.com>
a686f3c to
295f8a5
Compare
Signed-off-by: GodRishUniverse <risagarw@amd.com>
…ther than vllm takes some memory, then we can cosider that in our calculations Signed-off-by: GodRishUniverse <risagarw@amd.com>
|
@njhill At
Commands: HIP_VISIBLE_DEVICES=0,1,2,3 vllm serve google/gemma-4-31B-it --served-model-name gemma4 \
--tensor-parallel-size 4 --max-model-len 4096 --gpu-memory-utilization 0.97 --port 8000
vllm bench serve --backend openai --endpoint /v1/completions --model gemma4 \
--tokenizer google/gemma-4-31B-it --dataset-name random --random-input-len 512 \
--random-output-len 128 --ignore-eos --request-rate inf --seed 0 \
--max-concurrency 128 --num-prompts 1280 |
I think #65413 got merged. Yeah apologies, I meant that the PR is overlapping in diff but runtime is completely separate. The edits now should reduce overlap now as I changed my edits and scoped it to a new file. |
Overview
On ROCm,
hipMemGetInfocounts every allocation as at least 2 MB, so after warmup the extensible KV cache (#56492) sizes itself from free memory that looks a few GB smaller than it is. This PR adds that memory back once, at the final sizing, using amdgpu's exact VRAM count. Only unpartitioned discrete ROCm GPUs are affected.Root cause. The amdgpu driver rounds each allocation up to 2 MB in the free memory it reports, on purpose (commit 1ac354beecfd, "drm/amdgpu: Pessimistic availability based on rounded up allocations"), unless the GPU's memory is kept in system memory (
apu_prefer_gtt). Its own allocation check uses exact sizes. The many small kernel code objects loaded during warmup therefore hide a few GB. The sysfs countermem_info_vram_usedcounts exact sizes for the whole device.Claims
Validation
Unit tests (the sysfs lookup and correction arithmetic need no GPU; the GPU tests skip off ROCm):
On a ROCm GPU (Radeon AI PRO R9700, gfx1201).
--optionaladds tests that allocate all but 256 MB of the corrected free memory, both as 64 MB / 2 MB torch blocks and as oneExtensibleTensorchunk (thedefragment=Truecase):Serving comparison, main vs this PR, with a fresh compile cache for each run:
vllm serve meta-llama/Llama-3.1-8B-Instruct --max-model-len 8192 --gpu-memory-utilization 0.97 vllm bench serve --backend openai --endpoint /v1/completions --model meta-llama/Llama-3.1-8B-Instruct \ --dataset-name random --random-input-len 512 --random-output-len 128 --ignore-eos \ --request-rate inf --seed 0 --max-concurrency 128 --num-prompts 1280 # repeated with --max-concurrency 256 --num-prompts 2560Llama 3.1 8B, TP1, Radeon AI PRO R9700 (32 GB),
--gpu-memory-utilization 0.97. One run per config, so there is no variability figure:Not yet done:
Limitations.
gpu_memory_utilizationnow bounds exact use on unpartitioned discrete ROCm GPUs.request_memoryskips the startup check. An earlier instance's rounding is already in a later instance's baseline and is never added back, so a later instance gets a smaller cache, depending on start order. This doesn't cause OOM.tests/v1/workerruns onmi355_dpx(partitioned, so it falls back), but other jobs run onmi300_1/mi355_1. I don't know whether those pools are partitioned. If they aren't, this path runs there untested, and the correction could be limited to validated GPUs.--no-enable-extensible-kv-cacheare unchanged.Not a duplicate. I found no other open PR correcting ROCm's 2 MB-rounded free memory. #65413 (merged) corrects UMA free memory and is the fallback inside the new hook. #55828 (open) changes which memory counts as this process's own and only takes effect on NVML. No runtime overlap and either merge order works.
Related: #56492 (extensible KV cache), #65413 (UMA free-memory correction), #65418 (ROCm 3 GiB margin floor), #55828 (process-scoped KV sizing), #60121 (asynchronous VRAM release on ROCm).
Note: From my understanding,
amdsmi_get_gpu_memory_usagereads the samemem_info_vram_usedsoamdsmiandsysfsshould be the same but if it can't be read then it will ask KFD and will again use 2MB rounded values that hipMemGetInfo reports.Pull Request Checklist
[] I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
AI assistance was used. I have reviewed the changes.