Skip to content

refactor(rocprofiler-sdk): address review feedback on #8891 for remove-callbacks-counters - #9586

Merged
amd-vkale merged 12 commits into
users/vkale/remove-callbacks-countersfrom
users/vkale/remove-callbacks-counter-collection-cursor-fixes
Aug 18, 2026
Merged

amd-vkale merged 12 commits into
users/vkale/remove-callbacks-countersfrom
users/vkale/remove-callbacks-counter-collection-cursor-fixes

Conversation

@amd-vkale

@amd-vkale amd-vkale commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

This PR implements the fix @bwelton requested in his review of PR #8891 on 23 July, and intends to implement all of the comments and suggestions in the PR #8891 review by @vlaindic.

@bwelton identified that the migration changed completion routing from provenance-based to liveness-based, and prescribed the fix: route by packet provenance rather than current activeness, by iterating registered rather than only active contexts. That is what this PR does. Credit to @bwelton for identifying the regression.

@vlaindic 's review of PR #8891 asked for a number of important changes as well including function renaming of write and signal completion hooks, the enum and the copyright years. These are all also addressed here.

The regression noted by @bwelton, restated, is that the per-queue callback registry fired an in-flight dispatch's completion regardless of whether its context was still active, and the old stop_context relied on that. The new completion hook re-queried get_active_contexts at completion time, while context::stop_context nulls the active slot at context/context.cpp:371 before calling counters::stop_context. So a kernel dispatched while the context was active but completing after rocprofiler_stop_context() returns found zero active contexts, completed_cb never ran, the counter record was silently lost, and the packet_return_map entry leaked with possible misassociation on packet-address reuse.

It is not hypothetical as #8891's CI shows two test failures out of 955 on gfx94X, and both are pause/resume counter-collection tests:

rocprofv3-test-roctx-pause-resume-tracing.test_counter_collection_data
  E assert 0 > 0                                     <- zero counter records

rocprofv3-test-counter-collection-pmc-selected-regions.test_counter_collection_only_inside_selected_regions
  E AssertionError: expected 4 counter record(s) for 'target_kernel'
    (one per resume window) but found 1

On develop the same job passes 100% of 975 tests including both by name; #8891 changes 12 source files and no tests. Pause is stop_context, so a dispatch in flight at pause loses its completion — four resume windows yield one record.

Note that this PR is meant to be stacked onto users/vkale/remove-callbacks-counters, the head branch of #8891. Merging this PR updates that branch.

Technical Details

Completion routing

counters/queue_hooks.cpp — the exit hook iterates get_registered_contexts instead of get_active_contexts. completed_cb already resolves ownership by pointer lookup in packet_return_map and self-filters, so each context still processes only its own packets; this restores provenance routing without reintroducing the registry. The enter hook and is_any_active keep using active contexts, because new instrumentation must stop as soon as the context stops.

The lifetime condition from the review

@bwelton conditioned the fix on the callback thread and the counter_callback_info objects staying alive until in-flight dispatches drain, noting stop_context calls callback_thread_stop() unconditionally at core.cpp:190. Both halves hold, and the test now asserts the second:

  • counter_callback_info lifetime is structural. context.hpp:84 holds them as std::vector<std::shared_ptr<counters::counter_callback_info>> on the service, and neither counters::stop_context nor context::stop_context clears that vector. Since the hook now iterates registered contexts, the context and its shared_ptr callbacks are still reachable, so the objects outlive the drain by construction.
  • The callback thread does not strand anything. consumer_thread_t::exit() sets valid = false, then waits on exited, and consumer_loop only sets exited once read_ptr == write_ptr, so everything already queued is consumed before the join. After that, consumer_thread_t::add() takes the selfconsume path and invokes consume_fn inline on the caller, so a completion arriving after the thread is gone is still processed — on the async signal handler thread. counters/tests/consumer_test.cpp restart already covers the drain-on-exit half.

Serialization ordering

context/context.cpp — stop_context now stops counters, SPM, device thread trace and dispatch thread trace before the compare_exchange_strong that clears the active slot, so disable_serialization() always runs before dispatches stop being instrumented. Clearing the slot first opens the opposite window, in which a dispatch is submitted without serializer packets while the serializer is still enabled. This is the counters-side analogue of the second blocking issue bwelton raised on #8887.

Maintaining drain for liveness-based routing

Note: A stale comment on the exit hook, left over from the revision that removed the drain, claimed provenance routing made stop_context safe without a drain. It contradicted the code a few files away and is corrected here to say what each mechanism actually guarantees.

