fix(rocprofiler-sdk): serializer race fixes and hook tag range for counters (#8891) by amd-vkale · Pull Request #11869 · ROCm/rocm-systems

amd-vkale · 2026-09-18T18:09:03Z

Motivation

Carries the outstanding review fixes for #8891, including @bgopesh's two serializer findings. Stacked on users/vkale/remove-callbacks-counters so they land without rewriting that branch's history.

Technical Details

Serializer creation race

QueueController::serializer() read the per-agent refcount, released the lock, then looked up or inserted the serializer entry. An update_serialization() arriving in that gap finds no entry to transition, so the entry is subsequently created from a value that is already stale — and nothing ever corrects it. The observable result is serialization left enabled at refcount zero, or disabled while a context still requires it.

The refcount read lock is now held across both the lookup and the insertion, closing the gap. update_serialization() acquires the refcount lock before the serializer lock, so acquiring them in that same order here introduces no deadlock.

Transition-ordering race

update_serialization() determined which agents crossed zero while holding the refcount write lock, but applied the resulting enable/disable calls after releasing it. Two concurrent callers could therefore apply their transitions in the opposite order from the refcount mutations that produced them, again leaving the serializer disagreeing with the count.

Transitions are now applied inside the same refcount write lock, and each action is derived from the post-update refcount via state.enabled(agent_id) rather than from the caller's enable argument, which may be stale by the time it is read.

serializer() and update_serialization() are now byte-identical to the SPM stacked PR's versions, verified by direct comparison, so #8891 and #8887 will not conflict on these functions.

Hook tag range

COUNTERS_CLIENT_ID moves to -1001. QueueController::add_callback allocates strictly positive ids from 1, which is the range the hook tags previously used, and counters dispatches on its tag — so the collision was reachable rather than theoretical. client_ids.hpp becomes an enum client_id : int64_t with a static_assert keeping the ranges disjoint.

Perf regression test

tests/queue-hooks-perf/ adds a dispatch-path benchmark with a cost model and a launch sweep, so the hook path's overhead relative to the old registry path is measured rather than assumed.

Issue Tracking

JIRA ID: AIPROFSDK-1017

Stacked on #8891. Part of the callback-removal series with #8790, #8887 and #8895.

Test Plan

The serializer locking protocol was model-checked under ThreadSanitizer with concurrent serializer() and update_serialization() callers.
tests/queue-hooks-perf/ for the dispatch path.
clang-format under the pinned >=11.0.0,<12.0.0.

Test Result

The ThreadSanitizer model ran 5,500 concurrent iterations clean against the fixed code. The same harness produces 121 failures against the pre-fix code, so it demonstrably has power against these two specific races rather than merely passing.

Unit and integration tests were not run: they link libhsa-runtime64 and most call hsa_init(), and the authoring environment has no ROCm install. They need a CI or reviewer run.

Known gaps — these block #8891, not this PR

Two of @bgopesh's findings are not addressed here. Both live in context/context.cpp rather than in the counters service, so they want one fix shared across all four services instead of four copies:

kernel_dispatch_phase_exit_hook calls context::get_registered_contexts(), which walks stable_vector<std::optional<context>> with no lock, from the HSA async signal handler. It can race with optional::emplace() in allocate_context and optional::reset() in deregister_client_contexts. The fix is a published snapshot read through a shared_ptr; taking the contexts mutex in the handler would deadlock against the stop-path drain.
context::stop_context holds get_contexts_mutex() across hsa::queue_controller_sync(), an unbounded GPU drain, so every context lifecycle operation in the process stalls behind one context's GPU work. The fix is to phase stop into disable (locked), drain (unlocked), finish (relocked).

Submission Checklist

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

Adds LD_PRELOAD dispatch-counter workload and cost-model ctests to guard direct queue-hook vs callback-registry overhead without kernel replay passes. Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 50a1d31)

…id range The inst_pkt_t tag space is shared between the per-queue callback registry and the queue hooks. QueueController::add_callback allocates strictly positive ids counting up from 1, which is exactly the range the hook tags used, so a packet tagged by a not-yet-migrated service could compare equal to a hook tag. This was inert while the tags were only carried and never inspected, but counters, thread trace and SPM now each select their own packets by comparing against their tag, so the overlap is reachable. Move the tags to a negative range, which the registry cannot produce, and add a static_assert so the invariant survives future additions. Also converts the tags from separate constexpr int64_t values to an unscoped enum with a fixed int64_t underlying type, keeping the implicit conversion to hsa::ClientID so call sites need no casts. (cherry picked from commit b6596f9)

