Repository navigation
Conversation
…back registry Isolates the PC-sampling 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. PC sampling no longer registers a queue_callbacks_t with the HSA queue controller in pc_sampling_service_finish_configuration. Instead: - pc_sampling::signal_completion_hook (new pc_sampling/queue_hooks.{hpp,cpp}) delivers kernel completion to the CID manager; it is called explicitly from the HSA async signal handler in hsa/queue.cpp. kernel_completion_cb is moved out of the anonymous namespace and declared in hsa_adapter.hpp. - pc_sampling::is_configured_on_agent replaces the queue.get_notifiers() signal the old registration used to provide, so the write interceptor still injects the marker packet for PC-only runs. It is always linkable (guarded by ROCPROFILER_SDK_HSA_PC_SAMPLING) and compiled before the pc_sampling CMake early-return so it links even without HSA PC sampling support. The marker packet injection in the write interceptor is unchanged (it was never registry-based). Removes the now-unused intercept_cb_id from PCSAgentSession and adds a queue_hooks unit test. The per-queue callback registry is retained for the remaining service (thread-trace/spm/counters migrate in their own PRs). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Vivek Kale <vivek.kale@amd.com>
… at the shim Review asked whether the ROCPROFILER_SDK_HSA_PC_SAMPLING guard could move into is_pc_sample_service_configured instead of living in a separate wrapper. It cannot: queue_hooks.cpp is added to the object library before the early return in CMakeLists.txt so it is always built, while service.cpp defines that function after the early return and is not built when PC sampling is compiled out. Moving the guard would leave the write interceptor with an undefined symbol in that configuration. Record the constraint at the shim so the next reader does not retry the change, and bump the copyright year on the new files to 2026. (cherry picked from commit fa38a01)
…completion hook Three review findings on the completion path: The hook took the queue by reference, so the unit test covering the null-session early return had to synthesize one by dereferencing a null pointer. That is undefined behaviour at argument construction even though the hook returns before touching the queue. Take the queue by pointer instead, which lets the test pass nullptr honestly and documents that the callee must not assume a queue exists. kernel_completion_cb took a mutable reference to a packet it never modifies, forcing the hook to copy the kernel packet on every completion. Make the parameter const and drop the copy. The configured-agent check lived inside kernel_completion_cb, after that copy, so a process with any interceptor active paid the cost on every dispatch even when PC sampling was never configured. The registry-based path cost nothing until a callback was registered. Check is_pc_sample_service_configured in the hook before doing any work, restoring that property; kernel_completion_cb has one caller, so the check is not lost. Also drops a stale kernel_completion_cb declaration in pc_sampling_internals.hpp that still described the removed registry signature, and corrects two comments that referred to the old callback mechanism. (cherry picked from commit c93f8dd)
…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>
…emove-callbacks-pc-sampling 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/pc_sampling/tests/CMakeLists.txt
…sampling Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
project: rocprofiler-sdk
github actions
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
Author
|
Superseded: recreating from the |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review effort: Lite
Findings: None
What changed in this PR
Refactors PC sampling completion delivery away from the per-queue callback registry while preserving marker injection and adding hook coverage.
Changes:
- Adds explicit PC sampling configuration and completion hooks.
- Removes callback registration state and updates HSA completion handling.
- Adds unit tests and pins compatibility workflow CMake.
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/types.hpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/queue_hooks_test.cpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/pc_sampling_internals.hpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/CMakeLists.txt | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/queue_hooks.hpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/queue_hooks.cpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.hpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp | Updated as part of this pull request. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/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. |
| .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.
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
The purpose of this PR is to isolate the PC-sampling 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. Helps toward but independent of the kernel replay feature in #7960.
Underlying Problem
PC sampling currently registers a per-queue completion callback with the HSA queue controller so kernel completions reach the CID manager. That ties completion delivery to the shared per-queue callback registry even though marker-packet injection was never registry-based.
This PR:
pc_sampling_service_finish_configurationand delivers completion explicitly viapc_sampling::signal_completion_hookfrom the async signal handler.get_notifiers()gate withis_configured_on_agent()so PC-only runs still enter the write interceptor and receive marker packets on configured agents.PC sampling has no enqueue hook — marker injection stays inline in
WriteInterceptor. This is the smallest migration: completion-only, no batching change. The per-queue callback registry remains for counters, thread trace, and SPM until those services land in their own PRs.Notes on Bigger Picture
Sibling migrations: thread trace (#8790), SPM (#8887), counter collection (#8891).
Kernel replay (#7960) benefits from callback removal but this PR is independent and behavior-preserving on its own.
Technical Details
Permalinks pin to
ec87a2f.Hooks and call sites
is_configured_on_agent: queue_hooks.cpp#L36-L45signal_completion_hook→kernel_completion_cb: queue_hooks.cpp#L47-L66, hsa_adapter.cpp#L133-L150no_real_consumersgate (per-agent, not global): queue.cpp#L318-L321queue_idremoved from PC sampling types (types.hpp)PC sampling does not use
client_ids.hppproducer tags on this branch (marker injection is unchanged). Nois_any_active()— the gate is per-agent configuration state.Reviewer Guide
add_callback/remove_callbackin hsa_adapter.cpp configure path.AsyncSignalHandlercallspc_sampling::signal_completion_hookafter registry walk (queue.cpp#L172-L179).no_real_consumersusesis_configured_on_agent(agent)per dispatch queue's agent (queue.cpp#L318-L321) — PC-sampling-only on one GPU must still intercept that GPU's dispatches.is_configured_on_agent(queue.cpp).kernel_completion_cbstill guards onis_pc_sample_service_configuredand correlation id (hsa_adapter.cpp#L138-L149).#if ROCPROFILER_SDK_HSA_PC_SAMPLING— hook stubs must link when support is off (queue_hooks.cpp).Questions to answer:
With PC sampling configured on GPU-0 only, does GPU-0 still get marker injection and completion CID retirement?
With no PC sampling configured, does
is_configured_on_agentreturn false for all agents?Out of scope: enqueue hook extraction;
client_ids.hpptagging; per-agent scoping API; deletingQueue::_callbacks.Issue Tracking
JIRA ticket:
AIPROFSDK-1019
Test Plan
Test Result
Unit gate test as above. Treat the Checks tab as source of truth for CI.
Submission Checklist