Repository navigation
Add thapi_start() and thapi_stop() - #454
thilinarmtb wants to merge 64 commits into
Conversation
8120018 to
7a184b7
Compare
19954ee to
9817061
Compare
|
@TApplencourt : Two tests fail out of the three tests used to check the toggle API. Those two fail |
|
I can show you how to do it tomorrow : ) |
Works for me. |
889b0d5 to
7ae17f3
Compare
c4ebb89 to
547ce95
Compare
|
@thapi-local-reviewer review |
There was a problem hiding this comment.
Defects
1. push_downstream uses a global hostname instead of the per-event hostname
File: utils/thapi_toggle_callbacks.cpp, function push_downstream
auto key = ToggleKey{std::string(hostname_s), vpid_v};hostname_s is a file-scope static that is only written by thapi_start_callback. In a multi-host MPI run (the test toggle_plugin_mpi_np_2 uses mpirun -n 2), processes on different hosts produce events with different hostnames. push_downstream is invoked for every downstream message, but it always looks up the toggle map with the hostname of the last thapi_start it saw, not the hostname of the event currently being filtered.
Trigger: Two MPI ranks on different hosts. Rank 0 on hostA calls thapi_start() (sets hostname_s = "hostA"). A regular API-call event from rank 1 on hostB then arrives at push_downstream; the key is constructed as ("hostA", vpid_rank1) instead of ("hostB", vpid_rank1), so the lookup either misses (event incorrectly dropped) or hits the wrong entry (event incorrectly passed).
The fix is to extract the hostname from the current event/stream context inside push_downstream rather than relying on the global.
2. No null-check on the vpid field before dereferencing
File: utils/thapi_toggle_callbacks.cpp, function push_downstream
const bt_field *ccf = bt_event_borrow_common_context_field_const(event);
const bt_field *vpid = bt_field_structure_borrow_member_field_by_name_const(ccf, "vpid");
uint64_t vpid_v = bt_field_integer_signed_get_value(vpid);If the event's common context is absent (ccf == NULL) or does not contain a member named "vpid" (e.g., a stream whose context layout differs, or a malformed/legacy trace), vpid will be NULL and bt_field_integer_signed_get_value(NULL) is undefined behaviour (in practice a segfault in babeltrace2).
Trigger: Any event that flows through the filter whose common-context structure does not include a vpid member. The YAML in btx_thapi_toggle.yaml declares vpid in the context, but the filter sits after the muxer and sees events from all backends; a backend whose stream class omits vpid from its common context will hit this path.
3. Toggle granularity is per-process, not per-thread, despite the stated intent
File: utils/thapi_toggle_callbacks.cpp (all three callbacks and push_downstream)
The PR description says:
This API only affects the trace collection of the calling process/thread.
But every key is (hostname, vpid) — the virtual process id. vtid is received by the callbacks but never used in the key. If two threads in the same process call thapi_start() / thapi_stop() concurrently (or in sequence), the second thread's thapi_stop() will turn off tracing for the first thread as well, because they share the same vpid key.
Trigger: Thread A calls thapi_start(), thread B calls thapi_stop(). Thread A's subsequent API calls are silently dropped because the shared (hostname, vpid) entry is now false.
If per-process granularity is the intended design, the description should be corrected; if per-thread is intended, the key must include vtid.
Reviewed 2e63a99e — local model qwen3.8:27b. Verify before acting.
|
sorry @thilinarmtb just playing with some bot :p |
* Fix a logic error in thapi_auto_stop callback * Print error messages to stderr
push_downstream keyed the toggle map on a file-scope hostname that was only updated by thapi_start_callback, so on multi-host runs events were looked up under the hostname of the last thapi_start seen rather than their own. Read the hostname from the event's trace environment instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The toggle filter sees every event after the muxer. An event whose common context is absent or has no vpid member (or whose trace has no hostname entry) made push_downstream dereference a NULL field. Pass such events through untouched since they cannot be attributed to a process. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The toggle map was keyed on (hostname, vpid), so a thapi_stop() in one thread also stopped tracing for every other thread of the process, contrary to the API's stated per-thread scope. Add vtid to the key in all toggle callbacks and in push_downstream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Thilina Ratnayaka <thilinarmtb@gmail.com>
Replace count_base, count_traces, count_processors and count_calls with iprof_toggle_mpi, which runs toggle_mpi under iprof, and two filters over the tally on stdin: count_calls (one function's calls, or Total) and count_processes. count_calls now matches the function name exactly instead 8BD0 of as a regex. Also some general cleanup of the toggle tests: use $MPIRUN instead of mpirun, and set LD_LIBRARY_PATH only for the iprof run of toggle_api_dlopen, the one test that needs it, instead of exporting it for the whole file in setup_file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Thilina Ratnayaka <thilinarmtb@gmail.com>
Rename toggle.c to toggle_api.c, and the binary it builds, after the toggle_api test that uses it, like the other toggle test programs. Rename the tests to match their programs (toggle_dlopen, toggle_mpi_np_1/2, toggle_multi_host, toggle_threads) and drop "plugin" from the remaining names (toggle_events_without_vpid). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Thilina Ratnayaka <thilinarmtb@gmail.com>
Rebasing onto Blob v2 (argonne-lcf#543) kept both this branch's `babeltrace2 >= 2.0` / `lttng-ust >= 2.12.8` checks and devel's `babeltrace2 >= 2.1` / `lttng-ust >= 2.16` ones. The later checks silently won, so drop the older ones. lttng-ust 2.16 already includes the 2.12.8 fix that __attribute__((constructor)) needs, so move that comment to the remaining lttng-ust check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Metababel drops every event that has a registered callback after running it, so the lttng_ust_toggle start, stop and auto_stop events never reach push_downstream and are never forwarded: start and stop are treated alike. Check that the raw trace has start and stop events, and that with --toggle-on the MPI calls are kept but no toggle event is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
push_downstream read the toggle state with std::map::operator[], which adds a false entry for every thread whose events it sees before any toggle event of its own. Use find() instead, treating a missing entry as not traced, so the map only holds threads with toggle events. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thapi_auto_stop runs right after lttng-ust's constructors, with priority LTTNG_UST_CONSTRUCTOR_PRIO + 1. lttng-ust defines it as 150, but it can be overridden, and constructor priorities must be at most 65535. Fail with a clear message instead of the compiler's if it is too large. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Variant 0 deliberately falls through to variant 1, so that every rank turns tracing on before the MPI calls. Say so, and what each variant traces, so the fall-through does not look like a missing break. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
54eea07 to
10ddde7
Compare
I created
lttng_ust_toggleevent category and implementedthapi_start()andthapi_stop()user APIs (in a similar spirit to
cudaProfilerStart()etc.). The library which the user has to linkagainst is named
libThapi.so.Usage of the API is as follows:
If
libThapi.sois linked,thapi_auto_stop()(which is not part of the user API) is called automaticallyduring the library load time using
__attribute__((constructor)). I added a check inconfigure.actocheck if the compiler supports
__attribute__((constructor)).If the user runs
iprofwith--toggle-on, the traces generated bythapi_auto_stop(),thapi_start()and
thapi_stop()will be respected bybabeltrace(otherwise there is no change toiprof/babeltracebehavior). Since
thapi_auto_stop()is called automatically,babeltracewill filter out all of the tracestill it see a
thapi_start(). Trace filtering out will start again once the user callthapi_stop(). This APIonly affects the trace collection of the calling process/thread.
P.S: This feature requires
lttng-ust >= 2.12.8. Specifically, this fix.TODO
Respectlttng_ust_profiler_*lttng_ust_toggle_*events when calculating statisticsfilter.toggle.toggletobabeltracegraph