Repository navigation
refactor(rocprofiler-sdk): address review feedback on #8887 for remove-callbacks-spm - #10098
Merged
amd-vkale merged 6 commits intoAug 18, 2026
Conversation
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the rocprofiler-sdk SPM queue hook routing and context shutdown ordering to preserve in-flight dispatch completion behavior after callback-registry removal, and adds a regression test to validate the new hook path.
Changes:
- Route SPM completion handling via registered (not only active) contexts to ensure in-flight dispatches still complete after
stop_context. - Adjust
context::stop_contextordering so queue-interposed services stop before clearing the active slot, preserving serialization/enter-hook visibility guarantees. - Add a regression unit test covering the stop-before-completion scenario and update related test comments.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp | Adds a regression test exercising the queue_hooks completion path after stop_context; updates stop-context test comments. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp | Documents completion hook behavior to iterate registered contexts for provenance-based routing. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp | Implements completion routing over registered contexts; keeps enter hook / is_any_active() based on active contexts. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp | Restores/clarifies drain-before-disable-serialization rationale and ordering requirements in spm::stop_context. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp | Stops queue-interposed services before clearing the active slot to avoid instrumentation/serialization ordering gaps. |
Suppressed comments (1)
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp:1075
- This
continuepath also skipsrocprofiler_spm_destroy_counter_config(expected.id)even though the counter config was already created earlier in the loop iteration, leaking the config.
size_t map_size_before_stop = 0;
cb_info->packet_return_map.rlock(
[&](const auto& data) { map_size_before_stop = data.size(); });
if(map_size_before_stop == 0) continue;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
… leak Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>
Contributor
Fixed in commit fix(spm/tests): destroy counter config before early continue to avoid leak. Both early- |
|
Automated stacked-PR status check (Cursor) Stack position: Why:
Remaining steps:
Posted by an automated stacked-PR status check — no code or branch changes were made. |
1 task
Contributor
Author
|
Mergeability (local review + GPU unit tests, 2026-08-18) Ready to fast-forward into #8887 after one test-only compile fix. #8887's tip is an ancestor of this head. The production change matches the #8891/#9586 liveness fix that @bwelton requested here as well: Local results:
Not folding #9932 on top (see comment there): no human review, and it has diverged from this head by the later test-leak commit. I will include the two-line |
1 task done
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR 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.
It is one of four sibling migrations, each independently reviewable:
Kernel replay (#7960, #8622) is greatly enhanced by this change, but the two are independent.
This PR is behavior-preserving on its own and should merge into
developcleanly with or withoutthe kernel replay feature.
Technical Details
Counter collection no longer registers a
queue_callbacks_twith the HSA queue controller. The HSAwrite interceptor and the async signal handler now call explicit free functions instead.
New —
counters/queue_hooks.{hpp,cpp}write_hook,signal_completion_hook,is_any_active, each iterating activedispatch_counter_collectioncontexts and calling the existingqueue_cb/completed_cb.do.
New —
hsa/queue_hooks/client_ids.hppinst_pkt_tentries. All four service ids are reserved up front, notjust the one this PR needs, so the tag values stay stable as each remaining service migrates and
test ordering does not shift underneath the later PRs.
Changed
WriteInterceptorgate and theshould_batchpredicate detect counters viais_any_active(); counter collection no longer contributes toqueue.get_notifiers().queue_idfromcounter_callback_info.iterate_callbacks.Deliberately not changed
removed only once all four services are migrated.
callback_thread_start/callback_thread_stop) is preserved.Rebase, 12 Aug
Rebased onto
developat20bb64fb32to clear the merge conflict. There was exactly one conflictand it was mechanical:
developrenamedfirmware_restrictions.cpptofirmware_restrictions_test.cppwhile this branch addedqueue_hooks_test.cpp, so both entriesnow appear in
counters/tests/CMakeLists.txt.No source changes were made during the rebase. The diffstat is unchanged at 300 insertions, 82
deletions across 12 files, which is the check that nothing drifted. The two stacked PRs were
restacked at the same time and both remain mergeable.
Relationship to the stacked PRs
Review feedback on this PR is intentionally not applied here, to keep this PR a pure mechanical
migration that is easy to verify by inspection:
liveness-based, so an in-flight dispatch's completion can be dropped when the context stops — is
fixed in refactor(rocprofiler-sdk): address review feedback on #8891 for remove-callbacks-counters #9586.
kernel_dispatch_phase_enter_hook/kernel_dispatch_phase_exit_hook, converting the client ids to an unscoped enum with anint64_tfixed underlying type, and the copyright years — are also in refactor(rocprofiler-sdk): address review feedback on #8891 for remove-callbacks-counters #9586.Reviewers who want to see the end state rather than the increment should read #9586 alongside this
PR.
Issue Tracking
AIPROFSDK-1017.
Motivated by the kernel replay work tracked under AIPROFSDK-68; see #7960 for why the callback
registry is a problem for replay. This PR does not depend on that work.
Test Plan
The migration is intended to be behaviour-preserving, so the real coverage is the existing suite
rather than new tests.
counters/tests/queue_hooks_test.cpp,counters_queue_hooks.is_any_active_false_when_no_context_active.is_any_active()replaces thequeue.get_notifiers()signal the old registry gave the write-interceptor gate; with nodispatch-counter-collection context active it must report inactive. Requires no GPU or HSA
runtime, so it runs everywhere.
counters/tests/core.cpp, updated offiterate_callbacksto the new hook surface.that the migration preserved behaviour.
Test Result
CI is re-running after the 12 Aug rebase; the results below describe the state before it, which
the rebase should not have altered since it touched only a CMake source list.
Known failures, fixed in #9586. Two pre-existing integration tests fail on this PR and pass on
develop:Both are pause/resume counter-collection tests, and pause is
stop_context— which is exactly thecompletion-routing regression @bwelton identified. They are an objective target for #9586 rather
than tests authored alongside the fix.
Not caused by this PR, for whoever triages the check list:
Analyze (actions)andAnalyze (python)fail with a workflow template error rather than afinding —
.github/workflows/rocprofiler-sdk-codeql.yml, "Unexpected value ''". The jobs neverrun, and this affects every PR in the repository.
therock-pr-botreportsTimed out waiting for required checks to completeand is downstream ofthe above.
6.2 • ubuntu-22.04 • aqlprofile-internalfailed in image setup with aGPGKeyTemporarilyNotFoundErrorand an HTTP 500 from the package host.
For calibration: #8790 and #8895 are approved with zero real failures, and their check lists
contain exactly those same three repo-wide jobs.
Submission Checklist