…the SPM branch Derive the applied serializer state from the post-update refcount instead of the caller's `enable` flag and document why, and format queue_controller.cpp under the clang-format version the repository pins. No behavioral change: for an agent that transitioned, the post-update state and the caller's flag already agreed. Keeps serializer() and update_serialization() byte-identical with the SPM branch so the two migrations do not conflict when they land. (cherry picked from commit 72c897e)

Copilot

🟡 Changes recommended

The performance tests have compatibility and validation defects and do not implement the claimed baseline comparison.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes serializer transition races, separates hook tags from callback IDs, and adds dispatch-hook performance tests.

Changes:

Synchronizes serializer state transitions with refcount updates.
Moves hook tags into a negative ID range.
Adds a HIP workload, cost model, and launch-scaling tests.

File summaries

File	Description
`source/lib/rocprofiler-sdk/hsa/queue_controller.cpp`	Applies serializer transitions from current refcount state.
`source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp`	Assigns disjoint negative hook tags.
`tests/CMakeLists.txt`	Registers the performance-test directory.
`tests/queue-hooks-perf/CMakeLists.txt`	Configures performance tests.
`tests/queue-hooks-perf/main.cpp`	Implements the HIP benchmark workload.
`tests/queue-hooks-perf/perf_cost_model.py`	Defines timing ceilings.
`tests/queue-hooks-perf/run_and_validate.py`	Runs and validates scaling cases.
`tests/queue-hooks-perf/run_launch_sweep.py`	Executes the launch-count sweep.

Review details

Files reviewed: 8/8 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+def model_max_ms(
+    launches: int,
+    per_dispatch_ms: float | None = None,
+    fixed_ms: float | None = None,
+    margin: float | None = None,
+) -> float:


+    out = proc.stdout + proc.stderr
+    if proc.returncode != 0 or "[qh-perf] PASS" not in out:
+        raise RuntimeError(f"launches={launches} failed rc={proc.returncode}\n{out}")
+    return out


+    out_path = Path(os.environ.get("QH_PERF_SWEEP_JSON", ""))
+    if out_path:


+        ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/run_and_validate.py --testapp
+        $<TARGET_FILE:queue-hooks-perf-testapp> --preload
+        $<TARGET_FILE:rocprofiler-sdk-json-tool> --ballast-mb 8 --launches-low 8
+        --launches-high 32 --max-scaling-ratio 6.0)


… test Already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Two errors, both in counters/tests/queue_hooks_test.cpp: error: reinterpret_cast from 'std::nullptr_t' to 'rocprofiler::hsa::Queue *' is not allowed error: use of undeclared identifier 'set_client_ctx' The test built a Queue& by dereferencing a null pointer to reach the exit hook's early-return path. The hook never dereferences the queue, so take it as a pointer and let the test pass nullptr, matching the PC sampling branch. set_client_ctx exists in the SPM tests but was never carried over here, so add the same reset helper next to get_client_ctx. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…xit hook test Two errors g++ reports against the libstdc++ headers, so they are easy to miss when scanning diagnostics by source path: error: invalid new-expression of abstract class type 'rocprofiler::hsa::AQLPacket' error: use of deleted function 'rocprofiler::hsa::queue_info_session_t::queue_info_session_t()' AQLPacket is abstract; EmptyAQLPacket is the concrete no-op subclass intended for this. queue_info_session_t holds a Queue& so it has no default constructor, and the hook returns before dereferencing the session, so a null shared_ptr keeps the test free of any HSA runtime. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… older Python test-queue-hooks-perf-scaling and test-queue-hooks-perf-launch-sweep failed on rhel-8.8, rhel-9.5 and sles-15.6 (ubuntu-22.04 passed, since it ships 3.10): File "tests/queue-hooks-perf/perf_cost_model.py", line 22, in <module> per_dispatch_ms: float | None = None, TypeError: unsupported operand type(s) for |: 'type' and 'NoneType' PEP 604 unions in evaluated annotations need Python 3.10. Use typing.Optional instead, and likewise typing.Dict for the builtin generic in run_launch_sweep, so these scripts import on the oldest interpreter CI runs on. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

… perf harness rhel-8.8 and sles-15.6 run Python 3.6, where subprocess.run rejects both capture_output and text: TypeError: __init__() got an unexpected keyword argument 'capture_output' Use stdout/stderr=PIPE with universal_newlines instead. vermin now reports 3.6 as the minimum required version for all three scripts in this directory, which matches the oldest interpreter in CI. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…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>

