Skip to content

[ATT] Improved buffer management: per-agent shared trace buffers - #8272

Draft
minseobshin11 wants to merge 4 commits into
developfrom
users/mishin/att-buffer-management
Draft

[ATT] Improved buffer management: per-agent shared trace buffers#8272
minseobshin11 wants to merge 4 commits into
developfrom
users/mishin/att-buffer-management

Conversation

@minseobshin11

@minseobshin11 minseobshin11 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

Thread trace (ATT) allocated a separate GPU output buffer for every context on each agent (default 128 MB each), even though only one trace can be active per agent at a time. With multiple contexts on the same agent this wastes large amounts of device memory. This PR makes all contexts on an agent share a single output buffer sized to the largest requested buffer_size, and adds a per-agent lease so that at most one context traces a given agent at a time — a second, concurrent request runs untraced rather than writing into the shared buffer.

Technical Details

  • Adds a process-global per-agent buffer manager in source/lib/rocprofiler-sdk/thread_trace/shared_trace_buffer.{hpp,cpp}.
  • thread_trace::initialize() runs a pre-pass that registers every context's per-agent buffer_size before any buffer is built, so the shared buffer is sized to the max; finalize() frees the buffers once.
  • TraceMemoryPool::Alloc returns the shared device buffer (one per ring slot via output_buffer_index, so triple buffering keeps distinct slots); TraceMemoryPool::Free skips shared buffers to avoid a double-free across the contexts that reuse them.
  • Adds a per-agent trace lease in source/lib/rocprofiler-sdk/thread_trace/shared_trace_lease.{hpp,cpp} enforcing the "one active trace per agent" invariant the shared buffer relies on: the first active trace on an agent atomically claims it (owner-keyed, re-entrant refcount) in get_control(), and a different context requesting a trace while the agent is held fails fast — get_control() returns null and that context runs untraced instead of concurrently writing the shared buffer. The lease is released when the agent's last in-flight trace ends (iterate_data / stop / destructor).
  • The buffer and lease managers keep their process-global state in common::static_object rather than plain namespace-scope globals: the state is freed by finalize() (a registration::finalize() atexit handler), and on the attach path a plain global's destructor can run before finalize() — a use-after-free. static_object is destroyed by destroy_static_objects() (sequenced after finalize()), so the state outlives teardown without leaking.

JIRA ID

JIRA ID : AIPROFSDK-102

Test Plan

  • Unit: thread-trace-packet-test (including a new shared_buffer_reuse case covering cross-context reuse, distinct ring slots, and max sizing) and thread-trace-producer-consumer-test.
  • Integration: ATT single, multi, agent, large-buffer, before-hsa-init, and triple-buffer (consistency, hammer, slow, multiple-cmds) on MI300, ROCm 7.2.3.
  • Flakiness: repeated reruns via ctest --repeat until-fail:10.

Test Result

Unit tests pass reliably across repeated runs on MI300 (ROCm 7.2.3): thread-trace-producer-consumer-test 25/25 and thread-trace-packet-test 10/10. Integration tests single/multi/agent/large-buffer/before-hsa-init and triple-buffer slow/multiple-cmds pass 10/10 repeats each. The triple-buffer-consistency and triple-buffer-hammer tests can intermittently hit the pre-existing SQTT shutdown GPU hang tracked in AILIKFD-39; this was verified to reproduce on develop without this change (identical reason :GPU Hang), so it is unrelated to the buffer-sharing rework and is not addressed here.

Submission Checklist

@minseobshin11 minseobshin11 changed the title Per-agent shared trace buffers for ATT [ATT] Improved buffer management: per-agent shared trace buffers Jul 8, 2026
@therock-pr-bot

therock-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
🌿 Branch Name ✅ Pass
📝 PR Title/Description ✅ Pass
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

🎉 All policy checks passed!

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

@therock-pr-bot

therock-pr-bot Bot commented Jul 8, 2026

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.

@minseobshin11
minseobshin11 force-pushed the users/mishin/att-buffer-management branch from 6cf6b23 to e63ffa3 Compare July 8, 2026 20:32
@github-actions

github-actions Bot commented Jul 8, 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 force-pushed the users/mishin/att-buffer-management branch from e63ffa3 to 6b38dfe Compare July 10, 2026 11:16

@ApoKalipse-V ApoKalipse-V left a comment

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.

I think we shouldn't have two different code paths, one shared and one non-shared.
Ideally, there should be a single source of allocation. If that source cannot find an existing buffer satisfying the size requirements, then it can create a new shared one.
That way, we dont need to be checking if some pointer is shared.

Comment thread projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/aql_packet.hpp Outdated
std::mutex&
get_mutex()
{
static auto* _mutex = new std::mutex{};

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 the static mutex pointer?

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.

was trying to use lazy allocation/construct on first use, but the codebase does have better convention of using static_object - modified.

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.

That is not what I meant - Is not affected by the global destructor in the same way.

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.

oh yea the previous approach did leak the new std::mutex and never destroyed it - I've changed it to static_object which gets destructed after finalize()

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.

its fine but unnecessary. The mutex does not need to be a pointer or a static_object , it can be just a regular object.

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.

keeping it static_object to outlive finalize() - regular namespace scoped mutex (Sychronized) can be destroyed before finalization and was failing attach-once-att

@ApoKalipse-V ApoKalipse-V left a comment

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.

One more thing: We need a integration test that allocates hundreds of contexts (say with varying buffer size, target_cu, se_mask, simd_select, etc), runs each once and ensures it's all good. Note that not every target_cu is guaranteed to work, so the condition should be that "most" configs work for CUs in [0,1,2,3]
Allocating ~400 contexts of 1GB each would be impossible in the old implementation, so we can know for sure this works.

@minseobshin11

Copy link
Copy Markdown
Contributor Author

One more thing: We need a integration test that allocates hundreds of contexts (say with varying buffer size, target_cu, se_mask, simd_select, etc), runs each once and ensures it's all good. Note that not every target_cu is guaranteed to work, so the condition should be that "most" configs work for CUs in [0,1,2,3] Allocating ~400 contexts of 1GB each would be impossible in the old implementation, so we can know for sure this works.

~400 contexts is not possible due to each context alloacating hsa queue, created a PR that addresses this issue. #8644

@ApoKalipse-V ApoKalipse-V left a comment

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.

Changes looks good, but we are missing two things:
(1) A lock to ensure no two contexts can use the same buffer at the same time. Right now, there is nothing preventing that except context management. We need stronger enforcement.
We have edge cases such as "disable dispatch thread trace while a kernel is running, then enable device thread trace".
(2) The CPU buffers also need to be reused. This can be a distinct PR (quick scan was merged only recently), but ideally reusing the same locking mechanism.

@minseobshin11
minseobshin11 force-pushed the users/mishin/att-buffer-management branch from 6fd101b to 00a0b6f Compare July 20, 2026 22:54
Co-authored-by: Cursor <cursoragent@cursor.com>
@minseobshin11
minseobshin11 force-pushed the users/mishin/att-buffer-management branch from 00a0b6f to 7178390 Compare July 20, 2026 23:49
@minseobshin11
minseobshin11 force-pushed the users/mishin/att-buffer-management branch from 40458a3 to 90c1518 Compare July 22, 2026 04:01
@minseobshin11

Copy link
Copy Markdown
Contributor Author

added a per-agent trace lease - the first active trace on an agent claims it and if a different context tries to trace the same agent, returns null.

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