ggml-cuda: make GGML_CUDA_ENABLE_UNIFIED_MEMORY=0 actually disable it, and say so on HIP - #149
ggml-cuda: make GGML_CUDA_ENABLE_UNIFIED_MEMORY=0 actually disable it, and say so on HIP#149danielhanchen wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
One note on how this reaches users, since I got it wrong earlier today on #145. The nightly tree is the upstream tag plus Happy to open the pin as a follow-up, or to take it upstream first if that is preferred, in which case the pin points at the ggml-org PR and retires itself. |
ggml tests getenv(...) != nullptr, so =0 enables managed allocation. The name is spelled as a positive, so people reasonably write =0 to turn it off and get the opposite. GGML_CUDA_DISABLE_FUSION in this file already parses its value for the same reason. Empty and the usual falsy spellings are off; =1, =true, =on and =yes are unchanged. This is no longer a correctness bug. Before the cpy 2D fast path was fixed, =0 left the user corrupted while they believed they had opted out. It now costs throughput and capacity instead: on an integrated GPU managed allocation draws host RAM instead of the device carve-out rather than adding to it, measured at about 15% slower, and a model that loads without the variable can be OOM-killed with it. The HIP warning is rewritten accordingly. The earlier draft warned about corrupted output and linked ggml-org#26148; shipping that alongside the fast path fix would tell users to chase a defect their build does not have. It now states the cost, which is what remains true. Note this changes behaviour for every CUDA and HIP user who currently sets =0, not only AMD: they lose managed allocation, which is the point, but it is not a no-op. Rebased onto an upstream ancestor of both master and the base tag so it can be pinned; a branch cut from fork master drags master's divergence into the pin merge and conflicts on the deleted upstream workflow files.
a78bbb1 to
b65a2dc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b65a2dce12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GGML_LOG_WARN_ONCE("GGML_CUDA_ENABLE_UNIFIED_MEMORY is set, allocating device memory as managed. " | ||
| "On integrated GPUs this draws host RAM instead of the device carve-out, which is " | ||
| "usually slower and can reduce the largest model that will load. Unset the variable " | ||
| "to disable it.\n"); |
There was a problem hiding this comment.
On HIP configurations where cudaMallocManaged returns hipErrorNotSupported, such as the Windows case handled by the fallback below, this warning claims that memory is being allocated as managed and describes managed-memory costs before the allocation is attempted; the code then uses ordinary cudaMalloc instead. Emit this warning only from the err == hipSuccess branch so users do not receive an inapplicable warning immediately followed by the fallback warning.
Useful? React with 👍 / 👎.
| // Most ggml switches test presence only, and for a name like GGML_CUDA_NO_PINNED | ||
| // that is right: the only sensible use is to set it. GGML_CUDA_ENABLE_UNIFIED_MEMORY | ||
| // is spelled as a positive, so people reasonably write =0 to turn it off, and under a | ||
| // presence test that enabled the very path they were trying to leave. GGML_CUDA_DISABLE_FUSION | ||
| // in this file already parses its value for the same reason. |
There was a problem hiding this comment.
Replace this implementation-history narrative and the similar long block before the managed allocation with concise comments that retain only non-obvious invariants. These blocks hard-wrap sentences and document prior drafts, issue history, and obvious environment-variable behavior across many lines, contrary to this repository's explicit requirement that comments usually remain 1-2 lines, avoid excessive commentary, and not be hard-wrapped.
AGENTS.md reference: AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
…les it (#162) ggml tests getenv(...) != nullptr, so =0 enables managed allocation. With #157 pinned this is no longer a correctness bug, but it still costs about 15% throughput and can lower the ceiling: managed draws host RAM instead of the device carve-out rather than adding to it, so a model that loads without the variable can be OOM-killed with it. Verified locally to co-merge onto b10705 with #157 and #158, net +79/-5 across 4 files. Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
1 participant
Overview
Two small changes to
ggml-cuda.cu, both aimed at the same problem: users on AMD APUs are running with unified memory on, often without knowing, and cannot turn it off by the obvious means.1.
=0enabled itBoth read sites test presence:
The name is spelled as a positive, so
=0is the first thing anyone reaches for, and it did the exact opposite of what they asked with no indication. Presence testing is the right call for a name likeGGML_CUDA_NO_PINNED, where setting it is the only sensible use. It is the wrong call here.GGML_CUDA_DISABLE_FUSIONin this same file already parses its value for precisely this reason.Now: empty and the usual falsy spellings are off, anything else is on.
=1,=true,=onand=yesbehave exactly as before, so the only behaviour change is for values that were always meant to mean no.2. Warn once on HIP
Every report I have read of managed allocation misbehaving on an APU came from someone who did not know a front-end had set the variable on their behalf. A single line naming the variable and pointing at ggml-org#26148 is most of the diagnosis, and it costs one
GGML_LOG_WARN_ONCE. HIP only, since that is where the reports are.Scope
This is not a fix for the corruption in ggml-org#26148. It is the thing that has to be true before anyone can test around it: a switch that responds to being switched off, and a build that tells you the switch is on.
Verification
The helper is compile-checked standalone under
-Wall -Wextraand exercised against unset,1,0, empty,true,False,OFF,on,yes,no,2and a junk value, which behave as described above.I cannot build CUDA or HIP where I am, so the prebuilt legs are the first real check on the change in context. Worth a look at whether the warning is too chatty for anyone deliberately running managed memory to fit a large model in a shared pool; I kept it to one line for the process, but I would not argue if a reviewer wants it gated further.