counters::stop_context retains hsa::queue_controller_sync() ahead of disable_serialization(), and callback_thread_stop() now runs after it rather than alongside.

This is what makes the lifetime condition above hold literally rather than by argument. An intermediate revision of this branch removed the drain, reasoning that consumer_thread_t::exit() drains its own queue and add() self-consumes inline afterwards. Both facts are true and remain a useful backstop, but they are not the guarantee that was asked for: they argue a late completion is still processed, rather than ensuring there is no late completion to process. The comment at the call site now says which is which.

Ordering is load-bearing and stated at the call site. context::stop_context runs counters::stop_context while the context is still in the active list, so throughout the drain the enter hook still reaches queue_cb, whose disabled path returns serialize=true and keeps the serialized-to-unserialized transition coordinated until disable_serialization() runs. That is what removes the need for a separate draining flag on the counters side, and it only works because the drain happens before the active slot is cleared.

The companion SPM PR #8887 does the same, and reverts a removal that had followed @SrirakshaNag's suggestion on #8887. The reasoning for doing that there: provenance routing guarantees that a completion which arrives is delivered, whereas the drain bounds when completions arrive at all, and the teardown depends on the latter — so the two are not substitutes.

Review feedback from @vlaindic

Hook renames, using the name agreed in that thread, since "write" did not convey that this is a write-interceptor hook:

Before After
counters::write_hook counters::kernel_dispatch_phase_enter_hook
counters::signal_completion_hook counters::kernel_dispatch_phase_exit_hook

The exit hook is renamed for symmetry even though only the enter hook was flagged — leaving one half renamed reads worse than either alternative, and the paired names make the enqueue-versus-completion asymmetry above easier to follow. is_any_active keeps its name, being neither phase. SPM mirrors these names on its own branch and is deliberately left alone until #8887's review settles, so the two are briefly inconsistent by choice.

client_ids.hpp enum. The int64_t was not arbitrary: using ClientID = int64_t at hsa/rocprofiler_packet.hpp:37, and the tag is stored in inst_pkt_t as a ClientID. The four constants are now an unscoped enum with int64_t as its fixed underlying type, which groups them under one type while preserving the implicit conversion, so no emplace site needs a cast; enum class would have required one at each.

The declaration now also records bwelton's non-blocking observation from #8790: these tags numerically overlap the registry's auto-incrementing ClientID, which also starts at 1 and is still in use by the unmigrated services. That is inert only because no consumer dispatches on the tag — counters' completed_cb and thread trace's post_kernel_call both identify their own packets by pointer lookup or dynamic_cast. The comment states what has to happen before that changes, so it is visible to whoever first tries to route on these values rather than living only in a PR thread.

Copyright headers. 2025 to 2026 on the four files the base PR adds.

Software Architecture Doc

docs/conceptual/queue_hooks/queue_callback_removal_architecture.md documents the mechanism this stack establishes: why the enter hook routes over active contexts while the exit hook routes over registered ones, why that asymmetry is the difference between delivering and dropping an in-flight dispatch's completion, and why the stop path drains. It was written on a separate branch and is folded in here so it is reviewed next to the code it describes, and so the drain rationale lives somewhere more durable than a PR thread.

It references code by file and symbol rather than by line number, and carries its diagram as inline mermaid rather than a checked-in image. It is not added to _toc.yml.in, matching the kernel replay design doc on #8622, so it is not published to the documentation site.

Acceptance criteria

Validated when both of these go from failing to passing on a gfx94X runner:

  • tests.integration.validate.rocprofv3-test-roctx-pause-resume-tracing.test_counter_collection_data
  • tests.integration.validate.rocprofv3-test-counter-collection-pmc-selected-regions.test_counter_collection_only_inside_selected_regions

Both are pre-existing tests that pass on develop and fail on #8891, so they are an objective target rather than a test authored alongside the fix.

Notes on #8891's other check failures

Not caused by the code, for whoever triages them:

  • 6.2 • ubuntu-22.04 • aqlprofile-internal never got past image setup: GPGKeyTemporarilyNotFoundError and an HTTP 500 from the package host.
  • Analyze (python) and Analyze (actions) fail with a workflow template error rather than a finding — .github/workflows/rocprofiler-sdk-codeql.yml lines 62-63, "Unexpected value ''". The jobs never ran, and this affects every PR in the repository.
  • therock-pr-bot reports Timed out waiting for required checks to complete, listing the three above; it is downstream of them. refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry #8891's empty Test Plan and Test Result sections and its Not ready to Review label are worth fixing regardless, but they are not what turned this check red.

Integration with Kernel replay PR and associated tests

Kernel replay (#8622) runs N passes per dispatch and reuses process_packet_batch for each, so every pass creates its own instrumentation packet and packet_return_map entry. A dropped completion is one lost record today; under replay it is N leaked packets per dispatch plus silently missing counter groups. The replay loop also waits on its pass_done barrier with no timeout, so a stall on the completion path becomes an application hang rather than data loss.

JIRA ID

Tracked internally.

Test Plan

  • The two pre-existing integration tests under Acceptance criteria, which currently fail on refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry #8891.
  • Strengthened unit regression test, counters/tests/queue_hooks_test.cpp — stop_context_in_flight_completion_routes_via_hook_path, now written to what bwelton asked for. Three dispatches are registered while the context is active, the context is stopped with all three in flight, and completions fire one at a time. It asserts both that the map drops by exactly one per completion, which an aggregate check would not catch if one completion erased another's entry, and that every dispatch reaches the tool's record callback. The earlier version passed nullptr for the record callback and drove one dispatch, so it could observe neither property.
  • Existing counter-collection unit tests, including counters/tests/consumer_test.cpp restart, which covers the consumer drain-on-exit behaviour that now backs up the explicit drain rather than substituting for it.
  • Existing rocprofv3 counter-collection integration tests.

Test Result

Not yet built or executed — no ROCm toolchain or GPU in the environment these changes were prepared in. Verified statically:

  • clang-format 11.1.0, the version pinned by the repo's .clang-format, produces no churn outside the lines changed here. An initial pass with a newer clang-format reformatted unrelated lines and was discarded.
  • No references to the old hook names remain on the branch outside spm/, which is unmigrated on this base and correctly untouched.
  • A standalone translation unit confirms the new enum converts to ClientID in both the piecewise and make_pair forms.

Note the strengthened test's delivery assertion has not been run. It depends on counter evaluation yielding at least one record for a dispatch that never executed on hardware; if the profile's output buffer decodes to nothing, the assertion will report that rather than silently passing, which is the intended behaviour but is worth watching on the first CI run.

Submission Checklist

@amd-vkale
amd-vkale marked this pull request as ready for review July 31, 2026 22:28
@amd-vkale
amd-vkale requested review from a team as code owners July 31, 2026 22:28
@amd-vkale amd-vkale changed the title remove-callbacks-counters branch fixes from PR 8891 reviews refactor(rocprofiler-sdk): address review feedback on #8891 for remove-callbacks-counters Jul 31, 2026
@amd-vkale
amd-vkale requested review from MythreyaK and bwelton July 31, 2026 22:33
@amd-vkale amd-vkale self-assigned this Jul 31, 2026
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 1, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Report

Code Coverage Report

Tests Only

code coverage tests.png

Samples Only

code coverage samples.png

Tests + Samples

code coverage all.png

cursoragent and others added 12 commits August 13, 2026 01:22
Address review feedback on #8891: completion routing must use registered
contexts (packet_return_map provenance), stop_context must drain in-flight
GPU work before deactivation, and queue-interposed services must stop while
the context remains active for serialization coordination.

Adds queue_hooks regression test for stop-then-complete ordering required
by kernel replay (#7960/#8622).

Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Reverts the queue_controller_sync() added earlier on this branch. The
justification given for it was wrong: it claimed callback_thread_stop() could
strand records belonging to dispatches still in flight. It cannot.

- consumer_thread_t::exit() sets valid=false, notifies, then waits on the exited
  flag, and consumer_loop only sets exited once read_ptr == write_ptr, so
  everything already queued is consumed before the join. counters/tests/
  consumer_test.cpp restart already covers this drain-on-exit path.
- consumer_thread_t::add() falls back to calling consume_fn inline when valid is
  false, so a completion arriving after the thread is gone is still processed on
  the async signal handler thread.

Nothing else in counters::stop_context needs an idle GPU. In-flight completions
are handled by signal_completion_hook routing over registered rather than active
contexts, and the serializer transition is reconciled GPU-side by the hsa_barrier
that profiler_serializer::disable() pushes.

This aligns the counters path with the same change on the SPM branch, prompted by
review feedback from @SrirackshaNag noting that SPM was the only service with a
sync in its stop path and that the hook migration removes its original reasons.

The context::stop_context ordering is kept but re-justified: 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.

Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Applies the changes requested in review of the base PR, so that merging this
stacked PR clears them on users/vkale/remove-callbacks-counters.

Renames, per @vlaindic's comment that 'write' does not convey that this is a write
interceptor hook, and per the name agreed in the reply on that thread:
  counters::write_hook             -> counters::kernel_dispatch_phase_enter_hook
  counters::signal_completion_hook -> counters::kernel_dispatch_phase_exit_hook

The exit hook is renamed for symmetry even though only the enter hook was flagged;
leaving one half renamed reads worse than either alternative, and the paired names
make the enqueue-versus-completion asymmetry this branch introduces easier to
follow. is_any_active keeps its name, being neither phase.

SPM mirrors these names on its own branch and is deliberately left alone until the
review of #8887 settles, so the two branches are briefly inconsistent by choice
rather than by oversight.

client_ids.hpp: the four constants become an unscoped enum with int64_t as its
fixed underlying type. That answers the question asked in review -- the int64_t was
not arbitrary, it matches hsa::ClientID, which is how the tag is stored in
inst_pkt_t. Unscoped rather than enum class so the implicit conversion to ClientID
survives and no emplace site needs a cast.

The regression test now constructs the pair piecewise instead of through
std::make_pair, which would deduce the tag's enum type and lean on a pair-to-pair
conversion; piecewise converts the tag directly and matches the production hook.

Copyright headers on the four files the base PR adds move from 2025 to 2026.

Verified without a build, which is not available here: clang-format 11.1.0 (the
version the repo's .clang-format pins) leaves no unrelated churn, no references to
the old names remain, and a standalone translation unit confirms the enum converts
to ClientID in both the piecewise and make_pair forms.

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

Strengthens the stop-while-in-flight regression test to what the review on #8891
asked for: enqueue kernels, stop the context mid-flight, and assert every kernel
produces a record.

The previous version had two weaknesses. It passed nullptr for the record
callback, so it could not observe delivery at all -- and delivery is the failure
the review described. And it drove a single dispatch, which cannot distinguish
'routed the one entry present' from 'routed by provenance'.

Now three dispatches are registered while the context is active, the context is
stopped while all three are in flight, and completions are fired one at a time.
Two independent invariants are checked, because they fail separately:

- routing: the map is asserted to drop by exactly one per completion, not merely
  to reach zero at the end. An aggregate check would pass even if one completion
  erased another dispatch's entry.
- delivery: every dispatch reaches the tool's record callback. Erasure happens in
  completed_cb before the hand-off to the consumer, so a map-only test would pass
  even if nothing were ever delivered.

The delivery assertion also covers the lifetime condition raised in the review:
callback_thread_stop() has already run by then, so these records arrive through
the inline path in consumer_thread_t::add() rather than the consumer thread.

No kernel executes, so counter values are whatever the profile's output buffer
holds; the assertion is about delivery, not values. Not compiled here -- no ROCm
toolchain in this environment.

Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…'s ClientID

Captures the non-blocking observation from the review of #8790 at the declaration
rather than only in a PR discussion, since that is where someone about to route on
these values will look.

The tags overlap the per-queue registry's auto-incrementing ClientID, which also
starts at 1 and is still in use by the services not yet migrated. This is inert
only because no consumer dispatches on the tag: counters' completed_cb and thread
trace's post_kernel_call both identify their own packets by pointer lookup or
dynamic_cast. The comment states what has to happen before that changes.

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

Restores the queue_controller_sync() this branch had earlier removed, per review
priority: the review of #8891 accepted provenance-based completion routing on the
condition that the callback thread and the counter_callback_info objects stay alive
until in-flight dispatches drain, and the drain is what makes that hold literally
rather than by argument about consumer_thread_t's fallback behaviour.

An intermediate revision removed it on the reasoning that consumer_thread_t::exit()
drains its queue and add() self-consumes inline afterwards. Both facts are true and
they remain a useful backstop, but they are not the guarantee that was asked for --
they argue a late completion is still processed, rather than ensuring there is no
late completion to process. The comment now says which is which.

Ordering matters and is now stated at the call site: context::stop_context runs this
function while the context is still in the active list, so for the duration of the
drain the enter hook still reaches queue_cb, whose disabled path returns
serialize=true and keeps the serialized->unserialized transition coordinated until
disable_serialization() runs. That is what removes the need for a separate draining
flag -- and it only works because the drain happens before the active slot is
cleared. callback_thread_stop() now runs after the drain rather than alongside it.

Not compiled here -- no ROCm toolchain in this environment.

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

The comment claimed provenance routing makes stop_context safe without a GPU
drain, which contradicts counters::stop_context, where the drain was kept.
State what each mechanism actually guarantees: provenance routing delivers a
completion that arrives, the drain bounds when completions arrive.

Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Records how queue-interposed services are wired into dispatch interception
after the per-queue callback registry is removed: why the enter hook routes
over active contexts while the exit hook routes over registered ones, why
that asymmetry is the difference between delivering and dropping the
completion of an in-flight dispatch, and why the stop path drains the GPU.

Folded in from a separate docs branch so it is reviewed alongside the code
it describes.

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

context::correlation_id declares constructors, deletes its copy and move, and
holds private ref counters, so it is not an aggregate and a designated
initializer does not compile:

  error: initialization of non-aggregate type 'context::correlation_id'
         with a designated initializer list

Default-construct and assign the one member the test needs, which leaves every
other member at the same default the designated initializer would have.

This broke the rocprofiler-sdk build, which is why all four Core test jobs and
Code Coverage failed alongside the TheRock package builds.

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

counters::stop_context() drains in-flight dispatches via queue_controller_sync()
and coordinates serializer teardown, both of which require a live queue
controller. finalize() ran queue_controller_fini() first and invoked the client
finalizers afterwards, so stopping a counter-collection context during teardown
operated on destroyed queue state and segfaulted.

Also skip empty optional slots when walking the registered-context array; the
array is sparse and dereferencing a disengaged optional is undefined.

Fixes the hip-streams-per-thread SEGFAULT seen in CI.

Co-authored-by: Cursor <cursoragent@cursor.com>
Moving invoke_client_finalizers() ahead of the subsystem teardowns deadlocked
OMPT finalization: rocprofv3-test-ompt-tracing and rocprofv3-test-ompt-all timed
out on every CI OS, and openmp-tools.test_retired_correlation_ids failed with
them. The reorder also did not fix the hip-streams-per-thread SEGFAULT it was
meant to address, so revert it and keep the original ordering.

The sparse-optional guards in context.cpp are unaffected and stay in place.

Co-authored-by: Cursor <cursoragent@cursor.com>
buffered-api-tracing and the two thread-trace-api-triple-buffer tests
failed on ubuntu-22.04 only, while rhel-8.8, rhel-9.5 and sles-15.6
passed and TheRock's own rocprofiler-sdk shard passed. No content
change; this re-runs CI on an identical tree to confirm.

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

cursor Bot commented Aug 18, 2026

Copy link
Copy Markdown

Automated stacked-PR status check (Cursor)

Stack position: develop → #8891 → #9586 → #9868. Per the stacked-PR policy (base PR + this PR both must be yours, and "ready enough" before an automatic rebase+merge), this PR is not yet eligible to merge.

Why:

Remaining steps:

  1. Resolve refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry #8891's blocking review comment (route completion by packet provenance rather than current context activeness) and get it approved.
  2. Fix the failing CI jobs (Code Coverage, rocprofiler-sdk tests, docs build).
  3. Request a review on this PR.
  4. Rebase the stack onto current develop once refactor(rocprofiler-sdk): migrate counter collection off the per-queue callback registry #8891 is close to landing.

Posted by an automated stacked-PR status check — no code or branch changes were made.

@amd-vkale

Copy link
Copy Markdown
Contributor Author

Mergeability (local review + GPU unit tests, 2026-08-18)

Ready to fast-forward into #8891. This stacked PR is the actual fix for @bwelton's blocking review on #8891 (completion routing by packet provenance via get_registered_contexts, plus the stop-path drain). That is a fast-forward: #8891's tip is an ancestor of this head.

Local results on this head (9681ed78fe):

  • counters_queue_hooks.is_any_active_false_when_no_context_active — pass
  • counters_queue_hooks.stop_context_in_flight_completion_routes_via_hook_path — pass (this is the test the review asked for)
  • Every core.* test that actually belongs to that suite — pass in isolation
  • Full counter-tests in one process still aborts in core.destroy_counter_profile on the client-index CHECK. That reproduces without the queue-hooks tests, so it is a pre-existing in-process isolation issue, not a regression from this stack.

No human review landed on #9586 itself, but the blocking #8891 comment is implemented and the new test passes. Folding this into #8891 is what makes #8891 re-reviewable as a single PR.

Not folding #9868 on top of this (see comment there): it does not compile, and it is a feature, not a review fix.

@amd-vkale
amd-vkale merged commit 9681ed7 into users/vkale/remove-callbacks-counters Aug 18, 2026
50 of 54 checks passed
@amd-vkale
amd-vkale deleted the users/vkale/remove-callbacks-counter-collection-cursor-fixes branch August 18, 2026 05:59
amd-vkale added a commit that referenced this pull request Aug 18, 2026
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation organization: ROCm project: rocprofiler-sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants