Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
54f04db to
c3d0f74
Compare
c3d0f74 to
923ac10
Compare
…back registry Isolates the PC-sampling 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. 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>
923ac10 to
ec87a2f
Compare
vlaindic
left a comment
vlaindic
left a comment
There was a problem hiding this comment.
Overall, the PR LGTM, thanks! I added a few comments and basically I propose to think about a master hook that will forward the calls down the chain based on available services. If you think that this kind of implementation is not optimal, then please disregard the comment.
Also, I'd recommend you to run the existing PC sampling tests on MI300A machine to ensure that there's no regressions introduced by this PR. Thanks in advance.
Also adding AI review comments:
PR #8895 Review — refactor(rocprofiler-sdk): migrate PC sampling off the per-queue callback registry
Files changed: 9 | +205 / -31 | Branch: users/vkale/remove-callbacks-pc-sampling → develop
Overall Assessment
This is a clean, well-scoped refactor. The approach (explicit hook call in the signal handler instead of a registry-based callback) is sound, the #if ROCPROFILER_SDK_HSA_PC_SAMPLING gating is consistent, and the unit test for the no-runtime path is a good addition. A few concrete issues below, none of them blockers if addressed.
Findings
⚠️ IMPORTANT (1) — [hygiene] Missing JIRA ID and empty Test Plan / Test Result
Issues Detected
- The
## Issue Trackingsection contains only the template placeholder comments; no actual JIRA ID or GitHub issue number is present. ## Test Planand## Test Resultare both empty (only template comments remain).
Suggested Actions
- Add a JIRA ID in the format
ABC-1234to the Issue Tracking section, or if none exists, add a one-liner explaining why (e.g., pure refactor tracked only via parent PR #8730). - Fill in the Test Plan section: at minimum state which existing test suite was run (
pcs-testunit tests, any end-to-end PC sampling test), and what the pass/fail outcome was.
Risk / Impact Assessment
- Low in isolation — the description body is self-contained enough for reviewers familiar with the callback refactor. But the empty Test Plan is concerning for anyone approving a GPU signal-path change without knowing if the existing PC sampling test suite was exercised.
⚠️ IMPORTANT (2) — [clarity] Copyright headers on new files still read 2025
Issues Detected
queue_hooks.cppline 3:Copyright (c) 2025 Advanced Micro Devices, Inc.queue_hooks.hppline 3:Copyright (c) 2025 Advanced Micro Devices, Inc.tests/queue_hooks_test.cppline 3:Copyright (c) 2025 Advanced Micro Devices, Inc.
All three are newly added files in this PR.
Suggested Actions
- Update copyright header in
queue_hooks.cppfrom2025to2026— file is newly added in this PR. - Update copyright header in
queue_hooks.hppfrom2025to2026— file is newly added in this PR. - Update copyright header in
tests/queue_hooks_test.cppfrom2025to2026— file is newly added in this PR.
Risk / Impact Assessment
- Low — legal / compliance housekeeping, not runtime impact.
⚠️ IMPORTANT (3) — [generic] Mutable-reference copy in signal_completion_hook is a workaround that should be documented more prominently
Issues Detected
In queue_hooks.cpp:57-59:
// kernel_completion_cb takes a mutable reference to the kernel packet even
// though it does not modify it; copy to bind a non-const lvalue.
auto kern_pkt = kernel_packet;
hsa::kernel_completion_cb(queue.get_agent().get_rocp_agent(), kern_pkt, *session);The comment explains the workaround, but the root issue (the kernel_completion_cb signature takes rocprofiler_packet& despite not modifying the argument) is in hsa_adapter.cpp and hsa_adapter.hpp, not here. This cross-file mismatch is easy to miss in a future signature cleanup.
Suggested Actions
- Add a
// TODO:or// FIXME:comment at thekernel_completion_cbdeclaration inhsa_adapter.hpp:62noting that the parameter should beconst rocprofiler_packet&— this makes the mismatch visible at the definition site, not just at the sole callsite inqueue_hooks.cpp. - Alternatively, fix the
kernel_completion_cbsignature to takeconst rocprofiler::hsa::rocprofiler_packet&now (it is already marked/* kernel_pkt *// unused in the function body), removing the need for the copy entirely.
Risk / Impact Assessment
- Low — no runtime risk, but the defensive copy adds unnecessary overhead on every kernel completion when PC sampling is active.
💡 SUGGESTION (1) — [generic] Typo in PR description
Issues Detected
- "to isolatesthe" in the Motivation section — missing space and stray "s": should be "to isolate the".
Suggested Actions
- Fix before marking ready to review.
💡 SUGGESTION (2) — [hygiene] Test adequacy: only the #else branch is exercised
Issues Detected
queue_hooks_test.cpp tests is_configured_on_agent when ROCPROFILER_SDK_HSA_PC_SAMPLING is not set (or the service is unconfigured). The #if ROCPROFILER_SDK_HSA_PC_SAMPLING > 0 path (delegating to is_pc_sample_service_configured) has no unit-test coverage at all.
Suggested Actions
- This is acceptable for a no-GPU unit test. Note it explicitly in the test comment so a future contributor knows the HSA path is only covered by integration tests (e.g., in the existing PC sampling end-to-end suite).
- Consider adding a second test case:
is_configured_on_agentreturnsfalsefor an unrecognized agent even when HSA PC sampling is compiled in, using an existing mock or stub foris_pc_sample_service_configured.
Risk / Impact Assessment
- Low — the no-runtime path is thoroughly guarded. The real risk would be a regression in the HSA path, which is only caught by end-to-end tests.
💡 SUGGESTION (3) — [clarity] PR is marked "Not ready to Review" — confirm before requesting full review
The label Not ready to Review is still set. If the PR is ready for review, remove it; otherwise ping when ready.
Summary Table
| # | Severity | Tag | File | Issue |
|---|---|---|---|---|
| 1 | [hygiene] | PR description | Missing JIRA ID + empty Test Plan/Result | |
| 2 | [clarity] | 3 new files | Copyright year 2025 → should be 2026 | |
| 3 | [generic] | queue_hooks.cpp, hsa_adapter.hpp |
Unnecessary defensive copy / stale mutable signature | |
| 4 | 💡 SUGGESTION | [generic] | PR description | Typo "isolatesthe" |
| 5 | 💡 SUGGESTION | [hygiene] | queue_hooks_test.cpp |
Test only covers #else branch |
| 6 | 💡 SUGGESTION | [clarity] | PR labels | "Not ready to Review" label still set |
| // PC sampling completion is no longer routed through the per-queue | ||
| // callback registry; invoke its hook explicitly. | ||
| pc_sampling::signal_completion_hook(queue_info_session.queue, | ||
| packet.kernel_packet, | ||
| _session, | ||
| packet, | ||
| packet.instrumentation_packets, | ||
| dispatch_time); | ||
|
|
There was a problem hiding this comment.
I'm wondering whether we should have a master hook that will be invoked here. Then a master hook would check what service is enabled, and then invoke a specific hook.
There was a problem hiding this comment.
Posting note: drafted with Cursor, but I (@amd-vkale) own this post.
Ack, and apologies this sat unanswered — it's a fair design question and deserved a reply.
I'd rather not, and the reason is that a master hook is close to the thing this series is removing. The per-queue callback registry already was a dispatcher: one registration point that checked which services were interested and fanned out. Replacing it with a different dispatcher that checks which service is enabled and fans out would keep the indirection and just change where it lives. The value of the explicit call sites is that reading queue.cpp tells you exactly which services participate at which phase, with no runtime table to consult.
It would also create a dependency hub. A master hook has to know about all four services, so hsa/ would gain an include edge to counters/, thread_trace/, spm/ and pc_sampling/ — today the dependency runs the other way, and each service's hook self-filters cheaply on its own state.
Where I think you're pointing at something real: the four call sites are currently adjacent and hand-maintained, so adding a fifth service means remembering to edit two places in queue.cpp. If that's the concern, I'd rather address it with a compile-time list than a runtime dispatcher. Happy to do that as a follow-up once all four services have landed and the registry is deleted — at that point the shape of the call sites is stable enough to factor. Does that work, or do you feel strongly it should be in this PR?
| TEST(pc_sampling_queue_hooks, is_configured_on_agent_unconfigured) | ||
| { | ||
| rocprofiler_agent_id_t agent_id; | ||
| agent_id.handle = 123456; | ||
| EXPECT_FALSE(rocprofiler::pc_sampling::is_configured_on_agent(agent_id)); | ||
| } |
There was a problem hiding this comment.
Should we have a similar test for the case when PC sampling is configured?
There was a problem hiding this comment.
Posting note: drafted with Cursor, but I (@amd-vkale) own this post.
Ack — and you're right that the positive case is missing here. Being straight about why rather than claiming it's covered: configuring a PC sampling service needs a real HSA runtime and an agent that supports PC sampling, and these unit tests deliberately run without either. That's what lets them execute on any CI runner. So the configured path can't be exercised at this level without standing up a fake agent and service registry, which is a meaningful amount of scaffolding for this PR.
What the unit tests here do cover is the negative path, which is the one this PR actually changes: is_configured_on_agent() is the replacement for the queue.get_notifiers() signal the old registry provided to the write interceptor gate, so "no service configured means the interceptor stays off" is the regression I most want pinned. The configured path is unchanged from develop and is exercised by the PC sampling integration tests.
If you'd like the positive unit test anyway I'll build the scaffolding — I just didn't want to add a test that fakes enough of the runtime that it stops telling us anything. Your call.
| namespace pc_sampling | ||
| { | ||
| bool | ||
| is_configured_on_agent(rocprofiler_agent_id_t agent_id) |
There was a problem hiding this comment.
Don't mind having this function, but maybe seems similar to place #if ROCPROFILER_SDK_HSA_PC_SAMPLING > 0 inside the is_pc_sample_service_configured as that function is called by multiple places and already has a guard whether HSA is loaded.
There was a problem hiding this comment.
Posting note: drafted with Cursor, but I (@amd-vkale) own this post.
Got it. I tried exactly that and it doesn't link, which is worth explaining because the reasoning isn't visible from the diff.
The guard can't move into is_pc_sample_service_configured because that function is defined in service.cpp, and service.cpp is added to the object library after the early return in pc_sampling/CMakeLists.txt. queue_hooks.cpp is added before it, deliberately, because the write interceptor calls these shims unconditionally and they must stay linkable when PC sampling is compiled out. So in that configuration is_pc_sample_service_configured doesn't exist as a symbol at all, and a guard inside it would never be compiled. The #if has to sit at the call site.
You're right that the result reads oddly, so I've put the reason in the code rather than leaving the next reader to rediscover it — there's now a comment above is_configured_on_agent explaining the CMake ordering constraint, and a matching one in pc_sampling/CMakeLists.txt at the target_sources line saying why it precedes the early return.
Separately, and flagging it here since it touches this file: signal_completion_hook is now kernel_dispatch_phase_exit_hook, matching the naming @vlaindic asked for on #8891 and the convention counter collection already uses.
|
Mergeability (local review + GPU unit tests, 2026-08-18) No stacked PRs. This branch is the whole change, and it is merge-ready.
|
|
Ready for review. There is no per-agent stacked PR for PC sampling — configure is already per-agent. This branch is unchanged ( |
Co-authored-by: Vivek Kale <vivek.kale@amd.com>
|
@copilot resolve the merge conflicts in this pull request |
… 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
the
github actions
Marker commit only; no file changes. It keeps ROCm#8895 open as a distinct pull request now that its base is users/vkale/remove-callbacks-pc-sampling--with-cursor-fixes, which already contains every commit on this branch.
|
@copilot please fix all conflicts in this PR |
…back registry (#11969) Stop routing PC sampling kernel completions through the HSA queue controller's per-queue callback registry. Instead, call `pc_sampling::kernel_dispatch_phase_exit_hook` explicitly from `AsyncSignalHandler` in `hsa/queue.cpp`, which forwards to `kernel_completion_cb` after verifying the service is configured on the dispatch's agent. Add `pc_sampling::is_configured_on_agent` to the `no_real_consumers` gate in `WriteInterceptor`, replacing the notifier count that the old `add_callback` registration used to bump. This keeps marker-packet injection alive for PC-sampling-only runs on configured agents. Both hooks live in a new `queue_hooks.cpp`, added before the HSA-runtime-version early return in `pc_sampling/CMakeLists.txt` and guarded internally by `ROCPROFILER_SDK_HSA_PC_SAMPLING`, so they link as no-ops when HSA PC sampling support is unavailable. This is completion-only: marker injection stays inline in `WriteInterceptor`, with no batching or enqueue-hook change. Counters, thread trace, and SPM keep using the registry until their sibling PRs land. Hook names follow the series' `kernel_dispatch_phase_*_hook` convention. Other changes: - Move `kernel_completion_cb` out of the anonymous namespace and declare it in `hsa_adapter.hpp`; make its `kernel_pkt` param const. - Correct its comment: it runs on ROCr's async signal handler thread, not in the write interceptor. - Remove `intercept_cb_id` from `PCSAgentSession` (no longer needed). - Declare `is_hsa_initialized()` in `service.hpp` (was missing). - Remove stale `kernel_completion_cb` extern from test internals header. - Add unit tests for unconfigured gate, null-session no-op, and seeded-session configured path. Part of the per-service callback removal series. * Sibling PRs: #11967 (thread trace), #11968 (SPM), #11970 (counters) * Combined view: #12135 * Related: #8895, #8586, #8730 * Includes the completion-hook fix reviewed in #11871. JIRA ID: AIPROFSDK-1019 Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
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. |
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