Repository navigation
Conversation
…lback registry Isolates the thread-trace 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. Thread trace no longer registers a queue_callbacks_t with the HSA queue controller. Instead the HSA write interceptor and async signal handler call explicit free functions: - thread_trace::write_hook / signal_completion_hook / is_any_active (new thread_trace/queue_hooks.{hpp,cpp}) - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for counters, pc-sampling and spm; those services are migrated in follow-up PRs. The WriteInterceptor gate and should_batch predicate now detect thread trace via is_any_active() (it no longer contributes to queue.get_notifiers()). Adds thread-trace-queue-hooks-test unit test. Co-authored-by: Cursor <cursoragent@cursor.com>
…ad client global Addresses the two follow-ups @bwelton raised when approving #8790, which he asked be handled either there or as a fast follow before the next service migration. Neither has been picked up, so they are here as a stacked change. 1. Multi-context ATT ownership. context.cpp's active-context conflict check rejected a second dispatch_counter_collection context but had no equivalent for dispatch_thread_trace. That invariant previously held by accident: the per-queue callback registry registered one global client, so a second concurrent ATT context silently did nothing. With the registry gone the hooks iterate every active ATT context, and DispatchThreadTracer::post_kernel_call identifies packets by dynamic_cast plus agent lookup with no per-context ownership check, decrementing post_move_data before verifying agent ownership -- so two ATT contexts on one agent cross-talk and mis-refcount instead of one being ignored. Now rejected with ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT, mirroring the counter-collection check directly above it. Note this is a behaviour change: starting a second ATT context now fails where it previously succeeded and did nothing. He offered a documenting comment as the minimum acceptable alternative; the guard is implemented because silently doing nothing is the worse of the two contracts, but say so if the comment is preferred for an already-approved PR. 2. Dead code. thread_trace/core.cpp's client global (common::Synchronized<std::optional<int64_t>>) and its comment are unused now that the add_callback/remove_callback call sites are gone -- verified zero references on the branch. Removed so it does not mislead readers of the remaining service migrations into thinking single-client gating still exists. No include changes needed; those headers arrive transitively. Not compiled here -- no ROCm toolchain in this environment. clang-format 11.1.0, the version the repo pins, reports no churn outside these lines. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Apply per-agent scoping from PR #9868 to the thread-trace callback-removal branch: - Per-agent serialization ref-counting in queue_controller - DispatchThreadTracer configured_agents/collects_on/intersects helpers - Scoped enable/disable serialization in start_context/stop_context - Agent filtering in thread_trace::write_hook - Disjoint dispatch ATT context conflict rules Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Make agents_map_mut mutable for const query methods, rename locals that shadow the agents member, and apply clang-format 11 to queue_controller. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Declaration alignment in configured_agents and intersects. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Dispatch ATT already configures one agent at a time; these tests lock in the new contract that disjoint contexts can run together, overlapping ones still conflict, serialization stays on the configured agents, and write_hook skips the rest. Co-authored-by: Cursor <cursoragent@cursor.com>
- Route ATT completion via registered contexts (in-flight after stop) - Filter post_kernel_call by THREAD_TRACE_CLIENT_ID and agent ownership - Stop ATT services before clearing active slot (match SPM/counters) - Drain queues before disable_serialization on ATT stop - Fix per-agent serialization effective-state transitions - Add in-flight completion regression test for ATT queue hooks Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Keeps the conflict-resolved side for context.cpp stop ordering, ATT post_kernel_call packet filtering and the exit-hook registered-context walk (identical semantics, already present). Takes the review-fix side for update_serialization, which now derives the serializer action from the post-update refcount instead of the caller's enable flag, plus the accompanying queue_hooks tests.
Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…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.
Both are ordering bugs where thread trace diverged from the counters and SPM migrations, and both let dispatches run without the serializer barriers they were supposed to get. start_context set `enabled` before calling enable_serialization. Because context::start_context publishes the context into the active set before invoking the tracer, write_hook can already be running at that point, so the window let pre_kernel_call inject ATT packets with is_serialized=true while the profiler serializer was still disabled; profiler_serializer::kernel_dispatch then skips the ready/block barriers for exactly those packets. Enable serialization first, then arm the tracer, matching spm::start_context. pre_kernel_call returned serialize=false as soon as `enabled` was cleared. Stop clears `enabled`, drains with queue_controller_sync, and only then disables serialization, leaving the context active throughout, so new dispatches in that drain window could bypass the barriers while in-flight serialized ATT packets were still retiring. Return this context's configured serialization instead and skip only the trace injection, which is what the counters disabled path and the existing kernel-replay override already do. The check moves below the agent lookup so the per-agent bSerialize setting is available; a context that never serialized an agent still reports false for it. Also corrects two comments that described the pre-migration callback registry, and formats one line the completion-path agent filter left unformatted.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Thread trace (ATT) registered a per-queue callback with
QueueControllerto get notified on kernel dispatch and completion. That registry couples every service to the queue controller's callback lifetime, makes the write interceptor's "is anyone listening?" gate depend on an opaque notifier count, and forces all services to share one activation model. This PR migrates ATT to explicit hook invocation, the same migration being applied to counters (#8891), SPM (#8887) and PC sampling (#8895).This is a conflict-resolved and review-corrected continuation of #8790 against current
develop.Technical Details
Migration.
thread_trace::write_hookis called directly fromWriteInterceptorandthread_trace::signal_completion_hookfromAsyncSignalHandler, replacingadd_callback/remove_callback. The deadclientglobal left behind by the removed registration is gone.Routing. The enter hook iterates active contexts; the exit hook iterates registered contexts. That split is deliberate: a dispatch submitted before
stop_contextmust still retire after the context leaves the active set. Each tracer claims only packets taggedTHREAD_TRACE_CLIENT_IDand belonging to an agent it owns.Per-agent scoping.
start_contextconflicts only whendispatch_thread_traceagent sets intersect, so ATT contexts targeting disjoint GPUs can run concurrently. Serialization is scoped toconfigured_agents()rather than process-wide.Two serialization lifetime fixes made in this round:
start_contextsetenabledbefore callingenable_serialization. Becausecontext::start_contextpublishes the context into the active set before invoking the tracer,write_hookcan already be running, so the window letpre_kernel_callinject packets withis_serialized=truewhile the serializer was still disabled — andprofiler_serializer::kernel_dispatchskips the ready/block barriers for exactly those packets. Now serializes first, matchingspm::start_context.pre_kernel_callreturnedserialize=falseas soon asenabledwas cleared. Stop clearsenabled, drains viaqueue_controller_sync, then disables serialization, leaving the context active throughout; new dispatches in that window could bypass barriers while in-flight serialized ATT packets were still retiring. It now returns this context's configuredbSerializeand skips only the trace injection, which is what the counters disabled path and ATT's own kernel-replay override already do.Hook packet tags moved to a negative range (
THREAD_TRACE_CLIENT_ID = -1002).QueueController::add_callbackallocates strictly positive ids from 1, which is the range the tags previously used. This was inert while tags were only carried, but ATT now dispatches on its tag, so a registry-allocated id could have been mistaken for a hook tag. Astatic_assertkeeps the ranges disjoint.Issue Tracking
JIRA ID: AIPROFSDK-1016
Supersedes the conflicted branch behind #8790. Part of the callback-removal series with #8891, #8887, #8895.
Test Plan
thread_trace/tests/queue_hooks_test.cpp— hook gate when no context is active; in-flight completion routes through the hook path afterstop_context.thread_trace/tests/per_agent_scoping_test.cpp— disjoint-agent contexts start concurrently, overlapping ones conflict,write_hookfilters by agent.clang-formatunder the pinned>=11.0.0,<12.0.0, and the repository'sstd::regexrestriction check.Test Result
Static checks pass. Unit tests were not executed in the authoring environment: it has no ROCm install, and the ATT unit tests link
libhsa-runtime64and requirehsa_init(). They need a reviewer or CI run to execute.Not yet covered by tests — the two serialization fixes above are verified by reading and by parity with the counters/SPM paths, not by a regression test. Coverage for serialize-before-enable and for the drain-window
serializeflag is the most valuable thing to add.Submission Checklist