…ent scoping test The HIP-device-to-agent mapping pushed a match into gpu_agents and silently moved on when a device had no agent with matching PCI coordinates. Dropping a device shifts every later ordinal, so gpu_agents[0] stops being HIP device 0 and the contexts get scoped to a different physical GPU than the workload runs on. Both positive checks then fail while the negative ones pass trivially, which is what the code coverage job -- the only runner with enough GPUs to actually execute this test rather than skip it -- reported. Stop at the first unmapped ordinal and let main() skip with a message naming the device, and print the mapping and the raw counts the assertions read so a control failure is distinguishable from a scoping bug. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ontext The registered-context registry was walked with no lock from threads that cannot take get_contexts_mutex(). allocate_context() appends to the stable_vector while those walks are in progress, which races on the chunk list, and deregister_client_contexts() destroyed contexts out from under readers that had already taken a pointer. The registry now holds shared_ptr<context> and publishes an immutable snapshot under get_contexts_mutex() on every mutation. Readers load the snapshot instead of the registry, and holding one keeps the contexts it names alive for the duration of the read, so a context dropped mid-read survives until the reader is done with it. stop_context() held get_contexts_mutex() across the service teardown, which contains hsa::queue_controller_sync() -- an unbounded wait on in-flight GPU work. Every context lifecycle operation in the process stalled behind one context's dispatches, and the lock sat on the far side of a wait that the completion path has to get through. Stop is now phased: claim the context under the lock, tear the services down and drain with the lock released, retire the active slot under the lock again. The active slot stays populated across the drain, which the serializer transition depends on, so a set of in-progress stops keeps start_context() and deactivate_client_contexts() from reading that state as a running context. The conflict scan in start_context() moves inside the same lock acquisition as the slot search so the two agree. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…s exit hook kernel_dispatch_phase_exit_hook runs on the HSA async signal handler, so it cannot take get_contexts_mutex(): stop_context() waits on the drain that this completion is part of. It walked the registry unlocked instead, which races with allocate_context(). It now reads the published snapshot, which needs no lock and keeps every context it names alive for the length of the loop. The filter moves inline so the hook no longer builds a vector per completion. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

The build-docs-from-source job died in 82 ms configuring ROCR-Runtime with 'CMake 3.25 or higher is required. You are running version 3.22.1'. It installs cmake from apt, and this branch predates the container image it runs in, so the distro cmake is now too old for ROCR-Runtime. Same treatment the code coverage and release compatibility jobs already got: install cmake into a venv and hand that venv to the shared util action. Held at 3.25 rather than develop's 4.x because this branch's external submodules still declare cmake_minimum_required below 3.5. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…hases stop_context() now releases get_contexts_mutex() across the GPU drain, and for that window it was holding nothing but a raw pointer to the context it was tearing down. deregister_client_contexts() takes the same mutex, so it can run in exactly that window and drop the registry's reference -- which was the last one -- destroying the context underneath the drain. The same hole exists in start_context(), whose service starts also run unlocked. Both now hold the registry's shared_ptr for as long as they use the raw pointer, and deregister_client_contexts() waits for a pending stop before retiring a registry entry rather than pulling it out mid-teardown. The retired entries are moved into a local vector and destroyed after the lock is dropped, since a context destructor calls back into HSA and that is not something to run underneath the registry mutex. get_mutable_registered_context() goes back to indexing the registry directly. Routing it through the snapshot was never needed to close either gap -- the snapshot exists for the completion path, which cannot take the mutex -- and it turned an O(1) lookup into a scan for no benefit. start_context()'s slot search likewise goes back to its original shape, so the only change left in it is the wait for pending stops. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

The phased stop releases get_contexts_mutex() across the GPU drain, so for that window a context is still in the active array with nothing held. set_dispatch_agents() takes that mutex precisely to be atomic with activation state, but the lock alone no longer excludes a stop: it would read a context that is on its way out as a running one and refuse the agent assignment with CONFIGURATION_LOCKED. The wait that start_context() and stop_context() already use is promoted out of the anonymous namespace as context::wait_for_stopping_contexts(), and set_dispatch_agents() takes it after acquiring the mutex. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…stry's The registry and the snapshot published from it were two separate static objects. common::destroy_static_objects() runs registered destructors in reverse construction order, and construction order here is whichever was touched first: get_registered_contexts() reaches the snapshot, while allocate_context() reaches the registry, so either can come first depending on the tool. When the snapshot is constructed first it is destroyed last, and by then it holds the only remaining reference to every context -- so the contexts are destroyed after the statics registered between the two, including the HSA tables that ~agent_callback_data calls hsa_signal_destroy_fn through. Both now live in one struct behind a single static object, where the snapshot is destroyed before the registry by the ordinary reverse-member rule. Contexts are then released at the same point in teardown as before the snapshot existed. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

