Conversation
|
@ggerganov this should enable proper host memory usage on UMA devices other than Metal (i.e. ones that actually pin host memory), once this is verified the |
|
As suggested by @am17an I've asked Claude to add a post-mortem doc about my chases of the various bugs during this PR here: https://pwilkin.github.io/llama-scheduler/ring.html |
ORippler
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at formalizing this! I'll take a tour on DGX/RTX Spark later on and report back on perf
| 2. **Op support and operand location.** Otherwise the highest priority backend that supports the | ||
| op is used, preferring the backend holding the operands. Ops reading tensors in a buffer marked | ||
| `GGML_BACKEND_BUFFER_USAGE_WEIGHTS` prefer that buffer's backend, so that weights are not | ||
| copied. |
There was a problem hiding this comment.
How are these priorities determined?
There was a problem hiding this comment.
Augmented the docs to include the entire algorithm.
| says the memory is dead - so a later split on the owning backend can be given the same memory and | ||
| overwrite it. | ||
|
|
||
| Such tensors are pinned for the lifetime of the graph with `ggml_gallocr_pin_tensor()`, keeping |
There was a problem hiding this comment.
For my understanding. We are talking about pinning the memory on backend A for a node later on consumed by backend B. This will hold also for the path A -> C -> A -> B (which is why ggml_set_output is insufficient, as it garuantuees only within single cgraph execution).
| That move is only made when the target backend supports the op. Support can be conditional on | ||
| the tensor types - CUDA runs `GGML_OP_SET` only for F32 and I32 - so the backend owning the | ||
| aliased memory is not guaranteed to be able to run the op writing into it. There is no correct | ||
| placement in that case: the scheduler copies operands into a split, never results out of one, so | ||
| whichever backend runs the op, the write cannot reach the aliased memory. The node is left where | ||
| the earlier passes put it, which is what the scheduler did before this rule existed, and the | ||
| reason is logged under `GGML_SCHED_DEBUG`. |
There was a problem hiding this comment.
so the backend owning the
aliased memory is not guaranteed to be able to run the op writing into it. There is no correct
placement in that case: the scheduler copies operands into a split, never results out of one, so
whichever backend runs the op, the write cannot reach the aliased memory.
Wouldn't the correct behavior be to check for this during node-placement + expansion time?
There was a problem hiding this comment.
It might be, but I didn't want to do a full scheduler refactor for this. One is probably due anyway since the work in this PR outlined quite a few issues with the scheduler and quite a few of the current solutions (esp. regarding the "special" input / output tensors) seem really hacky.
| It maintains a vector clock per actor - the host thread and each backend - and a shadow map of | ||
| which actor last read or wrote every byte of every buffer. Synchronization points (backend | ||
| synchronize, event record, event wait, event synchronize, async copies) advance those clocks. When | ||
| an access conflicts with a previous one and no happens-before edge connects them, it reports: |
There was a problem hiding this comment.
Can we make this respect inter-backend events? AFAIK it's not current POR, but CUDA offers cross-stream events like such
CUDA_CHECK(cudaEventRecord(cuda_ctx_src->copy_event, cuda_ctx_src->stream()));
// wait on dst stream for the copy to complete
CUDA_CHECK(cudaStreamWaitEvent(cuda_ctx_dst->stream(), cuda_ctx_src->copy_event, 0));
f046c3e to
faa3fdf
Compare
|
I tested #26225 on this box a few days ago and reported the numbers over there, so here is the same again The box is unchanged from last time, a Framework Desktop with Strix Halo, gfx1151, 96 GB unified memory as For correctness i use the same needle test as before. A marker KANARIE-<8 hex> goes at a defined position For comparison, the 13 of 30 that i reported in #26225 was measured on build 10454, wich is an ancestor of The sanitizer is the part i find most interesting, that one is new compared to my last report. First ggml-sched-sanitize: 0 race(s) reported That was over four cells at 2000 and 4000 tokens, not the full grid, so its four requests and not thirty. Now speed. Prefill in t/s on the same machine and model, 9776 is the last build before #24233 and still
At 104777 tokens thats 193,1 against 196,9 with no fix at all, so 1,9 percent apart while my measurement Decode is unchanged as well, 9,62 t/s at 104777 tokens against 9,61 on unpatched master One thing that is not about your PR at all, but you might want to know since it showed up in the same runs. Anyway from where i sit, this looks good, better then the previous approach because the sanitizer confirms |
b16cc7d to
b3823d8
Compare
|
Linux Strix Halo ROCm validation on the exact current candidate: PASS.
Correctness:
I also ran a controlled server differential on the exact head with Qwen3.8-27B UD-Q5_K_XL (model SHA-256 GGML_SCHED_SANITIZE=1 GGML_SCHED_SANITIZE_NONFATAL=1 \
GGML_SCHED_UMA_RING=<2 or 1> \
llama-server -m model.gguf -ngl 999 -fa on -c 16384 \
-np 4 --kv-unified -b 2048 -ub 2048 -lm noneWorkload: four distinct prompts submitted concurrently to four slots, three rounds (12 completions), temperature 0, fixed seed, prompt cache disabled.
With the ring disabled, the sanitizer reported write-after-read conflicts in ROCm host memory for This gives a hardware-backed toggle differential for the UMA input hazard and the proposed ring-buffer fix on Strix Halo. |
| } | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
What is the benefit of this indentation?
| GGML_ABORT("CUDA graph uid reused after node properties changed - see [TAG_CUDA_GRAPH_UID]"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this addition debug code, or should similar things be rolled out to all other UMA backends?
Also, my suggestion would be to move this specific code to ggml_cuda_graph_verify_uid() itself, for better structure / smaller general functions.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
ie #25863 |
|
@am17an yeah, but that's a performance regression. |
|
@pwilkin i think the first problem here is that there is no consensus on how to fix the underlying issue, so we just need the stop gap for now |
|
All right, so we merge the PR you linked? |
|
might be better to just revert the pr that enabled prop.integrated on hip again. |
|
Huh, or that :) |
|
yes lets revert c7d8722 "AMD engineers [...] very likely know better than me." bold assumption |
Assisted-by: Codex
|
All right, the fix for pipeline parallel is in, I've tested it on both my 5060 and R9700 and there's no regression in graph reuse. |
|
I've tested this branch with |
On gfx1151 the revert made ROCm0 reject ROCm_Host buffers: token_embd GET_ROWS ran on the CPU and all graph inputs were copied host->device per step (measured, Qwen3.8-27B server load: graph splits 1 -> 2, ROCm0 compute buffer 244 -> 500 MiB target, 672 -> 928 MiB MTP ctx, 991 -> 1079 MiB DFlash drafter). Upstream reverted over ggml-org#27311 (async ubatch input writes racing in-place device reads on UMA APUs). halo 33870a2's scheduler UMA input ring addresses that race and is only detected when the device accepts the host buffer type, so restoring the flag also re-enables the ring (default depth 2). Claude-Session: https://claude.ai/code/session_01MkuKwgPNgpAg7EbxKR2iWC
|
I made a special build of this PR (rebased) to make testing easier, you can try with: |
Tested sched-uma-ring build (edf57f7) on 4x Radeon AI PRO R9700 (gfx1201, ROCm 7.13): wikitext PPL identical to master in single GPU and 4 GPU pipeline parallel (Qwen3-0.6B 17.7914, Qwen3.8-27B 6.3354), sanitizer level 2 reports 0 races, pp/tg throughput unchanged. |
|
Forgot to revert the revert #28604.. 😞 |
|
It doesn't matter if we just have to rerun the test; it's scripted:) |
|
What is the intent for this PR going forward? I am not at all familiar with what is being done here and even just getting up to speed would mean a significant time investment on my end. Will this PR be broken up into smaller chunks that are easier to review? |
|
@JohannesGaessler this is already split as much as it can be, since it contains #26167 + necessary changes to the sanitizer to cover all spots and the rest is the required logic. The shortest description of the problem is in #27311 (comment), this is basically trying to fix |
|
I previously did some work on the scheduler in #25319 before I went on break. Should that PR in your opinion also be merged into master and if yes in what order relative to this one? |
|
This PR is not ready for review. Supporting |
|
@JohannesGaessler they are unrelated, in that your work deals with making copies faster, while this PR addresses the case in which there is no copy made so the actual memory is being reused. @ggerganov but why? I mean, I discussed this with @ORippler pretty extensively (as well as the other guys from NVIDIA), I think I added a pretty detailed explanation of why this is needed and how it works. If @JohannesGaessler can't review this, then who is that "more knowledgeable person"? |
|
@pwilkin It's low priority because I don't think the impact is significant and warrants such a fundamental change. I didn't say @JohannesGaessler cannot review this. I meant that I would prefer someone who is well-versed in low-level ggml and has demonstrated consistently that they are familiar with the inner workings of the code and how it should be modified/extended. Atm, I don't have this confidence for this PR. If you want to make it work, start with some smaller contributions, improve the existing logic, refactor, improve the CI, etc. Adding a bunch of interfaces to ggml out of nowhere and changing how all user code should work is very difficult to accept. |
|
All right, I can understand the "trust in me that I know what I'm doing / required changes / performance gains" ratio doesn't quite add up :) I'll park it for now then and try to do some simpler stuff for the time being. |
|
can we at least merge the sanitizer PR which catches these issues? It can be run as part of CI in different configurations using small models |
|
Yeah, I'll extract my sanitizer enhancements from this PR to rebase on your sanitizer PR. |
Overview
As per discussion in #25863 , implement the ring buffer mechanism for input tensors, on top of @am17an 's sanitizer (#26167), plus additional hardening for the sanitizer and extra scheduler fixes (there was an error with duplicating pinned memory that was a view's base).
Additional information
Makes host buffers viable again, ping @ORippler for feedback / tests on CUDA integrated boxes. No measurable efficiency losses.
Supersedes #25863 , #26167 , #26225
Moved all scheduler documentation to a dedicated doc.
Requirements