ggml-cuda : fix UMA memory detection for HIP/ROCm on AMD APUs - #20472
ggml-cuda : fix UMA memory detection for HIP/ROCm on AMD APUs#20472hogeheer499-commits wants to merge 3 commits into
Conversation
Bug verification on AMD Ryzen AI MAX+ 395 (gfx1151, 128GB unified memory)Wrote a test program that simulates the exact code path in On AMD APUs, The |
Note on end-to-end testingI was unable to reproduce the However, the mechanism is clearly demonstrated above: On my 128GB system the 91 GiB reported by The fix itself is minimal and clearly correct: |
8674daa to
73357da
Compare
|
Great fix! For Windows users with the same APU, there's a complementary |
|
On my Strix Halo system I get the following on master: ggml_cuda_init: found 1 ROCm devices (Total VRAM: 62206 MiB):
Device 0: Radeon 8060S Graphics, gfx1151 (0x1151), VMM: no, Wave Size: 32, VRAM: 62206 MiB (62202 MiB free)
build: 8323 (57819b8d4) with GNU 15.2.1 for Linux x86_64
llama_params_fit_impl: projected to use 5438 MiB of device memory vs. 121595 MiB of free device memory
llama_params_fit_impl: will leave 116157 >= 1024 MiB of free device memory, no changes needed
llama_params_fit: successfully fit params to free device memory
llama_params_fit: fitting params to free memory took 0.18 seconds
main: printing fitted CLI arguments to stdout...
-c 0 -ngl -1With this PR I get: ggml_cuda_init: found 1 ROCm devices (Total VRAM: 62206 MiB):
Device 0: Radeon 8060S Graphics, gfx1151 (0x1151), VMM: no, Wave Size: 32, VRAM: 62206 MiB (62202 MiB free)
build: 8325 (e0dace50d) with GNU 15.2.1 for Linux x86_64
llama_params_fit_impl: projected to use 5438 MiB of device memory vs. 62060 MiB of free device memory
llama_params_fit_impl: will leave 56621 >= 1024 MiB of free device memory, no changes needed
llama_params_fit: successfully fit params to free device memory
llama_params_fit: fitting params to free memory took 0.18 seconds
main: printing fitted CLI arguments to stdout...
-c 0 -ngl -1So at the very least there are some edge cases that this PR does not handle correctly and it cannot be merged like this. |
|
Thanks for testing! I see the issue — on your system I think the better fix is actually simpler: instead of adding HIP-specific logic, just change the existing UMA path to take the maximum instead of unconditionally overwriting: size_t proc_free = (size_t)available_memory_kb * 1024;
if (proc_free > *free) {
*free = proc_free;
}This way it works for both CUDA and HIP without any
One question: this results in I'll push once you confirm. |
|
According to the llama.cpp AI usage policy:
|
|
Yeah fair point, I used AI to help structure that comment since English isn't my first language, but I should've just written it myself. Won't happen again. The fix itself (max instead of always overwriting) I do understand and stand behind. Want me to push it? |
|
As I said: on my Strix Halo system master correctly reports the full memory wile your PR does not. So even if your PR fixes some cases it breaks other ones and we cannot merge it like this. |
|
Sorry, I think I read your post incorrectly. Yes, push your new version, I'll take a look. |
AMD APUs report prop.integrated=1 which triggers the UMA memory path from ggml-org#17368. This overrides hipMemGetInfo() (accurate) with /proc/meminfo MemAvailable (too low), losing ~30 GiB on a 128GB Strix Halo system. For HIP builds, only enter the UMA path when GGML_CUDA_ENABLE_UNIFIED_MEMORY is explicitly set. This preserves correct behavior for both cases: - Default: hipMemGetInfo() reports accurate TTM-backed memory - GGML_CUDA_ENABLE_UNIFIED_MEMORY=1: /proc/meminfo is used (system RAM mode) Tested on AMD Ryzen AI MAX+ 395, Radeon 8060S (gfx1151), 128GB, ROCm 7.1. Fixes: ggml-org#18159
Co-authored-by: Johannes Gäßler <johannesg@5d6.de>
instead of always overwriting with /proc/meminfo, take whichever is higher. this way systems where hipMemGetInfo already reports TTM-backed memory correctly (like Strix Halo 128GB) keep their value, while systems where /proc/meminfo is higher still get the full amount. removes the HIP-specific #ifdef since the max approach works for both CUDA and HIP.
e0dace5 to
97ae46e
Compare
|
Sorry for pushing but this bug is critical not allowing to use large models. Please make attention to review. |
|
The max() fix has been pushed since March. It handles both configs correctly, your 62 GiB VRAM < 121 GiB meminfo and my 122 GiB > 91 GiB. Ready for review when you get a chance. |
|
I'm not convinced that that is the correct way to fix it. |
|
I am still having this issue on nightly llama-rocm builds as described in lemonade-sdk/llamacpp-rocm#86 |
|
Thanks for pushing back on this. I went back through the numbers, and I think you were right that On my 128GB Strix Halo system I now get: So at least on my machine, Because of that, I do not think taking the larger of I think the narrower change is probably the safer one for now: for HIP builds, do not take the bool uma_env = getenv(GGML_CUDA_ENABLE_UNIFIED_MEMORY) != nullptr;
#if defined(GGML_USE_HIP)
bool is_uma = uma_env;
#else
bool is_uma = prop.integrated > 0 || uma_env;
#endif // defined(GGML_USE_HIP)Before I update the branch again, could you check one thing on your Strix Halo system? cat /sys/module/ttm/parameters/pages_limit
getconf PAGESIZEI want to compare the converted |
1Gb UMA is set in BIOS |
|
Note that there was also a change to ROCm which was recently reverted: ROCm/rocm-systems#5204 I'm not sure whether/how that affects llama.cpp, but it might cause misreporting on nightlies released on or after April 14. I don't think the revert has made it into the latest nightly yet (as the latest bump of |
To be clear: this PR will not fix that issue. All this PR does is change how free and total memory is being reported by the ggml backend but it does not actually allow the ggml backend to allocate and use any additional memory. |
This resolved my issue. |
|
I did more testing on my Strix Halo with ROCm 7.2, because I wanted to understand what is going on before changing the PR again. The main thing I found is that default hipMalloc and hipMallocManaged behave very differently on this APU. For default hipMalloc, I tested touched allocations up to 112000 MiB. I also ran an 80000 MiB hipMalloc inside a Docker container limited to 72 GiB, and it still succeeded. GTT usage went up by about the allocation size, and the allocation was not blocked by the cgroup limit. So on this stack default hipMalloc goes through KFD/GTT, not normal system-memory accounting. hipMallocManaged behaves differently. With the same 72 GiB cgroup, 64000 and 70000 MiB succeed, but 80000 MiB gets OOM-killed. MemAvailable drops by about the allocation size while sysfs GTT usage barely moves. hipMemAdviseSetCoarseGrain does not change that. So managed memory is system/SVM-backed and cgroup-limited here. Master replaces hipMemGetInfo with MemAvailable because prop.integrated is set. That loses about 6 GiB on my system even though the default hipMalloc path can use the HIP/GTT budget. With an 80B Q4 model and --fit-target 70000:
This is also why I do not think the max() version was the right model. It fixes default HIP on my machine, but it reports the HIP/GTT number for the managed path too. I also looked at the Vulkan backend on the same machine. It reports the 8060S with 131584 MiB total / 130528 MiB free because it sums all memory heaps for integrated GPUs. vulkaninfo shows the RADV heaps as about 42.83 GiB + 85.67 GiB. This is not HIP proof, but it is a useful llama.cpp-side sanity check: another backend on the same integrated GPU already treats the memory model differently. The hard part is systems like yours and Kryoz's where hipMemGetInfo reports around 62 GiB. ROCm/hip#3892 tracks this as a reporting issue, and ROCm/rocm-systems#5596 is adding GTT to APU memory reporting when GTT > VRAM. I do not want to pretend this covers every case. The conservative change fixes my system and keeps reporting aligned with the allocator, but it stays at 62 GiB on yours until the ROCm fix lands. A GTT-aware bridge could help sooner, but TTM pages_limit is a capacity limit, not live free. After an 80 GiB hipMalloc, hipMemGetInfo.free drops but TTM stays the same, so you cannot just use TTM as free. The missing piece for that bridge is whether hipMemGetInfo.free tracks allocations correctly on the low-reporting systems. Given that, what scope would you prefer for this PR? I can keep the PR conservative and allocator-aligned, or look into a broader GTT-aware bridge if we can find a reliable live-free source. If you think the underreporting case should wait for the ROCm fix, that is useful to know too. Disclosure: I used AI assistance to organize my notes for this comment. The tests, results, and conclusions are from my local work. |
|
Small note: I have not pushed another change yet. The current branch still has the max() version. After the extra tests above, I think the more conservative HIP change is probably the better model, but I wanted to wait for feedback on the scope before updating it. For a broader GTT-aware fix, I think the missing piece is still whether hipMemGetInfo.free tracks touched hipMalloc allocations correctly on the low-reporting systems. I cannot test that locally because my system already reports the full 120 GiB through hipMemGetInfo |
|
I have also been wondering how to improve UMA memory reporting. It is not straightforward:
So, ideally we want a function which returns accelerator free memory taking into account different memory pools (dedicated, GTT) and system free memory. On my system hipMemGetInfo reports 122GB free, which is indeed the GTT amount I have made available to the GPU. It is incorrect because the system only has 114GB MemAvailable at this time. Also, to be robust I wouldn't want to load an AI model even of size 114GB, it would likely crash the system due to competing with system RAM, so the effective amount available as VRAM is even lower still. This is not a concern with dedicated GPUs. So the memory query functions are not really working as expected, I believe this is also the case on CUDA based UMA as well. |
|
Thanks, this is a good point, and I think I agree with the main concern here. The more I look at it, the less I think this is just a simple choice between On my Strix Halo system I’m seeing the same kind of split: default So I agree that even if That also makes me think the current The narrower version of this PR was trying to fix only one specific issue: default HIP allocations should not automatically fall back to the So I think there are two separate things here:
For the broader version, I think the missing piece is still whether So I do not think the current I realize that this conservative scope still leaves the low-reporting systems at the HIP-reported value. If that is not acceptable for this PR, then I think the right next step is a broader UMA-budgeting design rather than the current |
|
I'll try again to explain what UMA is (but English isn't my native language)... There are several ways to allocate memory with HIP (and CUDA):
In the case of an APU, since RAM is directly accessible to the GPU, there is no copying, and when "correctly" configured (hipMemAdviseSetCoarseGrain), there is almost no performance difference compared to allocation with hipMalloc. Now, the HIP/CUDA backend has two ways to allocate buffers:
Therefore, the reported memory size is currently correct in the backend. Now, GTT/TTM: I'll only cover the case of APUs (it's slightly different with an eGPU):
On recent kernels, allocating VRAM in the BIOS is pointless. For me the best config (the one I use ;) ) is:
On older kernels where it is necessary (for games?) to define VRAM (16/32 GB), UMA only allows the use of a portion of the memory. A VRAM+GTT configuration with allocation without UMA (hipMalloc) is the only way to utilize a large portion of the memory. Now, getting back to llama.cpp and the hip/cuda backend:
The question then becomes: how to force the opposite behavior? At least to force the use of VRAM+GTT on older kernels. I don't know how it works under Windows (I don't know if there's an equivalent to GTT). Before making a choice, it might be possible, if an APU is detected, to check the two memory sizes and add information/log if there is more VRAM + GTT than RAM, and provide a link(?) to recommendations. In my configuration (VRAM=256MB, GTT by default), it's more advantageous to leave the current behavior. However, while this configuration is now the simplest/best, it's difficult to change habits and the now outdated recommendation to define a "large" VRAM size. |
|
I think this explanation is largely correct in terms of the primary semantics. However, as of ROCm 6.4.0 the HIP runtime will fall back to system/shared virtual memory (GTT-backed on APUs) when With recent kernels this fallback has become a lot more performant and is usually faster than |
On unified-memory APUs (AMD Strix Halo / Ryzen AI Max+ 395 / Radeon 8060S/8050S, gfx1151) the GPU's VRAM is carved out of system RAM and the GPU reports its free memory against the full carveout. The scheduler, however, treats host free memory as an upper bound on the GPU budget: GetDeviceInfos, availableMemoryForLoad and availableMemoryForGPU all clamp the GPU free value down to system free. On these APUs the host pool (e.g. 32 GiB, mostly consumed by page cache once a model loads) is far smaller than the GPU carveout (e.g. 96 GiB), so the clamp collapses the reported available VRAM and loading a second model evicts the first even though both fit. Add ml.DeviceInfo.IsUnifiedMemoryAPU() and use it in the three places that clamp GPU memory to host free, returning the GPU's own free memory instead. Detection covers both backends the APU can run on: ROCm (via the structured GFXTarget) and Vulkan (matched on the reported device name, which is the only place the gfx target / SKU is surfaced). Other integrated GPUs keep the existing system-free clamp. This fixes the scheduler's eviction of co-resident models. A separate, load-time under-offload -- where llama-server's -ngl auto fit reads /proc/meminfo MemAvailable instead of the carveout free and splits a single large model to the CPU -- is fixed upstream in ggml-org/llama.cpp#20472 and is out of scope here. Tested on a Radeon 8060S (gfx1151, 96 GiB) with both backends: qwen3.5:35b and qwen3.6:latest (~24 GiB each) stay resident together. On Vulkan the scheduler reports 88.5 GiB available after the first model (full 111 GiB carveout) instead of evicting; on ROCm it reports 73.4 GiB instead of 4.2 GiB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Is this a duplicate of #24906 ? |
AMD APUs report
prop.integrated == 1, which triggers the UMA memory detection from #17368. This replaces the accuratehipMemGetInfo()value withMemAvailablefrom/proc/meminfo, which reports significantly less memory on systems with large TTM allocations (e.g. 122 GiB vs 91 GiB on a 128GB Strix Halo system).For HIP builds, skip the
prop.integratedcheck and only enter the UMA path whenGGML_CUDA_ENABLE_UNIFIED_MEMORYis explicitly set. This wayhipMemGetInfo()is used by default (which correctly reports TTM-backed memory), while the explicit env var override still works for users who need it.Verified on AMD Ryzen AI MAX+ 395 (gfx1151, 128GB unified memory, ROCm 7.1) that
prop.integratedreturns 1 andhipMemGetInfo()returns 122880 MiB whileMemAvailablereports ~91 GiB.Fixes #18159
Related: #19818, #19764, #18650