Skip to content

feat(rocprofiler-sdk): share per-agent ATT resources across contexts - #8644

Draft
minseobshin11 wants to merge 14 commits into
developfrom
users/mishin/att-shared-queue
Draft

feat(rocprofiler-sdk): share per-agent ATT resources across contexts#8644
minseobshin11 wants to merge 14 commits into
developfrom
users/mishin/att-shared-queue

Conversation

@minseobshin11

@minseobshin11 minseobshin11 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Motivation

ATT allocates heavyweight resources per context and per agent, even though only one thread trace can use an agent at a time:

  • A separate GPU output-buffer set per context wastes device memory. At ~1 GiB each, a few hundred contexts exceed a single device's HBM.
  • A separate HSA submission queue per context exhausts the agent's queue budget. HSA_AGENT_INFO_QUEUES_MAX is 128 on gfx942 and is shared with the application's own queues; exceeding it aborts.

This PR gives each GPU agent a single resource owner. Contexts targeting the same agent reuse its output slots, submission queue, and CPU staging buffers.

Technical Details

  • Adds shared_trace_resources.{hpp,cpp} with one AgentTraceResources per agent, owning the submission queue, CPU staging buffers, and GPU output slots.
  • Tracers, AQLProfile packets, and producer/consumer workers hold the owner by shared_ptr, so a buffer cannot be released while anything still references it.
  • Collects each context's buffer and staging sizes before the first owner is constructed. The output buffer is allocated from the AQLProfile allocation callback during the first tracer's construction, so its size must be final by then.
  • Maps AQLProfile output allocations by packet-local slot index, so slot i resolves to the same buffer for every context on an agent. Host-visible allocations stay per packet.
  • Passes each trace's own buffer size to the producer thread, since shared staging buffers are sized to the largest context on the agent.
  • Records the owning context on its first trace and fails loudly if a second context tries to trace the same agent concurrently.
  • Waits for a trace's stop packet to retire, and drains in-flight dispatches before removing the interception callback, so resources are released only once the GPU is done.

JIRA ID

AIPROFSDK-102

Test Plan

  • ~400 contexts with varying ATT parameters, alternating 1 GiB and 512 MiB buffers, in both device and dispatch mode — over 3x the 128-queue limit.
  • Validate decoded shader data: PCs resolve to loaded code objects, chunks come only from selected shader engines, and on gfx10+ waves come from the selected SIMD.
  • Shared-resource, producer/consumer, standard ATT, pre-HSA-init, large-buffer, and triple-buffer tests.
  • Hardware: AMD Instinct MI300X (gfx942).

Test Result

  • 400-context device and dispatch: 5/5 repeated runs passed.
  • thread-trace-packet-test: 9/9 passed.
  • thread-trace-producer-consumer-test: 10/10 passed.
  • ATT integration suite: all 12 enabled tests passed (2 disabled upstream).

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
🌿 Branch Name ✅ Pass
📝 PR Title/Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
JIRA ID : TESTAUTO-6039
JIRA ID - #330
JIRA ID #330
JIRA ID (on separate line)
ROCM-25757
ISSUE ID : TESTUTO-3334
ISSUE ID #3334
ISSUE ID - TESTAUTO-3433
ISSUE ID (on separate line)
AIRUNTIME-2352
ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
Closes #10
Fixes octo-org/octo-repo#100
Resolves: #123
#123
https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Title/Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

@github-actions

github-actions Bot commented Jul 15, 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

@minseobshin11
minseobshin11 changed the base branch from develop to users/mishin/att-buffer-management July 16, 2026 18:23
}

void
DispatchThreadTracer::register_shared_queue_sizes()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated. Maybe join with above function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

void
DeviceThreadTracer::register_shared_queue_sizes()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same deduplication

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

{
if(ctx->device_thread_trace) ctx->device_thread_trace->register_shared_buffer_sizes();
if(ctx->dispatch_thread_trace) ctx->dispatch_thread_trace->register_shared_buffer_sizes();
if(ctx->device_thread_trace)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can revert this change if they are joined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true-- changed

// Max triple-buffer staging size requested by any context on this agent; the
// shared queue is created with this size so every context's flips fit.
uint64_t max_triple_buffer_size = 0;
att_queue_ptr_t queue = {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe join with the buffer struct? They have the same semantics.
(not mandatory)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left them separate because their contents diverged after changes

// Keep each context active across a few dispatches so it reliably captures one.
constexpr uint64_t CAPTURE_WINDOW = 2;

// Repro mode for the AILIKFD-39 teardown hang: start one context and never stop it,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we reproing it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed -

if(leave_active())
{
// Deliberately do NOT stop the trace context: leave SQTT active so rocprofiler
// teardown destroys the still-active per-agent tracers one-by-one (AILIKFD-39).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment
Also, this is unlikely to work in dispatch mode

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed -


// Not every target_cu is guaranteed to have active waves, so require that MOST
// configs produced data rather than all.
assert(captured * 4 >= configured * 3 && "most configs should capture data");

@ApoKalipse-V ApoKalipse-V Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not not going to fail even if the profiler gave you the same header-only buffer every time. You need to verify the waves are (mostly) valid in the capture window and that they are all in the correct SIMD/CUs you selected in the configuration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that SIMD selection is different for gfx >= 10, you need to take SEL%4 while for gfx9 you take the bitmask.
Then in the validation: Verify you don't get wave_t from simd/cus you didnt ask for, and that you do get (most) of them for the ones you did. To verify waves are valid, you can check the instructions have a valid PC address (except for TRAP and for the first few in case of gfx1250).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed so that the test now decodes every captured trace and asserts real content

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe gfx9 emits all the wave records for all four SIMDs regardless of the requested simd_select - is there a way to validate on gfx10+

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to cover device thread trace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added more tests to cover

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Title/Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants