rocprofiler-sdk: switch PC sampling per kernel replay pass by amd-vkale · Pull Request #14928 · ROCm/rocm-systems

amd-vkale · 2026-11-03T16:50:00Z

Motivation

Allow kernel replay tools to place PC sampling and dispatch counters on separate passes. This avoids running both clock-gating-sensitive services together on MI2xx/MI3xx.

Technical Details

Apply localized PC-sampling context overrides at replay pass boundaries.
Stop and flush an agent's ROCr PC-sampling session before submitting a disabled pass.
Restart the session only for passes that enable PC sampling and restore the globally requested state after replay.
Preserve marker/completion CID pairing while the hardware session is suspended.
Serialize session transitions, wait out another thread's ROCr call, and return a real flush failure.
A failed start or stop puts the active context slot back so the public API can retry.
PC-sampling replay samples and the strict sample-count cases stay out of CI. Run them locally on a rebootable machine.

CI tests

Host unit tests in pcs-test cover sync, restore, sticky stop, and a global start that must not override a replay suspension. They do not touch the GPU.

One integration test stays enabled when PC sampling is available: test-kernel-replay-local-context-pc-sampling-stop-after-0. It only checks that the local stop ran.

Local experiments

Disabled in ctest. On a rebootable MI300X/MI325 or MI200/MI300A machine, enable them and set KR_LC_REQUIRE_SAMPLES=1:

test-kernel-replay-local-context-pc-sampling-all-passes
test-kernel-replay-local-context-pc-sampling-stop-from-start
test-kernel-replay-local-context-pc-sampling-stop-then-restart
test-kernel-replay-local-context-counters-pc-sampling-pass-0
test-kernel-replay-local-context-counters-pc-sampling-stopped
kernel-replay-counters-then-pc-sampling
kernel-replay-services-first and kernel-replay-services-last

Issue Tracking

JIRA ID: AIPROFSDK-1171

Test Plan

clang-format 11 and cmake-format on the touched files.
CI runs the host pcs-test cases and the one loose toggle check.
Sample-count and counters-plus-PC-sampling runs stay local.

Test Result

Formatting checks pass locally.
This VM has no ROCm/HIP install, so the GPU experiments were not run here.

Submission Checklist

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

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

therock-pr-bot · 2026-11-03T16:50:46Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/rocprofiler-sdk/samples/kernel_replay/counters_then_pc_sampling_client.cpp`, `projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp`, `projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp`, `projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/kernel_replay/local_context.hpp`, `projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/kernel_replay/tests/local_context.cpp` (+9 more); no test file found
🚫 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?

Copilot

🟡 Changes recommended

Unresolved transition races and unpropagated stop/flush failures can allow sampling to overlap counter replay, leave required drains incomplete, or keep sampling running after a reported successful stop.

5 open issues

What changed in this PR

This PR adds per-pass PC sampling control to rocprofiler-sdk kernel replay, allowing custom tools to separate PC sampling from dispatch counters and avoid MI2xx/MI3xx clock-gating conflicts.

Changes:

Applies replay-local overrides to agent sampling sessions, with stop-time flushing and restoration after replay.
Adds session-transition state and updates host-side tests for effective sampling requests.
Enables supported sampling replay samples and documents the new behavior.

File	Description
`projects/rocprofiler-sdk/tests/kernel-replay-local-context/CMakeLists.txt`	Updates the sampling test comment.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/types.hpp`	Adds session synchronization and suspension state.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/local_context.cpp`	Tests effective replay-local sampling requests.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/service.hpp`	Declares replay synchronization and restoration helpers.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/service.cpp`	Implements session transitions and replay overrides.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/queue_hooks.cpp`	Clarifies correlation-reference retirement in a comment.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.hpp`	Declares status-returning per-session operations.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp`	Implements per-session start, stop, and flush handling.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/kernel_replay/tests/local_context.cpp`	Updates the simulated sampling consumer expectations.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/kernel_replay/local_context.hpp`	Documents pass-boundary sampling control.
`projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp`	Synchronizes sampling before passes and restores it afterward.
`projects/rocprofiler-sdk/source/docs/conceptual/kernel_replay/kernel_replay_callback_api.md`	Explains sampling isolation and service limits.
`projects/rocprofiler-sdk/source/docs/conceptual/kernel_replay/kernel_replay_callback_api_design.md`	Updates the sampling override design description.
`projects/rocprofiler-sdk/source/docs/api-reference/kernel_replay.rst`	Documents supported per-pass sampling semantics.
`projects/rocprofiler-sdk/samples/kernel_replay/README.md`	Describes enabled sampling and service-order samples.
`projects/rocprofiler-sdk/samples/kernel_replay/counters_then_pc_sampling_client.cpp`	Filters counter collection to the replay workload.
`projects/rocprofiler-sdk/samples/kernel_replay/CMakeLists.txt`	Removes the blanket sampling replay test disablement.

