Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the rocprofiler-sdk SPM dispatch counting service to be agent-scoped, allowing tools to restrict SPM dispatch instrumentation/serialization to specific GPU agents and enabling concurrent SPM contexts when their agent sets are disjoint. It also updates queue serialization handling to be reference-counted per agent to support scoped enable/disable across multiple subsystems.
Changes:
- Add a new experimental public API
rocprofiler_spm_dispatch_counting_service_set_agents()and plumb agent sets through SPM dispatch context state. - Scope SPM dispatch enter-hook behavior (
spm::write_hook) and queue serialization enable/disable to the configured agent set. - Introduce per-agent serialization refcounting in
QueueController, plus SPM context conflict rules based on agent-set intersection.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp | Updates test initialization to match the current context::correlation_id constructor. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp | Adds the C API entrypoint for setting SPM dispatch agents. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp | Filters SPM dispatch handling by the queue’s agent id via collects_on(). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp | Declares internal set_dispatch_agents() plumbing for the new API. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp | Scopes serialization enable/disable to SPM’s agent set and implements set_dispatch_agents(). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp | Adds agent-set type, per-agent refcount tracking, and new serialization APIs. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp | Implements per-agent refcounted serialization transitions and is_serialization_enabled(). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp | Stores agent sets on SPM dispatch contexts and adds collects_on() / intersects(). |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp | Implements agent-set intersection and updates SPM context conflict rules. |
| projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h | Documents and exposes the new experimental API for agent scoping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| itr->second->wlock([&](auto& serializer) { | ||
| if(enable) | ||
| serializer.enable(queues); | ||
| else | ||
| serializer.disable(queues); |
There was a problem hiding this comment.
@copilot This seems to be an important concurrency bug to fix. Can you please provide a clear suggestion on the implementation for a fix in prose and a code outline? Please do not actually perform the fix yet.
There was a problem hiding this comment.
Root cause
update_serialization currently uses two separate, non-nested locks:
_serialization_refcount.wlock— updates the per-agent/global counters and identifies which agents transitioned (i.e., their effective serialization state flipped). This lock is then released._profiler_serializer.wlock— iterates over the transitioned agents and callsserializer.enable()orserializer.disable()based on the originalenableargument.
Between steps 1 and 2 another thread can call update_serialization with the opposite enable value for the same agent, increment/decrement the refcount again, and then also proceed to step 2. Both threads now hold stale copies of enable that no longer reflect the refcount, so they can each apply the wrong direction to the serializer.
Fix (prose)
The transitioned collection should carry the agent's new effective state (the result of state.enabled(agent_id) after the counter update) alongside the agent id. Both pieces of information are available while _serialization_refcount is still held, so they can be captured atomically with the counter mutation. Step 2 then uses the saved per-agent state instead of the caller's enable flag. A racing thread will compute a different (correct) new state for its own update, so both threads independently apply the right direction.
Code outline
// Change transitioned from vector<agent_id> to vector<pair<agent_id, bool>>
// where the bool is the post-update effective state for that agent.
auto transitioned = std::vector<std::pair<rocprofiler_agent_id_t, bool>>{};
_serialization_refcount.wlock([&](auto& state) {
// ... snapshot was_enabled, mutate counters (unchanged) ...
_profiler_serializer.rlock([&](const auto& serializers) {
for(const auto& [agent_id, _] : serializers)
{
auto itr = was_enabled.find(agent_id);
if(itr == was_enabled.end()) continue;
bool now_enabled = state.enabled(agent_id); // read while lock is held
if(itr->second != now_enabled)
transitioned.emplace_back(agent_id, now_enabled); // carry new state
}
});
// ...
});
_profiler_serializer.wlock([&](auto& m) {
for(const auto& [agent_id, now_enabled] : transitioned)
{
// ...
itr->second->wlock([&](auto& serializer) {
if(now_enabled) // use captured state, not caller's `enable`
serializer.enable(queues);
else
serializer.disable(queues);
});
}
});Note: this fix has already been applied in the previous commit (fix(rocprofiler-sdk): store effective state in update_serialization; add set_agents/disjoint-context tests), since an earlier review comment on the same thread requested it be implemented immediately.
| rocprofiler_status_t | ||
| set_dispatch_agents(rocprofiler_context_id_t context_id, | ||
| const rocprofiler_agent_id_t* agents, | ||
| size_t num_agents) | ||
| { |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
Fixed in commit fix(rocprofiler-sdk): store effective state in update_serialization; add set_agents/disjoint-context tests.
Race condition (queue_controller.cpp:506-510): transitioned now stores std::pair<rocprofiler_agent_id_t, bool> where the bool is the agent's new effective enabled state captured inside the _serialization_refcount lock. The serializer is then toggled using that captured state instead of the caller's enable flag, eliminating the window where a racing thread could apply the wrong direction.
Unit tests (spm/tests/core.cpp): Added two new tests:
spm_core/set_agents_restricts_collection— configures a context with a single agent viarocprofiler_spm_dispatch_counting_service_set_agents, then verifiescollects_onreturns true only for that agent and false for all others.spm_core/disjoint_contexts_no_conflict— verifies that two contexts sharing the same agent produceROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT, while two contexts with disjoint agent sets (when ≥2 SPM-capable agents are present) can both start successfully.
|
Automated stacked-PR status check (Cursor) Stack position: Why:
Remaining steps:
Posted by an automated stacked-PR status check — no code or branch changes were made. |
|
Mergeability (local review, 2026-08-18) Not ready to fold into #8887 / #10098.
Keep stacked. Rebase onto #8887 after #10098 is folded, then get a human review. |
2cd6987 to
758072b
Compare
|
Landed in #8887 via rebase, not a fast-forward. This branch had diverged from the SPM base after the #10098 stop-path tests and the The stacked branch was then force-updated to that same SHA so the history matches the base. GitHub closed rather than auto-merged because of the rewrite. The set_agents / disjoint-context tests are in #8887. |
Motivation
Apply per-agent scoping from PR #9868 to the SPM callback-removal branch.
Technical Details
Issue Tracking
Test Plan
Test Result
Submission Checklist