Repository navigation
refactor(rocprofiler-sdk): migrate thread trace off the per-queue callback registry (step 1) - #11973
Conversation
…lback registry Isolates the thread-trace slice of the callback-removal effort (reference PRs ROCm#8730 / ROCm#8586) into a small, single-service change, per review guidance to land callback removal one service at a time. Thread trace no longer registers a queue_callbacks_t with the HSA queue controller. Instead the HSA write interceptor and async signal handler call explicit free functions: - thread_trace::write_hook / signal_completion_hook / is_any_active (new thread_trace/queue_hooks.{hpp,cpp}) - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for counters, pc-sampling and spm; those services are migrated in follow-up PRs. The WriteInterceptor gate and should_batch predicate now detect thread trace via is_any_active() (it no longer contributes to queue.get_notifiers()). Adds thread-trace-queue-hooks-test unit test. Co-authored-by: Cursor <cursoragent@cursor.com>
…ad client global Addresses the two follow-ups @bwelton raised when approving ROCm#8790, which he asked be handled either there or as a fast follow before the next service migration. Neither has been picked up, so they are here as a stacked change. 1. Multi-context ATT ownership. context.cpp's active-context conflict check rejected a second dispatch_counter_collection context but had no equivalent for dispatch_thread_trace. That invariant previously held by accident: the per-queue callback registry registered one global client, so a second concurrent ATT context silently did nothing. With the registry gone the hooks iterate every active ATT context, and DispatchThreadTracer::post_kernel_call identifies packets by dynamic_cast plus agent lookup with no per-context ownership check, decrementing post_move_data before verifying agent ownership -- so two ATT contexts on one agent cross-talk and mis-refcount instead of one being ignored. Now rejected with ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT, mirroring the counter-collection check directly above it. Note this is a behaviour change: starting a second ATT context now fails where it previously succeeded and did nothing. He offered a documenting comment as the minimum acceptable alternative; the guard is implemented because silently doing nothing is the worse of the two contracts, but say so if the comment is preferred for an already-approved PR. 2. Dead code. thread_trace/core.cpp's client global (common::Synchronized<std::optional<int64_t>>) and its comment are unused now that the add_callback/remove_callback call sites are gone -- verified zero references on the branch. Removed so it does not mislead readers of the remaining service migrations into thinking single-client gating still exists. No include changes needed; those headers arrive transitively. Not compiled here -- no ROCm toolchain in this environment. clang-format 11.1.0, the version the repo pins, reports no churn outside these lines. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Apply per-agent scoping from PR ROCm#9868 to the thread-trace callback-removal branch: - Per-agent serialization ref-counting in queue_controller - DispatchThreadTracer configured_agents/collects_on/intersects helpers - Scoped enable/disable serialization in start_context/stop_context - Agent filtering in thread_trace::write_hook - Disjoint dispatch ATT context conflict rules Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Make agents_map_mut mutable for const query methods, rename locals that shadow the agents member, and apply clang-format 11 to queue_controller. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Declaration alignment in configured_agents and intersects. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Dispatch ATT already configures one agent at a time; these tests lock in the new contract that disjoint contexts can run together, overlapping ones still conflict, serialization stays on the configured agents, and write_hook skips the rest. Co-authored-by: Cursor <cursoragent@cursor.com>
- Route ATT completion via registered contexts (in-flight after stop) - Filter post_kernel_call by THREAD_TRACE_CLIENT_ID and agent ownership - Stop ATT services before clearing active slot (match SPM/counters) - Drain queues before disable_serialization on ATT stop - Fix per-agent serialization effective-state transitions - Add in-flight completion regression test for ATT queue hooks Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…id range The inst_pkt_t tag space is shared between the per-queue callback registry and the queue hooks. QueueController::add_callback allocates strictly positive ids counting up from 1, which is exactly the range the hook tags used, so a packet tagged by a not-yet-migrated service could compare equal to a hook tag. This was inert while the tags were only carried and never inspected, but counters, thread trace and SPM now each select their own packets by comparing against their tag, so the overlap is reachable. Move the tags to a negative range, which the registry cannot produce, and add a static_assert so the invariant survives future additions. Also converts the tags from separate constexpr int64_t values to an unscoped enum with a fixed int64_t underlying type, keeping the implicit conversion to hsa::ClientID so call sites need no casts. (cherry picked from commit 38a1dd2)
…serialization The serializer lambda captured now_enabled, bound by a structured binding in the enclosing range-for. Capturing a structured binding is a C++20 extension, so the build failed under C++17 with -Werror: queue_controller.cpp:511:24: error: captured structured bindings are a C++20 extension [clang-diagnostic-c++20-extensions] Read the pair members into plain locals instead. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
These two files were already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
After the callback-registry removal neither method mutates the tracer, so clang-tidy fails the build under -warnings-as-errors: core.cpp:520:23: error: method 'start_context' can be made const [readability-make-member-function-const,-warnings-as-errors] Both now read only configured_agents() and act on the queue controller, so const is accurate. The stale NOLINT for convert-member-functions-to-static goes with it: stop_context calls configured_agents(), so it uses this and that check cannot fire. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
get_active_contexts returns const pointers, so clang-tidy fails the build: queue_hooks.cpp:59:9: error: 'auto *ctx' can be declared as 'const auto *ctx' [readability-qualified-auto,-warnings-as-errors] This also matches signal_completion_hook, which already iterates with const auto*. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
queue_hooks_test.cpp read DispatchThreadTracer::post_move_data directly, but that member is private, so the test did not compile: error: 'post_move_data' is a private member of 'rocprofiler::thread_trace::DispatchThreadTracer' Add a small public accessor alongside get_agents() and use it from the test, rather than widening the member or friending the fixture. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…e_hook unit.ThreadTraceQueueHooks.StopContextInFlightCompletionRoutesViaHookPath failed on 'write_hook must inject ATT control packet': inst_pkt came back empty. thread_trace::initialize() calls resource_init() on the contexts registered at the time the HSA runtime registers. This test creates its context afterwards, so the tracer's agent map was never built and pre_kernel_call returned early without a packet. Call resource_init() explicitly, as att_packet_test.cpp already does. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…ing resource_init With resource_init() wired up, the test crashed instead of failing its assertion: ThreadTracerAgent's constructor does CHECK_NOTNULL on hsa::get_core_table() and hsa::get_amd_ext_table(), and this test's test_init() never populated those globals. att_packet_test.cpp, the one existing test that calls resource_init(), sets them via hsa::copy_table. Do the same here. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…ity jobs Both workflows failed before compiling any rocprofiler-sdk source. Code coverage died installing ROCR-Runtime with 'CMake 3.25 or higher is required. You are running version 3.22.1', and release compatibility died resolving find_dependency(NUMA) from hsakmt-config.cmake on the 'latest' ROCm container. This branch predates the fixes for both on develop, so take develop's versions of the two workflows and of the rocprofiler-sdk-util action they depend on. The action change is additive -- python-venv-path defaults to empty and is guarded -- so the continuous integration workflow, which does not pass it, is unaffected. The pinned CMake is held at 3.25 rather than develop's 4.x. The external submodules on this branch (json, pybind11, sqlite, yaml-cpp) still declare cmake_minimum_required below 3.5, which CMake 4 rejects outright, so 4.x would trade one configure failure for another. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…ility Develop dropped -DROCPROFILER_DISABLE_ATT_QUICK_SCAN=ON because it builds rocprof-trace-decoder, but this branch does not, so rocprofiler-sdk-tool's find_package(rocprof-trace-decoder REQUIRED) fails to configure against every ROCm release container. Restore the flag. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…acks-thread-trace Co-authored-by: Cursor <cursoragent@cursor.com>
start_context()/stop_context() are const on the context interface, so the enabled flag they toggle must be mutable alongside agents_map_mut. Co-authored-by: Cursor <cursoragent@cursor.com>
…1967 Marker commit only; no file changes. It keeps the fork-side thread trace pull request open as a distinct step-1 PR stacked on users/vkale/remove-callbacks-thread-trace--with-cursor-fixes, which already contains every commit on this branch. This replaces ROCm#8790, which GitHub auto-closed as merged when its base branch came to contain its head commit. Merged pull requests cannot be reopened, so the step-1 PR is recreated here. Nothing reached develop. Co-authored-by: Cursor <cursoragent@cursor.com>
0987bf0 to
58c06df
Compare
Why this PR exists, and what happened to #8790This PR replaces #8790, which was closed as merged by accident. I caused that, and this comment records both the mistake and the full review history from #8790 so nothing is lost. Nothing reached
|
| Time (UTC, 2026-09-21) | Event |
|---|---|
| 00:24:25 | Stale bot auto-closed #8790 after 30 days of inactivity |
| 19:45:17 | #8790 reopened |
| 20:04:12 | #8790's base changed from develop to users/vkale/remove-callbacks-thread-trace--with-cursor-fixes |
| 20:05:46 | I pushed a compile fix to that base branch; GitHub saw the base now contained the head commit and auto-closed #8790 as merged |
The push was to fix a genuine build break: the merge of the stacked review fixes had combined develop's const-qualified DispatchThreadTracer::start_context() / stop_context() with the stacked branch's new std::atomic<bool> enabled member, so the tracer mutated a non-mutable member from a const method.
core.cpp:594: error: passing 'const std::atomic<bool>' as 'this' argument discards qualifiers
core.cpp:602: error: passing 'const std::atomic<bool>' as 'this' argument discards qualifiers
The fix marks enabled mutable, matching agents_map_mut directly above it in the same private section. It is commit 0987bf04847 and it is on #11967.
My mistake was pushing to a branch that had become the base of an open PR 94 seconds earlier without checking what depended on it. GitHub auto-closes a pull request the moment its base branch contains its head commit, and merged pull requests cannot be reopened — the API rejects it with 422: the pull request cannot be reopened. Hence this replacement PR rather than a restore.
What is lost is #8790's number and its three approvals. The discussion itself is preserved below and remains readable on #8790.
Carried over from #8790: full review history
Approvals
@ApoKalipse-V (2026-07-20), @bwelton (2026-07-22) and @MythreyaK (2026-07-23) all approved. Those approvals predate a substantial amount of later work, so they should not be treated as current. The code now lives on #11967, which is where fresh approval is needed.
@bwelton's approval comment — two items, both now addressed
1. Multi-context ATT ownership gap. context.cpp's active-context conflict check only rejected a second context when both had dispatch_counter_collection set, with no equivalent for dispatch_thread_trace. This used to be safe by accident, because the old callback registry only ever registered a single global client, so a second concurrent ATT context silently did nothing. Once write_hook / signal_completion_hook iterate all active ATT contexts, and post_kernel_call identifies packets by dynamic_cast<TraceControlAQLPacket*> plus agent lookup with no per-context ownership check — decrementing post_move_data before verifying agent ownership — two ATT contexts on the same agent can cross-talk and mis-refcount. The ask was either an explicit rejection of a second concurrent dispatch_thread_trace context, or at minimum a comment documenting single-context as a hard invariant.
Status: addressed, and more thoroughly than the minimum asked for. context.cpp now carries a real conflict check using per-agent intersection, so two ATT contexts on disjoint agent sets are allowed and overlapping ones are rejected, rather than banning multi-context outright.
2. Dead client global in thread_trace/core.cpp. Status: removed. Only the client_ids.hpp include remains.
@ApoKalipse-V on test coverage — still open
On thread_trace/tests/queue_hooks_test.cpp: "Is this sufficient to cover the changes, as a test?"
The answer at the time was that the true-path behaviour, write_hook / signal_completion_hook when is_any_active() is true, is covered by the rocprofv3 --att integration test, which needs a live HSA queue and dispatch; and that a mock or fake queue would be needed to exercise the true path as a unit test. It was also noted that the test was adapted from @bwelton's original remove-callbacks PR and from #8730, and @bwelton's view was invited.
Status: still open. There is now a thread-trace-per-agent-test covering collects_on / intersects against configured agents, disjoint and overlapping contexts, serialization scoping, and write-hook filtering by agent — so coverage is materially better than when the question was asked. The mock-queue idea for the is_any_active() true path was never implemented.
Copilot, 2026-08-13, on client_ids.hpp — resolved
Flagged that the hook tags overlapped the still-active callback registry, since QueueController::add_callback allocates positive ids starting at 1, so a legacy callback could receive the same tag as thread trace during the staged migration. Suggested a disjoint range such as negative values.
Status: resolved. client_ids.hpp is now enum client_id : int64_t with COUNTERS_CLIENT_ID, THREAD_TRACE_CLIENT_ID, PC_SAMPLING_CLIENT_ID and SPM_CLIENT_ID at negative values, plus a static_assert keeping them negative and therefore disjoint from the registry's positive range. -1 is deliberately left unused because add_callback uses it as an "unassigned" initializer. @SrirakshaNag independently asked for an enum here on #8887, and this is the same change.
Copilot, 2026-09-21 — three findings, all still open
1. queue_controller.cpp:687, serializer creation from a stale snapshot. If creation reads false, then enable_serialization increments the refcount while the agent is still absent from _profiler_serializer, that update has no entry to transition; creation then inserts a disabled serializer from the stale snapshot and no later transition is guaranteed. The inverse race can leave a serializer enabled after disable.
2. queue_controller.cpp:744, refcount update and transition under different locks. An enable can change 0→1 and release the refcount lock, a disable can then change 1→0 and apply its no-op disable first, and when the delayed enable finally runs the serializer ends up enabled even though the final refcount is zero.
Status of 1 and 2: genuinely still open on this branch, and the fix already exists elsewhere in the stack. On this branch serializer() reads the refcount before taking the serializer lock and carries a comment arguing the race is resolved by the next transition reaching the entry once it is in the map; and update_serialization() computes the transition list inside the refcount write lock but applies the transitions after releasing it, under a separate _profiler_serializer.wlock. That is precisely finding 2's objection. The SPM branch on #11968 has the stronger version: it holds the refcount read lock across the whole lookup-and-insertion, taking the locks in the same order update_serialization() does. Porting that change here would close both findings.
3. thread_trace/queue_hooks.cpp, completion routing not tied to the producing tracer. Every active ATT tracer receives every ATT completion. With two disjoint active contexts, completing agent A can make tracer B decrement its post_move_data for A's TraceControlAQLPacket before post_kernel_call discovers that A is absent from B's agent map, so B's own later completion is skipped. It also loses an owner's completion after stop_context removes that owner from the active array before queued work drains, and a newly active context on the same agent can consume the stale packet. The suggestion is to persist a per-context owner with each injected packet and route completion to that registered owner, since the service-wide client tag alone cannot distinguish contexts.
Status: open. This is the same ownership gap @bwelton raised in July, re-derived against the current code. The per-agent conflict check added since then narrows it — two contexts can no longer share an agent — but the completion path still routes by service tag rather than by owning context, so the invariant is enforced at admission rather than in the hook itself.
Summary of what still needs doing on this stack
- Fresh approval on refactor(rocprofiler-sdk): migrate thread trace off the per-queue callback registry #11967 from @ApoKalipse-V, @MythreyaK and @bwelton, whose earlier approvals are stale.
- Port the serializer creation and transition-ordering fix from refactor(rocprofiler-sdk): migrate SPM off the per-queue callback registry (moved PR 8887 to here) #11968 to close Copilot findings 1 and 2.
- Decide whether finding 3 needs per-packet owner provenance, or whether the per-agent admission check is considered sufficient and should be documented as the invariant.
- @bwelton's view on @ApoKalipse-V's original question about unit coverage of the hook true path.
|
Reopening. #8790 cannot be restored: GitHub permanently refuses to reopen a pull request that was closed as merged, and every available route was tried (REST pulls endpoint, REST issues endpoint, changing the base back to |
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
fe5f483 to
168c757
Compare
|
Posting note: drafted with Cursor, but I (@amd-vkale) own this post. Closing in favour of #11967, which carries the same thread trace migration on current |
Step 1 of the thread trace stack. The code is reviewed on the base PR #11967, which targets
developfrom a branch inROCm/rocm-systemsso that CodeQL runs on it.This PR carries only a marker commit with no file changes, so its "Files changed" tab is empty by design. It exists to keep a fork-side step-1 PR in the stack.
It replaces #8790, which GitHub auto-closed as merged when its base branch came to contain its head commit. Merged pull requests cannot be reopened. Nothing was merged into
develop; the base was the topic branchusers/vkale/remove-callbacks-thread-trace--with-cursor-fixes.Review history and the three approvals from #8790 remain on that PR.
Made with Cursor