Repository navigation
fix(agc): keep queue 0's worker running when its wake reason is reaped - #12225
oneandonlydean merged 1 commit into
Conversation
Queue 0's idle wait also ends when completions are pending (boykopovar#7134). After the wait it checked the reasons again and left the wait loop unless one still held, assuming the submission queue was then non-empty. completionsPending() is not under the driver mutex: a submitter's opportunistic reap or WaitIdle can reap the batch between the wake and that check. The worker then left the loop with nothing pending, saw an empty queue, and ended its thread. enqueue starts a worker only when the thread is not joinable, so every later submission to queue 0 stayed queued and its labels were never written. After the wait the worker now goes back to the loop condition, which leaves the loop only when there is work, a failure or a stop.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The worker-loop change is correct: after the condition-variable wake, rechecking ready() at the loop condition handles the case where the non-mutex-protected completion reason is reaped before the second check. The worker remains alive and can process later queue-0 submissions instead of exiting with an unjoined thread.
The reported main-versus-change stress runs, targeted wake/reap-window runs, and same-machine Vulkan validation comparisons provide useful evidence for this narrow race. I independently reviewed the exact head, confirmed the current-main merge preview is clean, and ran git diff --check and python3 tools/check_conventions.py --base 68200c47ea5a5b205204e58269c6ed9cb6acfde6 successfully. The hosted Windows build is still pending; Linux and specialized builds were skipped by the workflow. I did not run the GPU stress tests locally.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
I manually compared the shared QueueWorker.cpp changes flagged by the overlap check. #12225 changes the queue-0 wake loop to retry after a wake whose completion reason has already been reaped; #7978 adds flip-completion submission handling in deferCompletion/completeSubmission. They touch separate functions and hunks, and I found no direct behavior or landing-order dependency in the current patches. This comparison is limited to the source overlap; it does not validate #7978's stale-base integration or tests. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 405 tests)Skipped (39)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Hey @dantdmdarealguy and @TechLevelZero, the newer #6416 report hits Could one failing Linux run capture the expected/observed label, phase (first submit, small submissions, REWIND, gate 77 or slow dispatch 78), actual wait duration, and worker backtrace or exit state? Keeping the exact build/environment and flushing timestamped stdout/stderr would help. Your existing worker-exit diagnostic may supply much of this; the normal two-second assertion should stay unchanged. I tried The first two-second wait can overlap lazy driver-device creation, its float-controls probe and shader preparation. That is a source-supported alternative to distinguish from the |
What
agc_submit_progressfails on CI withsubmission lost its real GPU completion storein builds of unrelated pull requests. Example: Linux job 122561999606 (2026-10-28 00:46Z,main58dfa66 + #11507, a cube-descriptor change), failed after 2.14 s. The test normally takes about 0.2 s.This isn't a slow GPU. In a reproduction the label never arrives, even after 12 s. A stack dump taken while the test waits shows no queue worker thread left in the process. Queue 0's worker had ended its own thread, and every later submission to queue 0 stayed queued.
Cause. In
Driver::run(QueueWorker.cpp), queue 0's idle wait ends when there is work, a failure or a stop. It also ends whenidleEpochchanges or completions are pending; #7134 added that last condition. After the wait, the worker checked those two reasons again:changed.wait(lock, [&] { return ready() || (id == 0 && (idleEpoch... != idleEpochAtWait || completionsPending())); }); if (id == 0) queue0Dormant.store(false, std::memory_order_relaxed); if (id == 0 && !ready() && (idleEpoch... != idleEpochAtWait || completionsPending())) continue; break;completionsPending()reads the recorder's completion counters, which the driver mutex does not protect. A submitter's opportunistic reap (SubmitRecorded(true)) orWaitIdlecan reap the batch between the wake and the second check. The worker then breaks out of the wait loop with no work. A few lines later,if (stopping || ... || pending.empty()) break;ends the thread.enqueueonly starts a worker whenworker.threadis not joinable, and an ended but unjoined thread still is, so no worker runs queue 0 again.Before #7134 the only other wake reason was
idleEpochchanging. That counter only grows, so the second check could not disagree with the first.Change. After the wait, the worker goes back to the loop condition (
continue).while (!ready())leaves only when there is work, a failure or a stop. Otherwise the worker reaps or waits again.Measurements (
agc_driver_submit_progress_testsasctestruns it:APS5_SUBMIT_PROGRESS=1,ANYPS5_REQUIRE_VULKAN=1). Arch Linux in WSL2 on the machine below, lavapipe (the CI runner uses llvmpipe), with 16 copies at once next to 32 busy-loop processes:mainA diagnostic build (diff) prints when queue 0's worker leaves its loop:
worker 0x0 left its loop stopping=0 pending=0came before the test's 2 s deadline.Targeted case: the same diagnostic build releases the mutex and sleeps between the wake and the second check, which widens the window. 4 copies at once, no load:
mainNo test in the repository. The window is a few instructions between two reads made under the same mutex, so it is only hit when the worker is preempted right there. Pinning the test to one or two CPUs doesn't trigger it (0/200 on
maineither way). I found no way to force it from a test without adding a hook to the driver. The diagnostic diff (queue0-worker-exit-diagnostics.diff) and the stress script (stress.sh) are attached.Tested
Windows 11 Pro, Intel Core i9-14900K, 128 GB RAM, NVIDIA GeForce RTX 5080 (driver 32.0.16.1742), WinLibs GCC 15.2.0, Release,
BUILD_TESTING=ON,main3850cc8. Linux: Arch Linux in WSL2 on the same machine, GCC, Release, lavapipe.cmake --build buildandcmake --build build --target libs: both succeed, with no warnings inQueueWorker.cpp.ctest -R "agc_submit_progress|idle_results|same_queue_label_wait|pipelined_indirect_draw|patched_libraries": 8/8 passed.patched_libraries:368 patched libraries loaded, 0 failed.ctest -R "^agc_|^guest_agc": 1103/1104 passed. The one failure isagc_driver_mesh_barycentric_emulation, which fails onmain3850cc8 on this GPU too (since feat(recompiler): read I/J from barycentric built-ins on MoltenVK #9193; the fix is open as test(agc): disable barycentric built-ins in the mesh barycentric emulation test #12062).mainand on this change, withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1andctest -V:was not found but was requested by env var VK_INSTANCE_LAYERS.VK_LOADER_DEBUG=layershowsInsert instance layer "VK_LAYER_KHRONOS_validation".VUID-in either log.main, 1500 runs with 8 at once: 0 failures. I couldn't reproduce it on Windows on this machine.python tools/check_conventions.py --base origin/main: passed.python tools/check_test_registration.py build:1480 test executable(s), 0 built and not run as recorded, none new.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)