Repository navigation
feat(hsakmt): add event age support on Windows and spin SDMA completion wait - #14978
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Required] PR title format: "Use event age under Windows and use spin in SDMA thread" does not follow the required type(component): format. ROCm squash-merges; this title becomes the permanent commit title. Suggest: feat(hsakmt): add event age support on Windows and spin SDMA completion wait. Note also that these are two logically separate changes (event-age protocol + SDMA workaround) — consider splitting the commits or at minimum reflecting both in the title.
CMakeLists.txt, device.h, device.cpp, events.cpp, and runtime.h changes look correct. The new lock-free WaitQueue/WaitEntry protocol in event.cpp is correct (the refcnt_/cnt barrier protocol prevents race conditions; QueryMultipleEvents logic is sound).
e68db44 to
4561f2c
Compare
Because Windows auto-reset event are 1:1, we cannot have multiple signals sharing a single event.
4561f2c to
e3effe7
Compare
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author replies
[Required] PR title still needs type(component): prefix before merge. Suggested: feat(hsakmt): add event age support on Windows and use spin for SDMA completion wait.
All other prior findings resolved (see inline thread replies). One new question on the SDMA timeout fallthrough below.
TheRock Submodule Bump ActivityNewest first
|
Motivation
Right now we use auto-reset events in Windows for notification, and this can cause deadlocks when multiple threads are waiting on the same event. By design, auto-reset events in Windows wake up one waiter and reset the event, so the other waiters will wait for the next rounds of notification. Currently, this shows up as a wait timeout instead of indefinite wait because we artificially cap the timeout to 6 seconds. Using
ROCR_DISABLE_WAIT_TIMEOUT=1can reproduce the hang when running Geekbench 5.Technical Details
This PR introduces an event age implementation for Windows. All waiters before the current age will return, and it also shows better performance under Geekbench 5.
However, this alone does not seem sufficient. The SDMA thread can still miss some notifications at the beginning of the run. We do not know the root cause of this, and for this PR, we simply set it to busy waiting. The p99 wait is 11ms, so it has minimal performance impact under Geekbench 5.
Issue Tracking
JIRA ID : AIRUNTIME-2653
Test Plan
Geekbench 5.
Test Result
With
ROCR_DISABLE_WAIT_TIMEOUT=1andGPU_ENABLE_PAL=0Before:
Run 1
Run2
Run3
After:
Run1
Run2
Run3
Submission Checklist