Repository navigation
(fix) avoid creating one utility queue per PC sampling instance - #358
samcrater-amd wants to merge 2 commits into
Conversation
PcSamplingCreateFromId and UpdateTrapHandlerWithPCS write the per-session init data, zero the trap buffers and write the trap handler TMA region with DmaCopy/DmaFill. Those go through blits_[BlitDevToDev], a blit kernel on queues_[QueueUtility], so configuring PC sampling on an agent creates the utility queue there even if the process never dispatches to that agent. Add pcs_blit_, a blit kernel on queues_[QueuePCSampling] (which PC sampling already creates on every configured agent), and use it for these three operations. The memory paths are unchanged, so this is equally valid with and without large BAR. On its own this does not remove the utility queue; see the next commit.
queues_[QueuePCSampling] is created through CreateInterceptibleQueue -> GpuAgent::QueueCreate, which touches queues_[QueueUtility] before creating any queue. So creating the PC sampling queue alone still creates the utility queue on every agent PC sampling is configured on. Skip that pre-creation while this thread creates the PC sampling queue. The utility queue is still created lazily on first real use (cache invalidation, blits, or the first user queue on the agent); only the early exhaustion check for it is deferred. rocprofiler-sdk configures PC sampling on every visible agent in every process. With 8 ranks per node, one process per GPU, and GPU_MAX_HW_QUEUES=8, ROCm 7.0.2 maps 10 + 8 x 2 = 26 compute queues per GPU, over the 24 the HWS runlist holds, and the runlist is oversubscribed. With both commits each process that does not use a GPU holds only its PC sampling queue there: 10 + 8 x 1 = 18. Measured on 2 x 8 MI350X (ADS QPS HSDP 2x8, stochastic PC sampling, same nodes): PC sampling off, ROCm 7.0.2 9 queues/GPU 14,631 QPS PC sampling on, ROCm 7.0.2 26 queues/GPU, oversub. 842 QPS PC sampling on, with this fix 18 queues/GPU 11,687 QPS
vlaindic
left a comment
vlaindic
left a comment
There was a problem hiding this comment.
Couple questions before approving.
| // Set while this thread creates queues_[QueuePCSampling] through QueueCreate. | ||
| static thread_local bool creating_pc_sampling_queue = false; |
There was a problem hiding this comment.
Wondering why we do this per thread and not per process?
There was a problem hiding this comment.
thread_local because we want to neutralize exactly one call of QueueCreate: the one where PCS is initializing the utility queue. other threads may be creating other queues at the same time.
| // Deferring longer risks exhausting queue count before ISA upload and invalidation capability is | ||
| // ensured. | ||
| queues_[QueueUtility].touch(); | ||
| if (!creating_pc_sampling_queue) queues_[QueueUtility].touch(); |
There was a problem hiding this comment.
Question: For a process that uses PC sampling and offloads workloads onto the device, are we only going to the option 1 or options 2:
- create PC sampling queue only
- create both PC sampling and utility queue?
I think we should use the option 2, so that the longer operations goes through the utility queue and not the PC sampling queue with max priority.
There was a problem hiding this comment.
Option 2 is what happens; we are just preventing the utility queue from being proactively created on PCS init and sending the 3 init operations through the PCS queue.
vlaindic
left a comment
vlaindic
left a comment
There was a problem hiding this comment.
Approving.
@shwetagkhatri could you please take a quick look. This impacts only the ROCm 7.0.2, no changes to the develop. Thanks!
shwetagkhatri
left a comment
shwetagkhatri
left a comment
There was a problem hiding this comment.
I don't see anything blocking here. The scope to the rocm-rel-7.9.2.1 branch looks correct.
develop branch - still has the unguarded queues_[QueueUtility].touch();. I have put the port up as a draft here - https://github.com/ROCm/ROCR-Runtime/pull/14437 - so it does not fall through the gap. I havent done any testing on this yet. If you want to take it over, I don't mind. I can close it.
I have few more comments - looking for your response. Will approve it otherwise. I hope this patch is thoroughly tested for this release branch for customer.
| @@ -1764,7 +1784,7 @@ hsa_status_t GpuAgent::QueueCreate(size_t size, hsa_queue_type32_t queue_type, u | |||
| // Ensure utility queue has been created. | |||
| // Deferring longer risks exhausting queue count before ISA upload and invalidation capability is | |||
| // ensured. | |||
There was a problem hiding this comment.
// Ensure utility queue has been created.
// Deferring longer risks exhausting queue count before ISA upload and invalidation capability is
// ensured. PC sampling setup never dereferences the utility queue, so that path is exempt.
There was a problem hiding this comment.
One open question, not a blocker - On the PM4-fallback PC sampling path, creating a session used to ask for two hardware queues - the PC sampling queue and the utility queue that - QueueCreate - pulled in on the way - and now asks for one, with the utility queue created later by whoever first dereferences it. Say - on a device that's already out of queue room that moves where the failure is reported: it used to fail inside - PcSamplingCreateFromId, and can now fail at code object load time instead, as OUT_OF_RESOURCES out of hsa_executable_freeze, because RegionMemory::Freeze reaches the utility queue for a code segment - either through DmaCopy call on the copy branch or through InvalidateCodeCaches call - either way.
Same failure, but the error no longer names the actual point of root cause of that failure - that consumed the queue. I have not tested this, but it could happen. If you can generate a test case and fix this issue, that will be great. its not a blocker though. Atleast add a comment in your new code.
| [blit_lambda, this, eng]() { return blit_lambda(true, queues_[QueueUtility], false, eng); }); | ||
| } | ||
|
|
||
| pcs_blit_.reset([this]() { |
There was a problem hiding this comment.
Non-blocking, Just adding a note here
pcs_blit_ owns its own lock_, so the setup copies no longer serialise against blocking blits_[BlitDevToDev] users. I dont think this is a defect - but its worth one line in the PR description.
dayatsin-amd
left a comment
dayatsin-amd
left a comment
There was a problem hiding this comment.
Please post PR against develop branch. Once it is approved for develop branch, then we can cherry-pick into 7.0.2.1 branch.
Also, this is technically not a fix, but an improvement, please change PR title to:
feat(rocr): avoid creating one utility queue per PC sampling instance
Motivation
Addresses AIPROFSDK-1170, where a very large reduction in runtime performance was noted when running PC sampling on a torchrun workload.
Technical Details
torchrun creates one process per GPU and PC sampling configures itself on every device, leading to 8 PC sampling instances on each device. Each PC sampling instance created a PC sampling queue and a utility queue. Collectively this exceeded the supported number of queues on the devices and created a large slowdown.
PC sampling only used the utility queue for a few small things, so those tasks were instead moved to the PC sampling queue and left the utility queue not created.
A note on the approach: torchrun does not limit each process' GPU visibility or provide any promises as to which GPU the process will perform its work on, so there is no external way for rocprof to determine which GPUs will be used and thus no way to selectively enable PC sampling.
Test Plan
Ran the repro workload on the same hardware, with and without PC sampling and with and without the fix.
Test Result
Speed was greatly improved relative to problem case.
Submission Checklist