Committed by accident alongside the set_dispatch_agents fix. The repository tracks no .cursor content, so this does not belong in a product change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

The stopping marker holds deactivate_client_contexts() off until phase four returns, so it can no longer clear the slot mid-teardown. Say that, instead of describing a race the marker already rules out. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…st HSA shutdown agent_callback_data's destructor reached through hsa::get_core_table() unconditionally. Nothing guarantees the owning context is destroyed while HSA is still up, so this faults when teardown runs after the runtime has been shut down. hsa::active_signals::destroy() already guards the identical call this way. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

…ounters' into cursor/resolve-stacked-counters-fixes Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # .github/workflows/rocprofiler-sdk-code_coverage.yml # .github/workflows/rocprofiler-sdk-docs.yml # projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/queue_hooks_test.cpp # projects/rocprofiler-sdk/tests/CMakeLists.txt # projects/rocprofiler-sdk/tests/queue-hooks-perf/CMakeLists.txt # projects/rocprofiler-sdk/tests/queue-hooks-perf/main.cpp # projects/rocprofiler-sdk/tests/queue-hooks-perf/perf_cost_model.py # projects/rocprofiler-sdk/tests/queue-hooks-perf/run_and_validate.py # projects/rocprofiler-sdk/tests/queue-hooks-perf/run_launch_sweep.py

amd-vkale · 2026-09-21T20:04:17Z

Superseded: every commit on this branch is already an ancestor of the base branch users/vkale/remove-callbacks-counters, so #8891 alone now carries the full set of lifecycle, race, scope and hook-tag fixes. Two further fixes were made on top of this content directly on the base branch: the completion-hook test fixture now uses a null session so it does not require a live HSA runtime, and a wording fix in per_agent_scoping_app.cpp.

Closing to keep the review on #8891. Nothing here is lost.

amd-vkale and others added 3 commits September 18, 2026 06:33

amd-vkale marked this pull request as ready for review September 18, 2026 18:09

amd-vkale requested review from a team as code owners September 18, 2026 18:09

github-actions Bot added the project: rocprofiler-sdk label Sep 18, 2026

amd-vkale requested a balanced review from Copilot September 18, 2026 18:09

Copilot started reviewing on behalf of amd-vkale September 18, 2026 18:10

View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

systems-assistant Bot added the organization: ROCm label Sep 18, 2026

cursoragent and others added 6 commits September 19, 2026 21:28

style(rocprofiler-sdk): apply clang-format to the counters queue hook…

d831c58

… test Already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

cursor Bot requested a review from a team as a code owner September 21, 2026 05:54

github-actions Bot added the github actions Pull requests that update GitHub Actions code label Sep 21, 2026

cursoragent and others added 11 commits September 21, 2026 06:07

chore: drop personal editor rules file from the branch

10b7d5e

Committed by accident alongside the set_dispatch_agents fix. The repository tracks no .cursor content, so this does not belong in a product change. Co-authored-by: Vivek Kale <vivek.kale@amd.com>

amd-vkale closed this Sep 21, 2026

Navigation Menu

Repository navigation

fix(rocprofiler-sdk): serializer race fixes and hook tag range for counters (#8891) - #11869

fix(rocprofiler-sdk): serializer race fixes and hook tag range for counters (#8891)#11869
amd-vkale wants to merge 21 commits into
users/vkale/remove-callbacks-countersfrom
users/vlkale/remove-callbacks-counters-stacked-withcursorfixes

amd-vkale commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

amd-vkale commented Sep 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		out_path = Path(os.environ.get("QH_PERF_SWEEP_JSON", ""))
		if out_path:

Conversation

amd-vkale commented Sep 18, 2026

Motivation

Technical Details

Serializer creation race

Transition-ordering race

Hook tag range

Perf regression test

Issue Tracking

Test Plan

Test Result

Known gaps — these block #8891, not this PR

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

amd-vkale commented Sep 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants