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

amd-vkale · 2026-09-21T20:01:18Z

Rehosted copy of #8887. Identical content, but the head branch users/vkale/remove-callbacks-spm lives in
ROCm/rocm-systems instead of a fork, so the CodeQL workflow can read the container
registry secrets it needs. Review history stays on #8887, which is now stacked on this
branch.

Motivation

The purpose of this PR is to isolate the SPM 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. This is independent of the kernel replay feature in PR #7960 but it aids it.

Underlying Problem

Dispatch SPM 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 SPM context is active.

This PR:

Stops registering SPM with that map. The write interceptor and async signal handler call explicit free functions instead (spm::write_hook, signal_completion_hook, is_any_active).
Routes completions by provenance — enter iterates active contexts, exit iterates registered contexts so in-flight dispatches still complete after stop.
Scopes SPM to GPU agents via rocprofiler_spm_dispatch_counting_service_set_agents, with GPU drain on stop and per-agent serialization refcounting.

The per-queue callback registry remains for counters, thread trace, and PC sampling until those services land in their own PRs.

Notes on Bigger Picture

Aids PR #7960 and the kernel replay work tracked under AIPROFSDK-68. This PR also helps other features that require removing the SPM per-queue callback registry, but does not depend on kernel replay landing first.

Sibling migrations: thread trace (#8790), counter collection (#8891), PC sampling (#8895).

Technical Details

Enter = active, exit = registered. Exit self-filters via packet_return_map so in-flight dispatches complete after stop.

flowchart LR
  WriteInterceptor --> WriteHook["spm::write_hook: active + collects_on"]
  AsyncSignalHandler --> CompletionHook["signal_completion_hook: registered"]
  ContextStop --> Drain["enabled=false, queue_controller_sync, unserialize, clear slot"]

Permalinks pin to 758072b.

Hooks and call sites

Declarations: queue_hooks.hpp
Enter: queue_hooks.cpp#L42-L75
Exit (registered / provenance): queue_hooks.cpp#L77-L101
is_any_active: queue_hooks.cpp#L103-L107
no_real_consumers gate: queue.cpp#L318-L322
Enter call site: queue.cpp#L632-L642
Exit call site: queue.cpp#L172-L179
Batching disabled while active: queue.cpp#L778-L779
start_context (no add_callback): core.cpp#L258-L276
Stop (enabled → drain → unserialize): core.cpp#L283-L323
Stop services before active-slot CAS: context.cpp#L392-L408
Conflict uses intersects: context.cpp#L311-L317
set_agents API: spm.h#L318-L338
Producer tag: client_ids.hpp#L37-L40

Reviewer Guide

Confirm enter = active, exit = registered (queue_hooks.cpp).
SPM-only runs still enter WriteInterceptor (queue.cpp#L318-L322).
start_context does not call add_callback (core.cpp#L258-L276).
context::stop_context stops SPM before the active-slot CAS (context.cpp#L392-L408).
spm::stop_context keeps queue_controller_sync (core.cpp#L283-L323) — drain bounds when completions arrive; provenance routing bounds delivery.
In-flight regression: spm_core test stop_context_in_flight_completion_routes_via_hook_path (core.cpp#L990+) — packet_return_map drains via signal_completion_hook after stop.
Per-agent: set_agents_restricts_collection, disjoint SPM contexts (core.cpp#L1128+).
Gate-only unit test: queue_hooks_test.cpp#L33-L36.

Questions to Answer:

In-flight SPM dispatch still gets DISPATCH_END / kfd_stop after stop? packet_return_map empty?
GPU-1-only SPM leave GPU-0 alone?

Out of scope: deleting Queue::_callbacks; other service migrations.

Issue Tracking

JIRA ticket: AIPROFSDK-1018

Related JIRA ticket on kernel replay: AIPROFSDK-68. The kernel replay PR is #7960.

Test Plan

is_any_active
In-flight + per-agent tests in spm/tests/core.cpp
Existing start/stop / sync / restart tests updated off callback iteration

Test Result

Intended coverage listed above. Treat the Checks tab as source of truth for CI.

Submission Checklist

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

…istry Isolates the SPM 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. SPM 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: - spm::write_hook / signal_completion_hook / is_any_active (new spm/queue_hooks.{hpp,cpp}) iterating active dispatch_spm contexts and calling the existing pre_kernel_call / post_kernel_call - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for counters, thread-trace and pc-sampling; those migrate in follow-up PRs. The WriteInterceptor gate and should_batch predicate detect SPM via is_any_active() (it no longer contributes to queue.get_notifiers()). Removes the now-unused queue_id from spm_counter_callback_info and updates spm tests off iterate_callbacks. Co-authored-by: Cursor <cursoragent@cursor.com>

Address review feedback on #8887 with the same completion-routing and stop_context lifecycle fixes applied for counters in #8891. signal_completion_hook iterates registered SPM contexts; stop_context stops queue-interposed services before deactivating the context slot. Adds queue_hooks regression test for stop-then-complete ordering. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Per review feedback from @SrirackshaNag: the queue_controller_sync() in spm::stop_context was there to handle in-flight dispatches and to avoid dangling callback pointers once the per-queue callbacks were unregistered. Neither reason survives the hook migration. - dangling pointers: nothing is registered any more; signal_completion_hook is a static function that resolves contexts at completion time. - in-flight dispatches: handled by signal_completion_hook routing over registered rather than active contexts (previous commit on this branch). post_kernel_call never gated on the enabled flag, so the completion runs normally. Two further reasons a drain might have been needed, both checked and neither applicable: - serializer transition: profiler_serializer::disable() records the previous state and pushes an hsa_barrier across the queues, and kernel_completion_signal reconciles in-flight dispatches against it. The transition is GPU-ordered and does not require the CPU to observe an idle GPU. - service teardown: spm::stop_context frees no buffer, destroys no profile and stops no hardware, so nothing it touches has a lifetime that ends here. Note the drain was never a guarantee in any case: Queue::sync uses a 5 s hint and only warns on timeout, and _active_kernels counts intercepted dispatches only. Also re-justifies the context::stop_context ordering. It is kept, but not for the reason originally given: 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. Comments in spm_core.stop_context_sync_and_restart updated to match; the test name is left alone to avoid churn in existing local test invocations. Open question for @SrirackshaNag: the drain also incidentally covered stop_context followed by destroying the counter config. If any tool does that, the guard belongs at spm_destroy_profile rather than at stop. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…s not address Comment-only. Names the two blocking issues raised in the review of #8887 and how each is handled, so a reader does not have to reconstruct it from the review. spm/queue_hooks.cpp: states the consequence of dropping an in-flight completion -- the DISPATCH_END record, kfd_stop() and barrier-signal cleanup are all skipped and the map entry leaks -- and records why widening to registered contexts stays per-origin: each context's post_kernel_call only claims packets in its own packet_return_map. That matters because, unlike dispatch_counter_collection, there is no conflict guard rejecting a second concurrent dispatch_spm context. spm/core.cpp: marks the drain removal as a deliberate divergence from the review, which asked for the sync to be kept plus an explicit draining state. The serialization window that state would protect is closed instead by context::stop_context running this function before clearing the active slot, so disable_serialization happens while the enter hook can still see the context. The two approaches are not obviously equivalent, so the reasoning is recorded rather than left implicit, and it should be revisited if that ordering changes. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… priority Restores the queue_controller_sync() an earlier commit on this branch removed. The review of #8887 asked for it to be kept, with SPM held visible to the enter hook and to serialization through a draining window until it and disable_serialization() complete. That takes precedence over the later suggestion to remove it. The removal argued that the sync's two original purposes were addressed by the migration: dangling callback pointers, and letting in-flight dispatches complete. The first is genuinely gone -- nothing is registered any more. The second does not survive scrutiny: provenance routing guarantees that a completion which arrives is delivered, whereas the drain bounds when completions arrive at all, and the rest of the teardown depends on the latter. The comment now records both the request and why the counter-argument is weaker than it appeared, so this is not relitigated silently. No draining flag is introduced, because the visibility half already comes from ordering: context::stop_context calls this function while the context is still in the active list, so throughout the drain the enter hook still reaches pre_kernel_call, whose disabled path deliberately returns serialize=true to coordinate the serialized->unserialized transition, and the slot is cleared only after this function returns. The comment marks that ordering as load-bearing -- if the slot were cleared first, the enter hook would go blind before disable_serialization() ran and a dispatch could be submitted unserialized while the serializer was still enabled. Also brings this branch's own earlier additions into clang-format compliance; they had not been formatted. Verified with clang-format 11.1.0, the version the repo pins, and confined to lines this branch adds -- nothing pre-existing is reformatted. Not compiled here -- no ROCm toolchain in this environment. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

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

… its default ctor The designated initializer does not compile: correlation_id is not an aggregate. Same pattern #9586 already used for the counters hook-path test, without which Co-authored-by: Cursor <cursoragent@cursor.com> #10098 cannot build.

Apply per-agent scoping from PR #9868 to the SPM callback-removal branch: - Per-agent serialization ref-counting in queue_controller - agents set on spm_dispatch_counter_collection_service with collects_on/intersects - rocprofiler_spm_dispatch_counting_service_set_agents() public API - Agent filtering in spm::write_hook and disjoint SPM context conflict rules Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Apply clang-format 11 to queue_controller and construct correlation_id via its constructor in the SPM stop_context completion test. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

The dispatch_thread_trace stop call fits on one line under the project clang-format profile. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…add set_agents/disjoint-context tests Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

- Skip SPM completion hook when inst_pkt has no SPM_CLIENT_ID tag - Cache is_any_active() once per WriteInterceptor invocation - Add unit test for exit-hook skip path Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ook test stop_context_in_flight_completion_routes_via_hook_path set corr_id.internal to 99 but never expected.correlation_id, so user_dispatch_cb compared against 0. 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 4e1e51a)

…ng races Two races in QueueController, both reported in review and both still open. serializer() read the per-agent refcount, released it, and only then looked up or inserted the serializer entry. A concurrent update_serialization() in that gap finds no entry to transition, so the entry is created from a stale observation and keeps the wrong state indefinitely: created from a stale `true` it stays serialized at refcount zero, created from a stale `false` the first dispatch runs unserialized. Hold the refcount read lock across the lookup and insertion so the observation and the insertion are atomic with respect to updates. update_serialization() computed which agents transitioned under the refcount write lock but applied the serializer enable/disable after releasing it. Two callers could therefore apply their transitions in the opposite order from the refcount mutations that produced them, leaving the serializer enabled at refcount zero. Apply the transitions inside the same refcount write lock, and derive the action from the post-update refcount rather than the caller's `enable` flag. Lock order is refcount -> serializer map -> per-agent serializer in both paths, so they cannot deadlock against each other; neither enable() nor disable() calls back into serializer() or the refcount. This is the protocol already used on the counters branch; both branches now carry identical serializer() and update_serialization() bodies. (cherry picked from commit 6b5b59e)

Already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

spm/tests/queue_hooks_test.cpp built a Queue& by dereferencing a null pointer to reach the hook's early-return path, which clang rejects outright: error: reinterpret_cast from 'std::nullptr_t' to 'rocprofiler::hsa::Queue *' is not allowed The hook never dereferences the queue, so take it as a pointer and let the test pass nullptr. This matches the counters and PC sampling branches. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… a pointer spm/tests/core.cpp still passed a FakeQueue by reference after the hook was changed to take const hsa::Queue*: error: cannot convert 'rocprofiler::hsa::FakeQueue' to 'const rocprofiler::hsa::Queue*' Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ook test Two errors g++ reports against the libstdc++ headers, so they are easy to miss when scanning diagnostics by source path: error: invalid new-expression of abstract class type 'rocprofiler::hsa::AQLPacket' error: use of deleted function 'rocprofiler::hsa::queue_info_session_t::queue_info_session_t()' AQLPacket is abstract; EmptyAQLPacket is the concrete no-op subclass intended for this. queue_info_session_t holds a Queue& so it has no default constructor, and the hook returns before dereferencing the session, so a null shared_ptr keeps the test free of any HSA runtime. 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>

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

…ve-callbacks-spm

Keeps develop's CMAKE_VERSION_DEB 4.4.3 in the code coverage workflow; the 3.25 pin on the CI-repair side predates this branch merging current develop, whose external submodules no longer declare cmake_minimum_required below 3.5.

therock-pr-bot · 2026-09-21T20:02:39Z

✅ 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?

amd-vkale · 2026-09-21T20:02:54Z

Superseded: recreating from the --with-cursor-fixes branch name so the base branches are consistently named.

Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

SPM conflict check races with context activation · New
Process-wide SPM batching disable affects unrelated GPU queues · New

What changed in this PR

Migrates SPM dispatch instrumentation from the per-queue callback registry to explicit hooks, with agent-scoped collection and serialization.

Changes:

Adds explicit SPM enqueue/completion hooks and provenance routing.
Adds GPU-agent restriction APIs and per-agent serialization refcounting.
Updates tests, build files, and compatibility workflow.

File	Description
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/CMakeLists.txt	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/CMakeLists.txt	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp	Updated as part of this pull request.
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h	Updated as part of this pull request.
.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml	Updated as part of this pull request.

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

+                cfg->dispatch_spm->intersects(*itr->dispatch_spm))
+        {
+            // Two SPM dispatch contexts can run concurrently as long as they target disjoint
+            // sets of GPU agents. A context with no agent restriction claims every agent.
+            return ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT;


+    // SPM requires per-packet mode; it no longer participates in the registry above.
+    if(spm_active) should_batch_packets = false;


amd-vkale and others added 24 commits July 20, 2026 19:18

fix(spm/tests): destroy counter config before early continue to avoid…

1297d31

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

fix(rocprofiler-sdk): repair SPM per-agent scoping CI failures

996e640

Apply clang-format 11 to queue_controller and construct correlation_id via its constructor in the SPM stop_context completion test. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

style(rocprofiler-sdk): clang-format context.cpp stop_context

a8615cf

The dispatch_thread_trace stop call fits on one line under the project clang-format profile. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

fix(rocprofiler-sdk): store effective state in update_serialization; …

758072b

…add set_agents/disjoint-context tests Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>

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

ee045e3

- Skip SPM completion hook when inst_pkt has no SPM_CLIENT_ID tag - Cache is_any_active() once per WriteInterceptor invocation - Add unit test for exit-hook skip path Co-authored-by: Vivek Kale <vivek.kale@amd.com>

style(rocprofiler-sdk): apply clang-format to the SPM tests

c9dc1a7

Already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Merge remote-tracking branch 'upstream/develop' into users/vkale/remo…

864e1f4

…ve-callbacks-spm

Copilot AI lite review requested due to automatic review settings September 21, 2026 20:01

amd-vkale requested review from a team as code owners September 21, 2026 20:01

github-actions Bot added project: rocprofiler-sdk github actions Pull requests that update GitHub Actions code labels Sep 21, 2026

Copilot started reviewing on behalf of amd-vkale September 21, 2026 20:02

View session

amd-vkale closed this Sep 21, 2026

Copilot AI reviewed Sep 21, 2026

View reviewed changes

amd-vkale mentioned this pull request Sep 25, 2026

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

Merged

1 task done

Navigation Menu

Repository navigation

refactor(rocprofiler-sdk): migrate SPM off the per-queue callback registry - #11965

refactor(rocprofiler-sdk): migrate SPM off the per-queue callback registry#11965
amd-vkale wants to merge 24 commits into
developfrom
users/vkale/remove-callbacks-spm

amd-vkale commented Sep 21, 2026

Uh oh!

therock-pr-bot Bot commented Sep 21, 2026

Uh oh!

amd-vkale commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		// SPM requires per-packet mode; it no longer participates in the registry above.
		if(spm_active) should_batch_packets = false;

Conversation

amd-vkale commented Sep 21, 2026

Motivation

Underlying Problem

Notes on Bigger Picture

Technical Details

Hooks and call sites

Reviewer Guide

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Sep 21, 2026

✅ All Policy Checks Passed

Uh oh!

amd-vkale commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants