Repository navigation
[rocprofiler-sdk] defer barrier retirement until transition packets drain - #6750
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses an intermittent roctx pause/resume deadlock in rocprofiler-sdk’s HSA profiling serializer by preventing premature retirement (signal destruction) of serializer transition barriers while any queue may still have an unexecuted transition packet referencing the barrier’s signal.
Changes:
- Add per-queue monotonic
serialized_dispatched/serialized_completedcounters to detect when a queue has executed past the dispatch that carried a barrier transition packet. - Extend
hsa_barrierwith per-queue “transition enqueued” tracking andsafe_to_retire()gating so barriers are only retired after transition packets drain (plus a destructor backstop to avoid handle reuse). - Add regression tests covering deferred retirement, queue teardown escape, and non-recycling of barrier signal handles while referenced.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/tests/hsa_barrier.cpp | Adds regression tests validating barrier retirement is deferred until transition packets drain and handles aren’t recycled while referenced. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.hpp | Introduces serialized dispatch/completion counters used to determine transition packet drain progress. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp | Increments the new serialized counters at dispatch and completion to drive drain detection. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/profile_serializer.cpp | Uses safe_to_retire() when clearing barriers and notifies barriers on serialized completions to drop drained queue tags. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/hsa_barrier.hpp | Adds drain notification API, safe_to_retire(), handle accessor, and changes enqueued tracking to a queue→dispatch-id map. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/hsa_barrier.cpp | Implements deferred retirement via drain tracking, safe_to_retire(), and a destructor backstop that avoids handle reuse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
meserve-amd
left a comment
There was a problem hiding this comment.
I think this is fix is overall correct, but I want to get rid of the leak-to-cover-up behavior. Some other comments around naming and comments. Overall I think some more explanation could be warranted around what the "notify_drain" is checking for.
meserve-amd
left a comment
There was a problem hiding this comment.
Looks good now. Thanks for addressing everything.
I recommend you ask @bwelton for a second review; they probably have the best understanding here.
bwelton
left a comment
There was a problem hiding this comment.
I am ok with these changes for the most part, i would suggest moving the counters into the serializer itself since its the only one that actually cares about them. I also think we may need to include this draining/serialization destruction in QueueController when we are destroying queues (however i am not sure that actually matters in practice).
A serializer barrier could be retired (its HSA signal destroyed) while a queue still held an un-executed transition packet on that signal. HSA recycled the freed handle into the next barrier, so an armed barrier stranded the queue's stale packet -> deadlock (seen with roctx pause/resume). Defer retirement until every transition packet has executed: tag each packet with its dispatch id, clear the tag on drain/teardown, and gate retirement on safe_to_retire(). Destructor leaks a still-referenced signal as a backstop. Adds regression tests.
99074aa to
8d409b6
Compare
Motivation
Technical Details
A serializer barrier owns an HSA signal that its queues' transition (barrier-AND) packets wait on. The hang occurred when a barrier was retired (signal destroyed) while a queue still held an un-executed transition packet; HSA recycled the freed handle into the next barrier, and if that one was armed the queue blocked on the wrong barrier → deadlock.
Fix: defer retirement until every transition packet has actually executed:
hsa_barriertracks per-queue the dispatch id of the transition packet it handed out (_barrier_enqueuedis now aqueue -> dispatch_id map).Queueexposes monotonicserialized_dispatched / serialized_completedcounters so a barrier can tell whether a queue has executed past a given dispatch.drain_queue(queue)clears a queue's tag onceserialized_completedreaches its token; it early-exits without taking the lock while the barrier is still armed, since a transition packet can't have executed before its barrier clears.remove_queue()clears the tag on teardownsafe_to_destroy() = complete()and no outstanding tags.clear_complete_barriers()retires a barrier (destroying its signal) only whensafe_to_destroy(), so the signal is freed only once nothing references it.ROCP_ERRORif a barrier is ever destroyed with outstanding tags (should be unreachable given the gating +remove_queueon teardown) and then destroys the signal normally, no leak.JIRA ID
ROCM-25255
Test Plan
Test Result
Submission Checklist