_{Review effort: Balanced}

+
+    // Flush internal PC sampling buffers (ROCr + 2nd level trap handler buffers) before another
+    // service can alter clock gating for this agent.
+    return flush_internal_agent_buffers(agent_session);


+    // A global context start must not override a replay-local suspension.
+    active = active && !session->replay_suspended.load();
+
+    auto state_lock = std::unique_lock{session->state_mutex};


+    if(session->active.load() == active) return ROCPROFILER_STATUS_SUCCESS;
+    if(session->transition_in_progress)
+        return session->transition_target == active ? ROCPROFILER_STATUS_SUCCESS
+                                                    : ROCPROFILER_STATUS_ERROR;


+        if(status != ROCPROFILER_STATUS_SUCCESS)
+        {
+            service->enabled.store(true);
+            set_service_sessions_active(service, true);
+        }


+            service->enabled.store(false);
+            set_service_sessions_active(service, false);
+        }
+        return status;


vlaindic · 2026-11-04T16:57:40Z

Question on concurrency between kernel-replay PC-sampling transitions and global context start/stop

sync_replay_service/restore_replay_service (service.cpp) and a real rocprofiler_context_start/stop() on the same context can both call set_session_active() on the same PCSAgentSession concurrently — one from the replay thread (holding agent_replay_mutex for that agent), one from whatever thread calls the context lifecycle API (no shared lock between the two paths). set_session_active resolves a conflicting concurrent transition by returning ROCPROFILER_STATUS_ERROR to the loser rather than blocking (reasonable, to avoid deadlocking against a flush-triggered tool callback) — but two things downstream make that outcome more serious than "retry later":

In queue.cpp, both sync_replay_service and restore_replay_service are wrapped in ROCP_FATAL_IF(... != SUCCESS). If the replay side loses that race, the whole process aborts. This call sits inside the per-pass loop, which can be indefinite (replay_plan.indefinite), so the exposure isn't a one-shot risk per kernel launch — it's a fresh chance per pass, for as long as that replay loop runs, if anything else in the process is starting/stopping this context concurrently.
In context.cpp, start_context() propagates pc_sampling::start_service()'s status to its caller, but stop_context() discards pc_sampling::stop_service()'s return value entirely. If the global-stop side loses that race, stop_service()'s internal rollback re-enables the sessions it just tried to stop — but stop_context() still unconditionally clears the context's active slot and returns ROCPROFILER_STATUS_SUCCESS, so the caller has no way to know PC sampling didn't actually stop.

Is this interleaving (an app/tool thread calling rocprofiler_context_stop/start on a context while a kernel-replay loop is mid-pass on one of that context's agents) considered out of scope / an accepted limitation for this change, or should it be handled more gracefully (e.g. not treating a lost race as fatal in the replay path, and/or propagating stop_service's status in stop_context())? Worth documenting either way even if it's deemed acceptable for now.

Optional: if this is accepted as a known gap rather than fixed now, it might be worth a regression test that drives a long/indefinite kernel-replay loop on one thread while another thread repeatedly calls context stop/start on the same context, to pin down the actual behavior (does it abort, and does stop_context() ever return success while sampling is still active).

vlaindic

Reviewed functionality, concurrency, and test design for the new per-session PC-sampling state machine this PR introduces (PCSAgentSession::state_mutex/transition_in_progress/transition_target/active/replay_suspended). I verified the Copilot-flagged lines against the code and traced them end-to-end; all four are real. I also found two issues Copilot didn't flag, in context/context.cpp (not part of this diff, so noted here rather than inline) and in test coverage.

Critical

context::stop_context()/start_context() diverge from the new pc_sampling rollback status (source/lib/rocprofiler-sdk/context/context.cpp:766 and :622/:653, not touched by this diff)

stop_context() clears the active-context slot in phase 3 before calling pc_sampling::stop_service() in phase 4, and discards its return status (rocprofiler::pc_sampling::stop_service(_expected); at line 766). If the new rollback in stop_service fails and re-enables sampling, rocprofiler_stop_context() still reports ROCPROFILER_STATUS_SUCCESS while the hardware session is still running, and a retry can't find the context to stop again (its active slot is already cleared).

Symmetrically, start_context() publishes the context into active_contexts at line 622 before pc_sampling::start_service() runs at line 653. If start_service fails and rolls back (service->enabled.store(false)), the context is still marked active, so a later rocprofiler_start_context() short-circuits to ROCPROFILER_STATUS_SUCCESS at line 545 ("already active") without retrying the hardware start.

Thread the pc_sampling service status into the active-slot transition in both functions, or explicitly document/handle the resulting divergent state.

Important

Test gap: the integration test for this exact scenario wasn't updated (tests/kernel-replay-local-context/client.cpp, driving test-kernel-replay-local-context-pc-sampling-stop-after-0)

This test still only asserts that the local stop/start toggle calls succeeded (g_local_stops/g_local_starts), not that the actual sample count (g_pcs_samples) reflects suspension -- unlike the SPM check right above it (check_exact("spm", kept("spm"), g_spm_records.load())) and the ATT check, both of which assert on real collected-data counts. The stderr message even still reads "agent-wide service; local start/stop is a no-op for collection", which this PR makes false. Please extend the pc-sampling block to assert on g_pcs_samples the same way SPM/ATT do.

No concurrency test for the race this PR's own comments call out

Nothing exercises a global rocprofiler_start_context()/stop_context() racing sync_replay_service()/restore_replay_service() on the same agent session from two threads. This is exactly the scenario behind the two critical findings on set_session_active below -- a host-side unit test driving set_session_active from two threads with an injected delay (or a TSAN-covered stress test) would have caught them.

Suggestion

PC sampling is already gated by the existing ROCPROFILER_PC_SAMPLING_BETA_ENABLED experimental flag, which should cover this behavior change. Worth a one-line mention in the PR description confirming that, since the description doesn't currently reference it.

vlaindic · 2026-11-04T18:00:38Z

+    if(!session) return ROCPROFILER_STATUS_SUCCESS;
+
+    // A global context start must not override a replay-local suspension.
+    active = active && !session->replay_suspended.load();


Critical: TOCTOU race lets a global start resurrect PC sampling during a counter pass.

replay_suspended is read here before state_mutex is acquired. A thread calling rocprofiler_start_context() can read replay_suspended == false and then stall before taking the lock; meanwhile the replay loop's sync_replay_service() publishes the suspension and successfully stops the agent session ahead of a counter pass. The stalled start then resumes with its stale active=true and restarts PC sampling mid-counter-pass -- the exact MI2xx/MI3xx clock-gating hazard this PR exists to prevent.

Compute active && !session->replay_suspended.load() after acquiring state_mutex so it observes an already-published suspension.

vlaindic · 2026-11-04T18:00:38Z

+    active = active && !session->replay_suspended.load();
+
+    auto state_lock = std::unique_lock{session->state_mutex};
+    if(session->active.load() == active) return ROCPROFILER_STATUS_SUCCESS;


Critical: these shortcuts don't reflect whether a real transition actually completed.

session->active.load() == active (line 67) compares against the pre-transition cached flag, and the transition_in_progress branch (lines 68-70) returns success/error purely by comparing targets -- neither waits for the in-flight ROCr start/stop/flush to finish.

Consequences: (a) a disabled replay pass can be told "stop succeeded" and submit before the real flush completes, which breaks this PR's core guarantee; (b) a benign race between a tool's own start/stop_context call and the replay loop can return ROCPROFILER_STATUS_ERROR for a perfectly legitimate request -- and since hsa/queue.cpp wraps sync_replay_service/restore_replay_service in ROCP_FATAL_IF, that aborts the whole profiled process.

Wait for the in-flight transition to settle and return its real post-transition outcome instead of guessing from pre-transition state.

vlaindic · 2026-11-04T18:00:38Z

+
+    // Flush internal PC sampling buffers (ROCr + 2nd level trap handler buffers) before another
+    // service can alter clock gating for this agent.
+    return flush_internal_agent_buffers(agent_session);


Critical: the flush-failure status this now depends on is always swallowed.

flush_internal_agent_buffers() (same file, a few lines below pc_sampling_session_stop) does std::runtime_error("Fail to flush ROCr's buffer explicitly"); on a failed hsa_ven_amd_pcs_flush_fn call -- that constructs-and-discards an exception (never thrown, never logged) and the function unconditionally returns ROCPROFILER_STATUS_SUCCESS regardless of the real HSA result.

