Repository navigation
hrx: import the weight mapping instead of copying it to the device (engine #856) - #226
Conversation
164d1f8 to
d097c4c
Compare
|
Both points addressed, plus the row you asked for. 1. Branch base — retargeted and rebasedYou are right, and it was the more dangerous of the two. #226 is now based on
2. The mapping — shared, not private, and the comment now says whyYour measurement is the correct one and mine was wrong: it was never "locking a private writable mapping" at import time, it was
Measured on this build (peak of the process's own
The anonymous copy is gone: The
|
30B-A3B -ngl 99 -ot exps=CPU, control |
value |
|---|---|
| peak own GTT | 1509 MiB |
RssFile / RssAnon |
15849 MiB / 186 MiB |
| generation | 19.4 t/s |
The import arm of that row did not finish loading inside a 12-minute hold — it was still in the load spinner when the ticket expired (I killed the child rather than let it run outside a hold). The box was heavily loaded at the time (another lane was running a 400 s GPU job against the same token queue), so I cannot yet say whether this is contention or a real load-time cost of pinning a large model's weights. Either way it is a fact about this change that is not in the PR body yet, and I would rather you see it than have me average it away.
What I did instead, to test the property your point is actually about (CPU-placed tensors must stay file-backed and reclaimable), is the -ot 'ffn_.*=CPU' row above: the CPU-placed tensors stay in the shared mapping (RssFile 2413 vs 2415), and only the device-resident part changes. That is the same mechanism as exps=CPU, on a model that loads in seconds.
I will re-run the 30B row — both arms, with -ot exps=CPU — on a quiet box and post the numbers here before this is READY, and if it turns out that importing a large model costs load time, I will say so and let you decide whether the gate should exclude MoE streaming.
d097c4c to
92ced18
Compare
The
|
| run | load+run | peak own GTT | RssFile / RssAnon |
gen t/s |
|---|---|---|---|---|
control (…HOST_PLACEMENT=0) |
— | 1509 MiB | 15849 / 186 MiB | 19.4 |
| import, before the gate | >14 min, still loading (killed at hold expiry) | — | — | — |
| import, with the gate below | 4 s | 1509 MiB | — | 17.2 |
isolation: -ngl 0 (mapping active, no device weights) |
127 s | 344 MiB | — | 36.0 |
Mechanism, not contention: the from-ptr buffer is created per (ctx, file) over [first, last] of that context's
tensors. With CPU-placed experts interleaved in the same file, that range is the whole 18.6 GB, so the import pins the
entire file to serve ~1.5 GB of device-resident weights, and the load never finishes. -ngl 0 with the same writable
mapping loads the same file in 127 s, so the mapping is not the cost — the pinning is.
Gate (src/llama-model.cpp): the from-ptr path is taken only when each file's region is tight against the context's
own tensor bytes (last - first <= ctx_bytes + ctx_bytes/8 + 1 MiB); otherwise that context falls back to a plain device
buffer. Dense models are unaffected:
4B -ngl 99, gated build |
peak own GTT | gen t/s | text |
|---|---|---|---|
| control | 3927 MiB | 49.4 | — |
| this change | 1245 MiB (−2682) | 53.1 | identical |
So the honest scope is what the PR already says — dense models win, MoE under -ot keeps today's behaviour instead of
regressing. Making the MoE case win as well needs the region restricted to the ranges the device actually imports, which is
a loader-side split rather than a one-line gate; that is the follow-up, not something this PR claims.
One more thing worth your attention, because a validator could have caught it and I did not: the first version of that gate
was written ctx_bytes / 8 + (size_t) 1 << 20, which parses as ((ctx_bytes/8) + 1) << 20 because shift binds looser than
+ — megabytes of slack, so every region would have looked "tight" and the gate would have appeared to work while the MoE
case stayed broken. It is parenthesised now, and the numbers above are from the fixed build.
…ngine ggml-org#856) At -ngl 99 the loader places every offloaded weight in one device buffer, and HRX serves a dense MUL_MAT weight from the repacked resident copy it materialises on first use - so the loader's GGUF-layout copy is a second copy of the same bytes that nothing reads: 5.9 GiB of GTT for a 2.38 GiB model, 2376 MiB of it that buffer. This backend can now take the region itself. device_buffer_from_host_ptr imports the loader's mapping with hrx_allocator_import_buffer and the tensor stays in this backend's own buffer type, so the scheduler keeps the op on the device; the loader's copy is never made. Three requirements the import turned out to have, all measured: * a page-aligned address - the loader hands over mapping + tensor offset, and an unaligned lock fails with a bare HSA_STATUS_ERROR; * a writable region - HSA locks read-write, and a read-only mapping fails the same way; * MAP_SHARED, not MAP_PRIVATE. MAP_POPULATE and mlock fault pages in with FOLL_WRITE, which breaks CoW on a private writable mapping and copies the whole file into anonymous memory at load time: +1024 MiB of RssAnon for a 1 GiB file, against +0 for the same mapping read page by page. A shared writable mapping keeps every page in the page cache, still shared between processes, and HSA locks it without copying. So the weight files are opened read-write and mapped MAP_SHARED|PROT_READ|PROT_WRITE when - and only when - a non-CPU device reports caps.buffer_from_host_ptr, which is this backend alone, and every file of the model passed a write-access check; otherwise the mapping is exactly the shared read-only one it always was. GGML_HRX_WEIGHT_HOST_PLACEMENT=0 turns the capability off and reproduces the previous placement, which is the A/B control. The import is taken only for a region that is tight against the ctx'\''s own tensors (last - first <= ctx_bytes + ctx_bytes/8 + 1 MiB, per file); otherwise that ctx falls back to a plain device buffer. Without it a MoE under -ot exps=CPU imports the whole file - the experts are interleaved in the same mapping, so the range spans all 18.6 GB - and the load does not finish (>14 min against 4 s with the gate, same model, same flags), for ~1.5 GB of device-resident weights.
92ced18 to
360944b
Compare
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 360944b0e on the loader/mmap half, which is the half that can hurt a user — the GTT saving that is the point of the PR needs an HRX build and a device, and I did not spend an hour of this queue's time on it. What I did instead: read every hunk, compiled and linked the whole non-HRX tree at this head, and traced the one regression this change could plausibly cause (read-only model files) to the code that prevents it.
Builds and links here (GGML_HRX=OFF, Release, Ninja, thermal-run): 332/332, llama-cli links. Per-TU -fsyntax-only is also clean on the four src/ files that carry the change (llama-mmap.cpp, llama-model-loader.cpp, llama-model.cpp, llama.cpp). So the new writable parameter plumbed through llama_mmap → llama_model_loader → init_mappings and the "r+b" open are consistent everywhere they are used, in the configuration every non-HRX user has.
The read-only regression is handled, and this is the part I would have blocked on. A shared writable mapping needs the file opened "r+b" and a W_OK on every shard, which is not true of a model on a read-only mount, a root-owned-but-world-readable file, or a signed/read-only medium. src/llama.cpp builds the flag accordingly:
- it probes the capability across both
params.devicesand, when that is NULL (whatllama-cliand the server pass), the global device registry — and the comment records that the first version only walkedparams.devicesand therefore never saw the capability at all, which is the kind of bug that makes a zero-copy patch silently do nothing; - it excludes
GGML_BACKEND_DEVICE_TYPE_CPU, because the CPU backend advertisesbuffer_from_host_ptrtoo and does not want a writable mapping; - it then requires
access(path, W_OK) == 0for every split and the main file before enabling, otherwise falling back to the old read-only mapping.
So a read-only model degrades to exactly today's behaviour, and only when an importing device is present and every file is writable does the new path engage.
The import itself is defensive in the right places (ggml-hrx.cpp): the region is imported as the page-aligned superset with the delta kept (hsa_amd_memory_lock rejects an unaligned address — the comment says that was measured, and the arithmetic aligned = ptr & ~(page-1), import_size = size + delta is right); the buffer context is flagged imported so buffer_free skips the pool release (an imported region was never pool-allocated — releasing it would corrupt the pool's accounting); a failed import logs a WARN and returns nullptr, which is the documented "fall back to a device buffer" path in the loader; and the region is registered in host_buffers so the in-place get/set and direct-binding paths still find it. The capability is advertised through device_i.buffer_from_host_ptr (was nullptr) and gated by GGML_HRX_WEIGHT_HOST_PLACEMENT=0, which is the A/B control.
The MAP_SHARED-not-MAP_PRIVATE reasoning is the sharpest bit of the PR and it is stated with its measurement: MAP_POPULATE/mlock fault with FOLL_WRITE, so a private writable mapping breaks CoW and copies the file into anon memory (+1024 MiB RssAnon for a 1 GiB file, measured), whereas MAP_SHARED|PROT_READ and a shared writable mapping both add nothing. Since the mapping is only made writable when an importing device exists, MAP_SHARED is right for the whole lifetime — a second process mapping the same file still shares the page cache, which the comment says explicitly.
What I did not verify, and it is the PR's headline: the GTT drop itself — "5.9 GiB of GTT for a 2.38 GiB model, 2376 MiB of it that buffer", from the amdgpu_bo_create tracepoint. That needs GGML_HRX=ON, which on this box means the ExternalProject build of hrx + loom + IREE (tens of minutes) and then a GPU window; I judged that not the right spend for one PR at this hour and said so rather than letting a READY imply it. The re-derivation is GGML_HRX=WEIGHT/unset vs GGML_HRX_WEIGHT_HOST_PLACEMENT=0 with the same model at -ngl 99, tracepoint GTT total plus RssAnon — and the =0 arm is in the PR, so the A/B is one env var. If that pair does not move GTT, this verdict is about the code only.
Refs engine#856 and no close: the issue's other causes are not in this diff.
|
Rebased-base A/B is in (
Nothing changed behaviourally across the rebase: GTT is bit-identical to the pre-rebase numbers (−2682 MiB), So the state of ggml-org#1303 is: code-side accepted at your last review, re-pinned to |
At
-ngl 99the loader places every offloaded weight in one device model buffer (src/llama-model.cpp, one buffer perbuft), and this backend serves a dense
MUL_MATweight from the repacked resident copy it materialises on first use. TheGGUF-layout copy is therefore a second copy of the same bytes that nothing reads: 5.9 GiB of GTT for a 2.38 GiB model,
2376 MiB of it that buffer (engine ggml-org#856, measured with the
amdgpu_bo_createtracepoint).This backend can now take the region itself instead.
device_buffer_from_host_ptrimports the loader's mapping withhrx_allocator_import_buffer, and the tensor stays in this backend's own buffer type, so the scheduler keeps the opon the device and no
offload_opis involved; the copy the loader would have made is gone.Two requirements the import turned out to have, both measured rather than assumed:
mapping + tensor_offset; an unaligned lock fails with a bareHSA_STATUS_ERROR. The import covers the page-aligned superset and keeps the aligned address as the buffer base.hsa_amd_memory_lockis read-write only (access_policy.cpassesflags=0), and a read-onlymapping fails the same way — proven with a three-way probe on one 16 MiB region:
PROT_READ → FAIL(2),PROT_READ|PROT_WRITE → OK(0), anonymousrw → OK(0). It must beMAP_SHARED: a privatewritable mapping is CoW-copied by
MAP_POPULATE/mlock, which is exactly the failure this PR had before review. Theweight files are opened
O_RDWRand mappedMAP_SHARED|PROT_READ|PROT_WRITEonly when a non-CPU device reportscaps.buffer_from_host_ptrand every file of the model passes a write-access check — so every other backend, loadmode and non-HRX run maps exactly as before.
GGML_HRX_WEIGHT_HOST_PLACEMENT=0turns the capability off and reproduces the previous placement exactly; that is theA/B control used below.
Measured (strixhalo, gfx1151, no-SVM trio, Qwen3-4B-Q4_K_M,
-ngl 99 -fit off -c 256 -n 32 --temp 0)Peak of the process's own
drm-total-gttfrom/proc/<pid>/fdinfo, plusRssFile/RssAnonfrom/proc/<pid>/status:RssFileRssAnonGGML_HRX_WEIGHT_HOST_PLACEMENT=0)-ot 'ffn_.*=CPU', control-ot 'ffn_.*=CPU', this PRGTT is bit-exact across repeats and the generated text is token-identical at
--temp 0. Decode on a clean box state measured73.8 → 71.5 t/s (−3 %); on a loaded box the arms are within noise of each other in both directions.
The pages stay file-backed and shared. That is the point of
MAP_SHARED:RssFileholds at ~2410 MiB in every arm andRssAnondoes not grow with the model. A private writable mapping would instead have been CoW-copied wholesale byMAP_POPULATE/mlock(which fault pages in withFOLL_WRITE) — measured on a 1 GiB file as +1024 MiB ofRssAnon,against +0 for the same mapping read page by page.
MoE / CPU-placed tensors
Qwen3-Coder-30B-A3B-Instruct-Q4_K_M(18.6 GB) at-ngl 99 -ot exps=CPU: control 1509 MiB GTT, 19.4 t/s. The importinitially never finished loading (>14 min, killed at hold expiry), because the from-ptr region is
[first, last]of thecontext's tensors and the experts are interleaved in the same file — so it pinned all 18.6 GB to serve ~1.5 GB of weights.
With the gate below the same arm loads in 4 s at 1509 MiB (identical to control), i.e. it falls back rather than
regressing.
-ngl 0with the mapping active loads the same file in 127 s, which is what attributes the cost to the pinningrather than to the mapping.
The gate: the from-ptr path is taken only when each file's region is tight against the context's own bytes
(
last - first <= ctx_bytes + ctx_bytes/8 + 1 MiB); otherwise that context uses a plain device buffer. Dense models keepthe win (4B: 3927 → 1245 MiB with identical text). Restricting the import to the ranges a device actually uses, so MoE
wins too, is a loader-side split and is named as the follow-up.
Why not the simpler alternative
Keeping the weight in a host buffer type costs −2582 MiB of GTT and −66 % of decode (63.7 → 21.6 t/s), because a
host-resident weight hands its node to the CPU: the scheduler's escape hatch for that needs
offload_op(
ggml-backend.cpp), which this backend does not implement, and host-buft claiming is deliberately opt-in since ggml-org#1078.Claiming it instead restores the rate but costs +458 MiB over doing nothing. Both measured; the import avoids the whole
question by keeping the buffer this backend's own.
Scope
Dense models. MoE experts are contracted by
MUL_MAT_IDand keep their device copy — a per-tensor split is the follow-upnamed in engine ggml-org#856.