Repository navigation
@FIR-2216 - llama.cpp: expose ggml perf-summary symbols across ollama's split-binary backend boundary - #158
Conversation
…it-binary build ollama statically links llama-context.cpp into its Go binary while the ggml core (ggml.c, where ggml_perf_accumulate is defined) is built into a separately dlopen'd backend shared library, so the symbol was link-time unreachable there (undefined reference). Add a generic, backend-agnostic dynamic-lookup path (gated by -DOLLAMA) that resolves it at runtime through ggml_backend_reg_get_proc_address against whichever backend registry exports it; native llama.cpp builds keep the original direct call unchanged. Also adds the llama_backend_log_profile entry point and the ggml_backend_i.profile hook plumbing (ggml_backend_log_profile_info, graph_reserve/buffer_size/reset/profile struct fields) that ggml-tsavorite.cpp's existing #ifdef OLLAMA backend struct literal already references but that ggml-backend-impl.h/ggml-backend.cpp did not yet define. Verified: incremental posix debug build (SDK 0.4.24) compiles and links cleanly with no new warnings/errors.
tsi_init_per_txe_state_once() lazily allocates device_free[] and the per-TXE packed_args (and scalar_*_args) buffers on first use, guarded only by unsynchronized check-then-act reads (if (!device_free), if (packed_args.size() != num_of_txes)). It's called unconditionally at the top of every op-dispatch entry point (add/mult/rms_norm/triton add/matmul), so when multi_thread_enable=true lets independent worker threads dispatch different ops concurrently, two threads can race into the first-use allocation path simultaneously and both mutate the same std::vector (packed_args.assign(...)) at once -- undefined behavior on a non-atomic container that can corrupt its backing storage. This reproduced as an intermittent SIGSEGV inside the closed-source libTsavRTShimCAPI.so's tsi_shmem_handle_from_ptr(), called from the Triton MAT_MUL dispatch path with a corrupted/dangling packed_args pointer -- consistent with the allocation race rather than a bug in the matmul dispatch logic itself. Serialize the whole lazy-init body behind a dedicated tsi_init_mutex so the check-then-act sequence is atomic across concurrent callers.
call_triton_matmul_full_packed() populates a set of static (process-wide, reused-across-calls) MemRefDescriptor buffers before handing them to the tsi_pack_mutex-protected dispatch call -- the population itself happens with no synchronization. Add a dedicated mutex around the whole populate+dispatch sequence so no two callers can interleave writes to these shared buffers. Note: analysis of the current single-threaded per-node call site suggests this specific function is not the source of the intermittent MAT_MUL SIGSEGV under multi_thread_enable=true (that crash still reproduces after this change) -- but the lack of synchronization here was a real latent bug independent of that, and is fixed regardless.
…lama Same cross-boundary issue as ggml_perf_accumulate: these three are also defined in ggml.c and were only ever exercised in ollama's build under GGML_PERF_RELEASE, which never calls them -- so the gap stayed hidden until testing the GGML_PERF_DETAIL path (the Target/TSI_KERNEL-RUN columns), which needs all three and immediately hit undefined references at link time. Factor the OLLAMA-gated dynamic backend lookup used by llama_perf_ggml_accumulate into a shared llama_resolve_ggml_proc_address() helper and add matching wrappers: llama_perf_ggml_log_open, llama_perf_ggml_write_detailed_csv, llama_perf_ggml_backend_type. Verified: native posix debug build compiles and links cleanly.
PR Summary by QodoExpose ggml perf/profile symbols for Ollama split-binary builds (llama.cpp + Tsavorite)
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Code Review by Qodo
1.
|
- src/llama.cpp: llama_backend_log_profile() was creating a fresh TSAVORITE backend instance (and runtime context) via ggml_backend_init_by_name() on every call and never freeing it -- a real leak, and worse, ggml_tsavorite_init() also resets the device's op-run stats and re-acquires the device on each call. llama_backend_free()'s matching ggml_backend_init_by_name()+ ggml_backend_free() was the same create-then-immediately-discard anti-pattern. Cache a single lazily-created instance shared by both, freed exactly once in llama_backend_free(). - include/llama.h: fix llama_backend_log_profile()'s doc comment -- "currently only used for MPI" was copy-pasted from the adjacent llama_backend_free() comment and doesn't apply to this function. - src/llama-context.cpp: the four OLLAMA dynamic-lookup wrappers cached a null resolution result permanently in a function-local static, so a lookup attempted before a backend registers its proc address would disable that functionality for the process lifetime. Retry instead of caching null. Also wraps a line that exceeded 120 columns. - ggml/src/ggml-tsavorite/ggml-tsavorite.cpp: tsi_init_per_txe_state_once() took its new mutex unconditionally on every call, even long after initialization completes, despite running on every op dispatch under multi_thread_enable=true. Add a fast atomic pre-check so the mutex is only touched during the actual first-use initialization window. Not changed (reasoning given directly on the PR): - ggml_backend_i interface extension (graph_reserve/buffer_size/reset/ profile fields): the ABI-mismatch scenario raised (an older-ABI backend .so loaded alongside a newer one) doesn't apply here -- every backend in both ollama's and llama.cpp's build is compiled from the same synced source tree in one CMake invocation, so there's no scenario where two backends disagree on struct layout in practice. - call_triton_matmul_full_packed()'s new mutex "serializing matmul globally": traced its one call site (ggml_tsavorite_run_tmu_mul_mat, called from the single sequential per-node dispatch loop) -- it is never invoked concurrently with itself, so the mutex is uncontended in practice; the suggested thread_local/per-device refactor is legitimate future work but out of scope for this targeted fix. Verified: native llama.cpp posix debug build compiles and links cleanly.
|
Addressed all 5 findings from this summary in aea1a75:
Native llama.cpp posix debug build and ollama posix debug build both verified clean after these changes. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- src/llama-context.cpp: the four OLLAMA perf-symbol caches used a plain static ggml_perf_*_t pointer with an unsynchronized read-then-write -- a genuine data race if decode() ever runs concurrently on more than one thread, even though every racing writer computes the same value. Make each cache a std::atomic<fn_t> with relaxed load/store; still retries on null rather than caching a permanent miss. - ggml/src/ggml-tsavorite/ggml-tsavorite.cpp: tsi_init_per_txe_state_once()'s new fast-path flag was a function-local static that never got reset, but tsi_cleanup(), ggml_tsavorite_free(), and tsi_log_profile_info() all free device_free and expect a later init to reallocate it. Move the flag into the shared runtime state (per_txe_state_initialized) and reset it alongside device_free at all three teardown sites, so a subsequent dispatch after any of those doesn't skip reallocation and dereference a freed pointer. - src/llama.cpp: tsi_log_profile_info() (the .profile hook) doesn't just print stats -- it also finalizes the runtime (tsi_finalize(), blob unload, device_free teardown). The cached TSAVORITE backend introduced in aea1a75 would then get torn down a second time by llama_backend_free()'s ggml_backend_free() call, operating on an already-finalized context. Track whether the profile hook already ran and skip the redundant teardown call if so -- the process is exiting right after llama_backend_free() anyway, so leaking the small backend/ context struct at that point is far safer than a second teardown. Verified: native llama.cpp posix debug build compiles and links cleanly.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…free 2eb4d95 reset per_txe_state_initialized alongside device_free at all three teardown sites (tsi_cleanup, ggml_tsavorite_free, tsi_log_profile_info), but tsi_init_per_txe_state_once() only reallocates packed_args/scalar_*_args when the vector's *size* differs from num_of_txes -- their contents (old pointers into memory invalidated by tsi_finalize()) were left untouched, and size doesn't change across a teardown/reinit cycle. The next dispatch after a reinit would have reused dangling per-TXE buffers. Factor the full reset (all 8 per-TXE vectors + the flag) into a single tsi_reset_per_txe_state_after_teardown() helper, called at all three teardown sites, so a size mismatch is guaranteed on the next init and every buffer gets freshly allocated. Verified: native llama.cpp posix debug build compiles and links cleanly.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Hi Anoop,
Did you test these changes with Ollama ? As the llama-context.cpp and some other changes in the PR may conflict with changes in Ollama.
If you have can you also post a draft PR for Ollama as I am not clear on the Ollama side of the changes.
Thanks
Ashish
|
Ollama-side change is just a commit-id update in Makefile.sync -- no other Ollama code changes, so no conflict. Tested at Ollama (posix, qwen:0.5b, both release and debug table formats), results are in the PR description under Validation. Please take a look. |
Both are only defined in ggml.c under #if defined(GGML_PERF_DETAIL) -- the get_proc_address case added in aacf5ea exposed them unconditionally, leaving a dangling undefined reference whenever a build uses GGML_PERF_RELEASE or plain GGML_PERF instead. Never exercised until now because every prior test build used debug mode (always GGML_PERF_DETAIL); a release-mode build surfaced it immediately as a link failure in libggml-tsavorite.so. ggml_perf_accumulate and ggml_backend_type are unaffected -- both are defined under `#if defined(GGML_PERF) || defined(GGML_PERF_RELEASE) || defined(GGML_PERF_DETAIL)`, so they exist in every perf mode. Verified: native llama.cpp posix release build compiles and links cleanly (previously failed with undefined reference to both symbols).
Summary
Ollama statically links
llama-context.cppinto its Go binary via cgo, while the ggml core (ggml.c) and the Tsavorite backend are built into separatelydlopen'd shared libraries (libggml-base.so,libggml-tsavorite.so). Several symbols the "GGML Perf Summary" table depends on are defined inggml.cand were link-time unreachable fromllama-context.cppin that configuration:ggml_perf_accumulate— undefined reference at link time (table never printed at all).ggml_perf_log_open,ggml_perf_write_detailed_csv,ggml_backend_type— same issue, only surfaced once the detailed (Target/TSI_KERNEL-RUNcolumn) table variant was exercised.Also,
ggml-tsavorite.cpp's existing#ifdef OLLAMAbackend struct literal already references.profile,.graph_reserve,.buffer_size,.reset, butggml-backend-impl.hdidn't yet define thoseggml_backend_ifields, andggml_backend_log_profile_info()(declared inggml-backend.h) had no definition — both needed for that file to even compile under-DOLLAMA.Changes
include/llama.h,src/llama.cpp: addllama_backend_log_profile(); restore theggml_backend_free(TSAVORITE)call inllama_backend_free().ggml/include/ggml-backend.h,ggml/src/ggml-backend.cpp: addggml_backend_log_profile_info().ggml/src/ggml-backend-impl.h: addgraph_reserve,buffer_size,reset,profilefields tostruct ggml_backend_i.ggml/src/ggml-tsavorite/ggml-tsavorite.cpp: exposeggml_perf_accumulate,ggml_perf_log_open,ggml_perf_write_detailed_csv,ggml_backend_typeviaggml_backend_tsavorite_get_proc_address().src/llama-context.cpp: add a sharedllama_resolve_ggml_proc_address()helper plus#ifdef OLLAMA-gated wrappers (llama_perf_ggml_accumulate,llama_perf_ggml_log_open,llama_perf_ggml_write_detailed_csv,llama_perf_ggml_backend_type) that resolve these symbols dynamically at runtime through the backend registry. Native (non-OLLAMA) builds keep the original direct calls, unchanged.Scope is intentionally llama.cpp-only. Ollama picks this up via a separate
Makefile.synccommit-id bump once this merges -- no ollama-side changes in this PR.Also included (found while investigating a separate, still-open
multi_thread_enable:trueintermittent crash in ollama, unrelated to the perf-symbol work above but touching the same file):tsi_init_per_txe_state_once(): unsynchronizedstd::vectormutation (packed_args,device_free) on first use from concurrent callers.call_triton_matmul_full_packed(): its static descriptor buffers were populated with no synchronization before reaching thetsi_pack_mutex-protected dispatch call.These are real, standalone concurrency fixes, but do not fully resolve the intermittent
multi_thread_enable:trueSIGSEGV in the Triton MAT_MUL dispatch path (still under investigation, tracked separately).Validation
Tested in ollama (posix build,
SDK_VERSION=0.4.24), via a temporary local sync pointed at this branch:GGML_PERF_DETAIL) — full detail table prints correctly, matching llama.cpp's owntsi-pkg-build.shdev output format:qwen:0.5b, HTTP 200, no crash.GGML_PERF_RELEASE) — plain customer-safe table prints correctly, no CPU/OPU breakdown:qwen:0.5b, HTTP 200, no crash.multi_thread_enable:false(the known-stable path);multi_thread_enable:trueremains separately tracked as an open, intermittent issue unrelated to this fix.Summary by cubic
Restores GGML perf-summary tables in
ollamasplit-binary builds and adds a backend profile hook. Previouslyllama-context.cppcould not reachggml_perf_*/ggml_backend_typeand printed no tables; now runtime resolution restores detailed and release tables, and exports for detailed CSV are correctly guarded to prevent release build link errors. Addresses FIR-2216.ollama:ggml-tsavorite.cppexposesggml_perf_accumulateandggml_backend_typein all perf modes, and exposesggml_perf_log_open/ggml_perf_write_detailed_csvonly underGGML_PERF_DETAIL;src/llama-context.cppadds OLLAMA-only wrappers that resolve these at runtime with atomic caches and retry after late backend registration; nativellama.cppbuilds keep direct calls.ggml_backend_log_profile_info()and optionalgraph_reserve,buffer_size,reset,profileinstruct ggml_backend_i. Introducesllama_backend_log_profile()and caches a singleTSAVORITEbackend; tracks when the profile hook finalizes the runtime to avoid double teardown inllama_backend_free().ggml-tsavorite: serializes first-use lazy init intsi_init_per_txe_state_once()via an atomic flag plus mutex; serializescall_triton_matmul_full_packed()descriptor populate+dispatch; on all teardown paths, clearspacked_argsand allscalar_*per‑TXE vectors and resets the init flag to force clean reallocation on the next init.ollamawill pick this via Makefile sync.Written for commit bcb7c7e. Summary will update on new commits.