That return value is exactly what this line now forwards up through set_session_active -> sync_replay_service as the signal that "stop + flush completed." A genuine flush failure is therefore silently reported as success, letting a clock-gating-sensitive replay pass proceed on an unflushed buffer.

Propagate the failure (log it and return ROCPROFILER_STATUS_ERROR) instead of discarding the exception.

vlaindic · 2026-11-04T18:00:38Z

+    // can temporarily suspend one agent's ROCr session without changing that global state.
+    // Transitions are serialized because ROCr start/stop and the stop-time flush must complete
+    // before a replay pass can use another clock-gating-sensitive service.
+    std::mutex        state_mutex{};


Optional, but would really help future readers: this new state machine (state_mutex/transition_in_progress/transition_target/active/replay_suspended) has four entry points -- global start/stop (start_service/stop_service), and replay-local suspend/restore (sync_replay_service/restore_replay_service) -- each commented locally in service.cpp but with no single consolidated description of the end-to-end lifecycle (session creation in configure_pc_sampling_service -> normal start/stop -> replay suspend/restore -> teardown in service_fini). A short workflow comment block here (or at the top of service.cpp) covering all of that in sequence would make this subsystem much easier to reason about -- especially given the two critical concurrency findings on set_session_active in this review.

vlaindic

Mainly review by the help of AI. Overall, I think the redesign is sound, but there are some edge cases we might want to ensure are properly covered. Please check my previous comments.

@samcrater-amd Please take a look if you can, as another pair of fresh eyes is welcomed.

Add host unit tests for sync/restore and suspend-vs-global-start, expand the local-context PC sampling matrix, and require samples on enabled passes. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Read replay_suspended under the session lock, wait out another thread's ROCr transition, and return a real flush failure instead of discarding it. A failed start or stop now puts the active context slot back so the public API can retry. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

github-actions · 2026-11-05T09:43:15Z

Code Coverage Report

Tests Only

Samples Only

Tests + Samples

Leave sample-count and counters-plus-PC-sampling cases disabled for local runs. CI keeps the host sync tests and one toggle check, and the source files match clang-format 11. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

feat(rocprofiler-sdk): switch PC sampling per replay pass

bb87eed

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

amd-vkale requested review from a team as code owners November 3, 2026 16:50

Copilot AI balanced review requested due to automatic review settings November 3, 2026 16:50

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

Copilot started reviewing on behalf of amd-vkale November 3, 2026 16:50

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

systems-assistant Bot added the organization: ROCm label Nov 3, 2026

vlaindic reviewed Nov 4, 2026

View reviewed changes

vlaindic requested changes Nov 4, 2026

View reviewed changes

cursoragent and others added 2 commits November 5, 2026 08:13

test(rocprofiler-sdk): cover PC sampling replay pass switching

8bf9818

Add host unit tests for sync/restore and suspend-vs-global-start, expand the local-context PC sampling matrix, and require samples on enabled passes. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

cursor Bot mentioned this pull request Nov 6, 2026

[Tracking] rocprofiler-sdk: kernel replay and range replay #15178

Closed

test(rocprofiler-sdk): keep PC sampling replay checks out of CI

16b75eb

Leave sample-count and counters-plus-PC-sampling cases disabled for local runs. CI keeps the host sync tests and one toggle check, and the source files match clang-format 11. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

amd-vkale mentioned this pull request Nov 6, 2026

[Tracking] rocprofiler-sdk: kernel replay (experimental) #15179

Open

cursor Bot mentioned this pull request Nov 7, 2026

[DO NOT MERGE] Integration preview: range replay + kernel replay perf stack on develop #10193

Open

Conversation

amd-vkale commented Nov 3, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

CI tests

Local experiments

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

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

Uh oh!

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

vlaindic commented Nov 4, 2026

Uh oh!

vlaindic left a comment

Choose a reason for hiding this comment

Critical

Important

Suggestion

Uh oh!

vlaindic Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

vlaindic Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

vlaindic Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

vlaindic Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

vlaindic left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 5, 2026

Code Coverage Report

Code Coverage Report

Tests Only

Samples Only

Tests + Samples

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amd-vkale commented Nov 3, 2026 •

edited by cursor Bot

Loading

therock-pr-bot Bot commented Nov 3, 2026 •

edited

Loading