refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry by amd-vkale · Pull Request #11861 · ROCm/rocm-systems

amd-vkale · 2026-09-18T17:17:27Z

Motivation

Supersedes #8891 (same change, branch rehosted in ROCm/rocm-systems so CI secrets resolve).

Dispatch counter collection currently registers a queue_callbacks_t on every HSA queue. That hides “is this service active?” inside a per-queue map, makes enqueue and completion the same walk, and serializes every GPU while any counter context is active.

This PR:

Stops registering counter collection with that map. The write interceptor and async signal handler call explicit free functions instead.
Scopes collection to GPU agents so a context instruments/serializes only named GPUs, and disjoint agent sets can be active together.

Public dispatch callback/record APIs are unchanged except for rocprofiler_dispatch_counting_service_set_agents. Device counting is untouched. The per-queue callback map remains for unmigrated services; it is not deleted here.

Technical Details

Enqueue and completion use different context sets. The registry invoked the enqueue-time callback even after stop. The hooks reproduce that without a registry:

Enter iterates active contexts — stop adding instrumentation when the context stops.
Exit iterates registered contexts — work already on the GPU must still deliver its record.

Using active contexts on the exit path is silent data loss: completed_cb never runs, and the packet_return_map entry leaks.

flowchart LR
  WriteInterceptor --> EnterHook["enter: active contexts"]
  AsyncSignalHandler --> ExitHook["exit: registered contexts"]
  ContextStop --> Drain["enabled=false, drain, unserialize, stop callback thread, clear slot"]

Write-up: queue_callback_removal_architecture.md. Permalinks pin to 70bef64.

Hooks

queue_hooks.hpp
Enter (active + collects_on + COUNTERS_CLIENT_ID): queue_hooks.cpp:45-84
Exit (registered / provenance): queue_hooks.cpp:86-107
is_any_active: queue_hooks.cpp:109-113
Counters-only still enters WriteInterceptor: queue.cpp:318-328
Enter call site: queue.cpp:633-644
Exit call site: queue.cpp:172-179
Batching forced off while counters active: queue.cpp:768-781
start_context no longer add_callback; callback thread kept: core.cpp:148-172
Producer tags + ClientID overlap warning: client_ids.hpp:33-56
queue_id removed from counter_callback_info: core.hpp

Without is_any_active() in no_real_consumers, a counters-only run would skip the interceptor.

Stop path

Stop services before the active-slot CAS: context.cpp:377-405
enabled=false → drain → disable_serialization(agents) → callback_thread_stop: core.cpp:174-209
Drain: queue_controller.cpp:786-794

Exit delivers completions that arrive; the drain bounds when. Because counters::stop_context runs while the context is still active, queue_cb still returns serialize=true during the drain — no separate draining flag.

Attach-mode: no callback to unregister. enabled=false no-ops new dispatches; is_any_active() drops after the slot clears.

Per-agent scoping

API: dispatch_counting_service.h:148-185
Agent set / collects_on / intersects: context.hpp:82-110, context.cpp:109-121
Conflict uses intersects: context.cpp:305-314
set_agents locked while active: core.cpp:211-248
Serialization refcounted per agent: queue_controller.hpp:97-175, queue_controller.cpp:568-635
Callback-thread refcount: sample_processing.cpp:171-190

Empty agent set still means every GPU. Filter before queue_cb (it returns serialize=true even when it declines to instrument).

Reviewer Guide

Walk the invariants (line links are in Technical Details):

Architecture doc, then the three hooks. Confirm enter = get_active_contexts, exit = get_registered_contexts.
queue.cpp: counters-only still enters the interceptor (318-328); batching disabled while counters active (768-781).
start_context does not call add_callback; callback_thread_start still runs (core.cpp:148-172).
context::stop_context (377-405) stops counters before the active-slot CAS. counters::stop_context (174-209): enabled=false → drain → unserialize → callback_thread_stop.
Attach-mode: no callback to unregister. enabled=false no-ops new dispatches; is_any_active drops after the slot clears.
collects_on before queue_cb (queue_hooks.cpp:62-66); set_agents rejected while active (core.cpp:222-233).
Serialization refcounted per agent (queue_controller.hpp:104-113).
client_ids.hpp overlaps registry ClientID. Inert because nothing routes on the tag. Do not add tag-based routing here.
Tests: queue_hooks_test.cpp:194-326, per_agent_scoping_test.cpp, per_agent_scoping_app.cpp.

Answer: In-flight record after rocprofiler_stop_context? GPU-1-only context leave GPU-0 alone? Stopping one serializer leave the other enabled? packet_return_map drop-by-one, empty, every record delivered?

Out of scope: deleting Queue::_callbacks / get_notifiers / add_callback; migrating other services; device-counting.

Issue Tracking

No JIRA ticket. Internal rocprofiler-sdk refactor of dispatch counter collection, plus the agent-scoping API.

Test Plan

is_any_active; stop-while-in-flight (three dispatches; per-completion map drain and record delivery after callback_thread_stop).
Per-agent unit (per_agent_scoping_test.cpp): set_agents validation/lock; disjoint start; overlapping conflict; serialization scope + refcount; enter-hook filter.
Start/stop tests in core.cpp now assert enabled, not iterate_callbacks.
Multi-GPU (per_agent_scoping_app.cpp): GPU-1-scoped context must not serialize/instrument GPU-0. Skips below two GPUs.

Test Result

Intended coverage is listed above; this does not claim green CI. Latest Actions on 70bef64 still reported failures (rocprofiler-sdk gfx94X shard, source, CodeQL Analyze, therock-pr-bot). Treat the Checks tab as source of truth.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

…ue callback registry Isolates the counter-collection slice of the callback-removal effort (reference PRs #8730 / #8586) into a small, single-service change, per review guidance to land callback removal one service at a time. Independent of kernel replay. Counter collection 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: - counters::write_hook / signal_completion_hook / is_any_active (new counters/queue_hooks.{hpp,cpp}) iterating active dispatch_counter_collection contexts and calling the existing queue_cb / completed_cb - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for thread-trace, pc-sampling and spm; those migrate in follow-up PRs. The callback thread (callback_thread_start/stop) is preserved. The WriteInterceptor gate and should_batch predicate detect counters via is_any_active() (it no longer contributes to queue.get_notifiers()). Removes the now-unused queue_id from counter_callback_info and updates counter tests off iterate_callbacks. Co-authored-by: Cursor <cursoragent@cursor.com>

Address review feedback on #8891: completion routing must use registered contexts (packet_return_map provenance), stop_context must drain in-flight GPU work before deactivation, and queue-interposed services must stop while the context remains active for serialization coordination. Adds queue_hooks regression test for stop-then-complete ordering required by kernel replay (#7960/#8622). Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Reverts the queue_controller_sync() added earlier on this branch. The justification given for it was wrong: it claimed callback_thread_stop() could strand records belonging to dispatches still in flight. It cannot. - consumer_thread_t::exit() sets valid=false, notifies, then waits on the exited flag, and consumer_loop only sets exited once read_ptr == write_ptr, so everything already queued is consumed before the join. counters/tests/ consumer_test.cpp restart already covers this drain-on-exit path. - consumer_thread_t::add() falls back to calling consume_fn inline when valid is false, so a completion arriving after the thread is gone is still processed on the async signal handler thread. Nothing else in counters::stop_context needs an idle GPU. In-flight completions are handled by signal_completion_hook routing over registered rather than active contexts, and the serializer transition is reconciled GPU-side by the hsa_barrier that profiler_serializer::disable() pushes. This aligns the counters path with the same change on the SPM branch, prompted by review feedback from @SrirackshaNag noting that SPM was the only service with a sync in its stop path and that the hook migration removes its original reasons. The context::stop_context ordering is kept but re-justified: with no drain, the point is that disable_serialization() runs before dispatches stop being instrumented, so no dispatch is submitted without serializer packets while the serializer is still enabled. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

@vlaindic

Applies the changes requested in review of the base PR, so that merging this stacked PR clears them on users/vkale/remove-callbacks-counters. Renames, per @vlaindic's comment that 'write' does not convey that this is a write interceptor hook, and per the name agreed in the reply on that thread: counters::write_hook -> counters::kernel_dispatch_phase_enter_hook counters::signal_completion_hook -> counters::kernel_dispatch_phase_exit_hook The exit hook is renamed for symmetry even though only the enter hook was flagged; leaving one half renamed reads worse than either alternative, and the paired names make the enqueue-versus-completion asymmetry this branch introduces easier to follow. is_any_active keeps its name, being neither phase. SPM mirrors these names on its own branch and is deliberately left alone until the review of #8887 settles, so the two branches are briefly inconsistent by choice rather than by oversight. client_ids.hpp: the four constants become an unscoped enum with int64_t as its fixed underlying type. That answers the question asked in review -- the int64_t was not arbitrary, it matches hsa::ClientID, which is how the tag is stored in inst_pkt_t. Unscoped rather than enum class so the implicit conversion to ClientID survives and no emplace site needs a cast. The regression test now constructs the pair piecewise instead of through std::make_pair, which would deduce the tag's enum type and lean on a pair-to-pair conversion; piecewise converts the tag directly and matches the production hook. Copyright headers on the four files the base PR adds move from 2025 to 2026. Verified without a build, which is not available here: clang-format 11.1.0 (the version the repo's .clang-format pins) leaves no unrelated churn, no references to the old names remain, and a standalone translation unit confirms the enum converts to ClientID in both the piecewise and make_pair forms. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… just unmapped Strengthens the stop-while-in-flight regression test to what the review on #8891 asked for: enqueue kernels, stop the context mid-flight, and assert every kernel produces a record. The previous version had two weaknesses. It passed nullptr for the record callback, so it could not observe delivery at all -- and delivery is the failure the review described. And it drove a single dispatch, which cannot distinguish 'routed the one entry present' from 'routed by provenance'. Now three dispatches are registered while the context is active, the context is stopped while all three are in flight, and completions are fired one at a time. Two independent invariants are checked, because they fail separately: - routing: the map is asserted to drop by exactly one per completion, not merely to reach zero at the end. An aggregate check would pass even if one completion erased another dispatch's entry. - delivery: every dispatch reaches the tool's record callback. Erasure happens in completed_cb before the hand-off to the consumer, so a map-only test would pass even if nothing were ever delivered. The delivery assertion also covers the lifetime condition raised in the review: callback_thread_stop() has already run by then, so these records arrive through the inline path in consumer_thread_t::add() rather than the consumer thread. No kernel executes, so counter values are whatever the profile's output buffer holds; the assertion is about delivery, not values. Not compiled here -- no ROCm toolchain in this environment. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…'s ClientID Captures the non-blocking observation from the review of #8790 at the declaration rather than only in a PR discussion, since that is where someone about to route on these values will look. The tags overlap the per-queue registry's auto-incrementing ClientID, which also starts at 1 and is still in use by the services not yet migrated. This is inert only because no consumer dispatches on the tag: counters' completed_cb and thread trace's post_kernel_call both identify their own packets by pointer lookup or dynamic_cast. The comment states what has to happen before that changes. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ntext Restores the queue_controller_sync() this branch had earlier removed, per review priority: the review of #8891 accepted provenance-based completion routing on the condition that the callback thread and the counter_callback_info objects stay alive until in-flight dispatches drain, and the drain is what makes that hold literally rather than by argument about consumer_thread_t's fallback behaviour. An intermediate revision removed it on the reasoning that consumer_thread_t::exit() drains its queue and add() self-consumes inline afterwards. Both facts are true and they remain a useful backstop, but they are not the guarantee that was asked for -- they argue a late completion is still processed, rather than ensuring there is no late completion to process. The comment now says which is which. Ordering matters and is now stated at the call site: context::stop_context runs this function while the context is still in the active list, so for the duration of the drain the enter hook still reaches queue_cb, whose disabled path returns serialize=true and keeps the serialized->unserialized transition coordinated until disable_serialization() runs. That is what removes the need for a separate draining flag -- and it only works because the drain happens before the active slot is cleared. callback_thread_stop() now runs after the drain rather than alongside it. Not compiled here -- no ROCm toolchain in this environment. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…th drain The comment claimed provenance routing makes stop_context safe without a GPU drain, which contradicts counters::stop_context, where the drain was kept. State what each mechanism actually guarantees: provenance routing delivers a completion that arrives, the drain bounds when completions arrive. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Records how queue-interposed services are wired into dispatch interception after the per-queue callback registry is removed: why the enter hook routes over active contexts while the exit hook routes over registered ones, why that asymmetry is the difference between delivering and dropping the completion of an in-flight dispatch, and why the stop path drains the GPU. Folded in from a separate docs branch so it is reviewed alongside the code it describes. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… its constructor context::correlation_id declares constructors, deletes its copy and move, and holds private ref counters, so it is not an aggregate and a designated initializer does not compile: error: initialization of non-aggregate type 'context::correlation_id' with a designated initializer list Default-construct and assign the one member the test needs, which leaves every other member at the same default the designated initializer would have. This broke the rocprofiler-sdk build, which is why all four Core test jobs and Code Coverage failed alongside the TheRock package builds. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…eue interception counters::stop_context() drains in-flight dispatches via queue_controller_sync() and coordinates serializer teardown, both of which require a live queue controller. finalize() ran queue_controller_fini() first and invoked the client finalizers afterwards, so stopping a counter-collection context during teardown operated on destroyed queue state and segfaulted. Also skip empty optional slots when walking the registered-context array; the array is sparse and dereferencing a disengaged optional is undefined. Fixes the hip-streams-per-thread SEGFAULT seen in CI. Co-authored-by: Cursor <cursoragent@cursor.com>

Moving invoke_client_finalizers() ahead of the subsystem teardowns deadlocked OMPT finalization: rocprofv3-test-ompt-tracing and rocprofv3-test-ompt-all timed out on every CI OS, and openmp-tools.test_retired_correlation_ids failed with them. The reorder also did not fix the hip-streams-per-thread SEGFAULT it was meant to address, so revert it and keep the original ordering. The sparse-optional guards in context.cpp are unaffected and stay in place. Co-authored-by: Cursor <cursoragent@cursor.com>

buffered-api-tracing and the two thread-trace-api-triple-buffer tests failed on ubuntu-22.04 only, while rhel-8.8, rhel-9.5 and sles-15.6 passed and TheRock's own rocprofiler-sdk shard passed. No content change; this re-runs CI on an identical tree to confirm. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Dispatch counter collection applied to the whole machine: a context had no notion of which GPUs it collected on, so serialization was enabled on every agent for as long as any counter-collection context was active, and only one such context could be active at all. A tool could decline to profile a dispatch by returning a null config, but that only suppressed instrumentation -- the dispatch was still serialized, which is the part that costs performance and the part the tool had no lever over. The public header has carried a note since the service was introduced saying the agent restriction was unresolved and left to the tool. Store the agent set on the context's counter-collection service and factor it into the three queries that need it: serialization scope at start/stop, the dispatch enter hook, and counter-collection context conflict. An empty set still means every agent, so existing tools are unaffected. Contexts with disjoint agent sets may now be active simultaneously, which is what makes "context A on GPU-0, context B on GPU-1" expressible. Serialization is now reference counted per agent. Counter collection, thread trace and SPM each enable it independently, and without a refcount whichever service stopped first silently unserialized the ones still running. That was a pre-existing bug; scoping serialization per agent makes it reachable more easily, so it is fixed here rather than left behind. Adds rocprofiler_dispatch_counting_service_set_agents() to declare the set. It is rejected while the context is active, since the dispatch path reads the set without a lock and start_context has already used it to scope serialization. Co-authored-by: Cursor <cursoragent@cursor.com>

The previous commit was formatted with clang-format 18 and left the source and cmake format checks failing. CI pins clang-format 11.1.0 and cmake-format 0.6.13, and v18 disagreed with v11 on two spots -- it joined a pre-existing ROCP_ERROR continuation in counters/core.cpp onto one line and moved the extern "C" brace in dispatch_counting_service.cpp -- neither of which this change had any reason to touch. Co-authored-by: Cursor <cursoragent@cursor.com>

…ware The unit tests assert the internal state, which cannot show the property a user actually cares about: that a GPU outside the context's agent set keeps running its streams concurrently. This adds an integration test that measures a multi-stream workload on GPU-0 while a counter collection context is scoped to GPU-1, and fails if GPU-0 gets serialized or instrumented. Machines differ too much for an absolute time to mean anything, so every timing assertion is a ratio against a baseline measured in the same process, and the unrestricted context serves as a positive control -- when it does not visibly serialize the same workload the machine cannot demonstrate stream concurrency and the timing assertions are skipped instead of guessed at. The record and callback assertions are exact and always run. The test reports SKIP and exits 0 below two GPUs. Verified against a deliberately reverted build: without the agent filtering the scoped GPU-0 workload goes from 7.7ms to 32.0ms and three checks fail. Co-authored-by: Cursor <cursoragent@cursor.com>

@jrmadsen

Suggested by @jrmadsen while reviewing #9868, applied as written: hsa/queue_controller.hpp using agent_handle_set_t = std::unordered_set<rocprofiler_agent_id_t> context/context.hpp std::unordered_set<rocprofiler_agent_id_t> agents{} context/context.hpp return agents.empty() || agents.count(agent_id) > 0 Also pulls in <rocprofiler-sdk/cxx/hash.hpp> and <rocprofiler-sdk/cxx/operators.hpp>, which he noted give full support for using the opaque handles in map/set/uomap/etc. Those includes are what let the sets key on rocprofiler_agent_id_t directly instead of unwrapping to the uint64_t handle. The cascading retyping this implies for the serialization refcounts and their call sites follows in the next commit, so that this one holds only the suggested text. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ects out of line Follows the suggestions applied in the previous commit. Retyping the two agent sets to rocprofiler_agent_id_t leaves the surrounding code still speaking in raw uint64_t handles, so this carries the change through: - serialization_refcount::per_agent keys on rocprofiler_agent_id_t, and count(), enabled() and any() take/iterate the typed id - queue_controller.cpp and counters/core.cpp stop unwrapping to ::handle at the call sites - comments that described the sets as holding rocprofiler_agent_id_t::handle are reworded, since they no longer do Also addresses jrmadsen's fourth review thread, which asked for the body to live in the .cpp: dispatch_counter_collection_service::intersects is now declared in context.hpp and defined in context.cpp. He described that one rather than writing it, so it sits here rather than in his commit. Co-authored-by: Cursor <cursoragent@cursor.com>

Use rocprofiler_agent_id_t values in per_agent_scoping_test serialization calls instead of raw handles, and apply clang-format 11 to queue_controller. Co-authored-by: Cursor <cursoragent@cursor.com>

…ace in update_serialization Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

… refcount, serializer lock, context mutex) Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

…ader The Aug 18 review follow-up declared get_contexts_mutex in context.hpp while leaving the original anonymous-namespace definition in context.cpp, which made every unique_lock on that mutex an ambiguous overload. Keep a single std::mutex& definition in the context namespace. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Vivek Kale <vivek.kale@amd.com>

therock-pr-bot · 2026-09-18T17:18:46Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-09-18T17:18:48Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot

🟡 Changes recommended

Critical concurrency and test-build issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Refactors dispatch counter collection to use explicit queue hooks with per-GPU agent scoping and serialization refcounting.

Changes:

Replaces per-queue counter callbacks with enter/exit hooks.
Adds agent-scoped counting and serialization coordination.
Adds lifecycle, unit, integration, and architecture documentation coverage.

Outstanding issues include concurrency/lifecycle races and two test defects preventing safe approval.

File summaries

File	Summary
`projects/rocprofiler-sdk/tests/counter-collection-per-agent/per_agent_scoping_app.cpp`	Multi-GPU scoping workload
`projects/rocprofiler-sdk/tests/counter-collection-per-agent/CMakeLists.txt`	Builds integration test
`projects/rocprofiler-sdk/tests/CMakeLists.txt`	Registers integration tests
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp`	Invokes counter hooks
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt`	Registers hook headers
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp`	Defines producer identifiers
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp`	Declares scoped serialization
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp`	Implements serialization refcounting
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt`	Adds hook directory
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/dispatch_counting_service.cpp`	Implements agent-scoping API
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/queue_hooks_test.cpp`	Tests hook routing and completion
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/per_agent_scoping_test.cpp`	Tests scoping and conflicts
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/core.cpp`	Updates lifecycle assertions
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/CMakeLists.txt`	Registers counter tests
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/sample_processing.cpp`	Refcounts callback thread
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/queue_hooks.hpp`	Declares counter hooks
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/queue_hooks.cpp`	Implements enter/exit routing
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/core.hpp`	Updates counter state declarations
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/core.cpp`	Updates lifecycle and agent configuration
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/CMakeLists.txt`	Adds hook implementation
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp`	Adds agent-set state
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp`	Adds scoping and conflict handling
`projects/rocprofiler-sdk/source/include/rocprofiler-sdk/dispatch_counting_service.h`	Declares agent-scoping API
`projects/rocprofiler-sdk/source/docs/conceptual/queue_hooks/queue_callback_removal_architecture.md`	Documents hook migration

Review details

Suppressed comments (1)

projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/sample_processing.cpp:183

start_context publishes the context in the active-slot array before it calls counters::start_context, while stop_context can acquire the context mutex and run counters::stop_context in that window. In that interleaving the service is still disabled, but callback_thread_stop() decrements this refcount (even though this start has not incremented it yet), and the subsequent callback_thread_start() runs after the context has already been stopped, leaving the refcount/thread state inconsistent. Synchronize service startup with the same start/stop transition (or make the refcount/start-stop operation conditional on an actual enabled transition).

    ++callback_thread_refcount;
    callback_thread_get().start();

Files reviewed: 24/24 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        else if(cfg->dispatch_counter_collection && itr->dispatch_counter_collection &&
+                cfg->dispatch_counter_collection->intersects(*itr->dispatch_counter_collection))


-    CHECK_NOTNULL(controller)->enable_serialization();
+    // Scope serialization to the agents this context collects on. An empty set still means
+    // every agent, so an unrestricted context serializes the whole machine as before.
+    CHECK_NOTNULL(controller)->enable_serialization(ctx->dispatch_counter_collection->agents);


+    rocprofiler::counters::kernel_dispatch_phase_exit_hook(
+        *reinterpret_cast<rocprofiler::hsa::Queue*>(nullptr),
+        fq_pkt,
+        sess,
+        packet,
+        inst_pkt,


+    registration::set_init_status(1);
+    registration::finalize();
+    context::pop_client(1);
+    set_client_ctx(get_client_ctx());


…pstream-remove-callbacks-counters Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/CMakeLists.txt # projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp # projects/rocprofiler-sdk/tests/CMakeLists.txt

Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

Copilot · 2026-09-21T20:19:24Z

🟡 Changes recommended

Critical concurrency and test-build issues remain unresolved....

Addressed in 8305ff9: the dispatch-counter conflict scan and activation now stay under the same context mutex, dispatch-counter start/stop only changes serialization state on real enabled-state transitions, and the duplicate-start regression is covered by a new test.

github-actions · 2026-10-17T00:21:43Z

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

Add new commits
Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

github-actions · 2026-10-23T00:22:38Z

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

amd-vkale and others added 23 commits August 13, 2026 01:21

fix(rocprofiler-sdk): repair #9868 CI compile and clang-format failures

fbe364e

Use rocprofiler_agent_id_t values in per_agent_scoping_test serialization calls instead of raw handles, and apply clang-format 11 to queue_controller. Co-authored-by: Cursor <cursoragent@cursor.com>

fix: hold refcount lock through serializer transitions to eliminate r…

4bfedef

…ace in update_serialization Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

fix: address all reviewer comments (header docs, test races, callback…

106307d

… refcount, serializer lock, context mutex) Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

perf(rocprofiler-sdk): counters exit-hook fast path and gate caching

454b141

Co-authored-by: Vivek Kale <vivek.kale@amd.com>

amd-vkale requested review from a team as code owners September 18, 2026 17:17

Copilot AI lite review requested due to automatic review settings September 18, 2026 17:17

ROCm deleted a comment from cursor Bot Sep 18, 2026

github-actions Bot added documentation Improvements or additions to documentation project: rocprofiler-sdk labels Sep 18, 2026

Copilot started reviewing on behalf of amd-vkale September 18, 2026 17:18

View session

therock-pr-bot Bot added the Not ready to Review label Sep 18, 2026

Copilot AI reviewed Sep 18, 2026

View reviewed changes

systems-assistant Bot added the organization: ROCm label Sep 18, 2026

Copilot started work on behalf of amd-vkale September 21, 2026 20:15

View session

fix review issues in dispatch counter activation

8305ff9

Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

Copilot finished work on behalf of amd-vkale September 21, 2026 20:19

This was referenced Sep 25, 2026

refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry #11970

Merged

refactor(rocprofiler-sdk): migrate SPM off the per-queue callback registry (moved PR 8887 to here) #11968

Merged

github-actions Bot added the Stale PR has no activity for 25+ days label Oct 17, 2026

github-actions Bot closed this Oct 23, 2026

Navigation Menu

Repository navigation

refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry - #11861

refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry#11861
amd-vkale wants to merge 25 commits into
developfrom
users/vkale/remove-callbacks-counters

amd-vkale commented Sep 18, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI commented Sep 21, 2026

🟡 Changes recommended

Uh oh!

github-actions Bot commented Oct 17, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		else if(cfg->dispatch_counter_collection && itr->dispatch_counter_collection &&
		cfg->dispatch_counter_collection->intersects(*itr->dispatch_counter_collection))

Conversation

amd-vkale commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Hooks

Stop path

Per-agent scoping

Reviewer Guide

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

therock-pr-bot Bot commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI commented Sep 21, 2026

🟡 Changes recommended

Uh oh!

github-actions Bot commented Oct 17, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amd-vkale commented Sep 18, 2026 •

edited

Loading

therock-pr-bot Bot commented Sep 18, 2026 •

edited

Loading