Repository navigation
fix(agc): stop and join the driver's thread pools at shutdown - #6641
oneandonlydean merged 12 commits into
Conversation
CloseRegistrationPreparation took RegistrationPreparePool::Instance(), which creates the pool and starts its detached workers (half the hardware threads, at least two) when nothing was queued for preparation yet. In a process that prepared nothing in the background, the driver's shutdown started the workers only to close the pool, and the process exited while they were still starting. Their first waits take the global lock of winpthreads' statically initialized condition variables; ExitProcess killed them holding it, and the destructor of the next static condition variable that was never waited on spun on it forever at DLL detach (~DeviceUseGate in ~Driver, or one of libkernel's). On GitHub's Windows runners agc_interpolant_mapping hung that way in 15 of 60 rounds of eight test runs. Shutdown now closes only the pools that exist, and a pool first used after it starts no workers, so later preparations run on the caller as they did on a closed pool.
Closing a preparation pool waited until its queue was empty, but its detached workers stayed parked on the queue until ExitProcess killed them. A pool first used shortly before shutdown still had workers starting at exit, inside the first waits that take winpthreads' global condition-variable lock, which the next DLL-detach destructor of an unwaited static condition variable then waits for forever. The workers are now joinable and leave once their pool is closed and idle, and Close joins them, so after the driver's shutdown no preparation worker is running or starting.
…worker at shutdown Two more detached driver threads were never stopped: the helpers of the parallel compare pool, started at the first large compare, and the pipeline library's job thread, started at the first link-time optimized pipeline. A process that exited right after starting them killed them inside the first waits that take winpthreads' global condition-variable lock, as with the preparation workers. The driver's shutdown now stops both and joins them, after the device is reset. A compare in progress finishes first and later compares run on the caller. The library worker finishes the job it is running, the jobs still queued are dropped and counted off their device, and later pipelines are not optimized in the background.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
Driver::stop reset the device and then stopped the worker. No job could outlive the device, because ClearPipelineLibraries waits for the device's jobs to finish, but that was only visible inside the wait. The worker now stops first: a running job finishes, queued jobs are dropped, and no job runs once the device's teardown starts. agc_driver_draw_cache_reuse checks that the driver's own shutdown leaves no library worker running.
The committer of pipelined draws, started at the first pipelined submission, was never stopped. The driver's shutdown joined the queue workers and left the committer parked on its queue, and a commit the last submission left queued, such as its deferred completion, could still run after the shutdown had reset the device. A process that exited right after its first pipelined submissions could kill the committer inside its first wait on the queue, which takes winpthreads' global condition-variable lock, as with the preparation workers. The driver's shutdown now stops the committer after joining the queue workers and before taking the GPU lock. The committer runs the commits still queued and leaves, and the shutdown joins it. A commit enqueued after that runs on the caller.
The shader disk cache's writer, started with the cache at the first shader lookup, was detached and never stopped. It stayed parked in a timed wait on its queue until the process exit killed it, and entries still queued at exit were never written. A process that exited right after its first shaders could kill the writer inside its first wait, which takes winpthreads' global condition-variable lock, as with the driver's own threads. The writer is now joinable. The driver's shutdown stops it after the other driver threads: the writer writes the entries still queued and leaves, and the shutdown joins it. A store after that is written on the caller, and a cache first used after the shutdown starts no writer. ShaderDiskCache::Shutdown joins the names the cache key check has reviewed, since it changes nothing a recompile produces.
github-actions
Bot
added
needs rebase
|
Independent Windows native composition check of #6641 at There is one textual merge conflict in std::map<VkDevice, std::size_t> pending;
std::map<VkDevice, std::size_t> activeBuilds;
std::thread worker;
bool stopping = false;No other manual resolution or production alteration. The resolved local source tree is Windows x64 / Intel Core i7-8700, supported WinLibs GCC 15.2.0 r7 UCRT, CMake 4.4.4, Release, BUILD_TESTING enabled, SPIRV-Tools disabled. Built the real
Limits: selected native targets/tests only, not a full all-target build or full CTest sweep. No local natural-exit hang was reproduced on main, and 60 successful exits do not prove absence of every shutdown race. No real Vulkan/GPU, title/FPS or Linux/macOS validation. I did not repeat the independent mocked-Vulkan part-error/link-error fixtures from #6827 on this composition; the official pipeline-clear regression does pass. Attached: source probe, DLL hashes, exact source tree IDs, logs, raw repetition results and reproduction instructions. No binaries or game data. |
yabanana
left a comment
yabanana
left a comment
There was a problem hiding this comment.
What this change does: it stops the driver workers before their resources are torn down, and makes lazy initialization respect shutdown. I reviewed head 43830b93bdc8a58d436c7d0d3d8c9493eecdd8f1 and added focused checks for the in-flight and first-use cases.
Additional tests: patch based on this head.
--joinednow holds both preparation tasks open while another thread closes the pools. It checks that closing waits, all workers are gone on return, repeated close is safe, and later work runs on the caller.--late-shader-cacheinitializes the cache for the first time after driver shutdown. It checks that no writer starts and stores still reach disk. This reuses the existing cache-write test and its Windows PATH setup.
Verification:
- Ubuntu 24.04 amd64, GCC 13.3 Release, QEMU 10.2.3 with
-cpu max: all six shutdown CTests pass 10/10 fresh processes each (60 runs). No CPU guard, assertion, or CTest timeout was changed. - These Linux runs use integration
dd8824b63629d68b7f205e66bcc578abb4a42e60: this PR, #6814, the two test patches, and the existing #6789 fixture-storage correction. The only production merge conflict isLibraryStore; keep #6814'sactiveBuildsalongside this PR'sworkerandstopping, and remove the replacedstartedfield. - The combined pipeline lifetime regression, journal and address-space teardown tests pass.
agc_driver_pipeline_optimized_readback_d32executes on llvmpipe (LLVM 20.1.2, 256 bits): one fast draw, one optimized draw, one optimized creation, exact readback and the worker-shutdown check pass. - Separately, the unchanged
ParallelCompare.cppfrom this PR passes 20 native ARM64 processes under ThreadSanitizer on macOS 27.2 / Apple Clang 21. Four callers each make 100 comparisons while helpers stop; half the processes also race the pool's first use with shutdown. Every comparison is checked, no helper survives, and TSan reports nothing. Probe source and commands. - Conventions and whitespace checks pass.
The general graphics CTest exceeds 30 s in this emulated setup after #6789 fixes the earlier fixture error. The same binary completes its assertions in a separate, bounded 90 s diagnostic run; it reports write-watch-dependent cases unavailable. That diagnostic is not a full CTest pass, and the repository timeout remains unchanged.
Verdict: no new blocker found in the exercised shutdown paths; the additional regression tests are ready to reuse.
Not checked: Windows execution, physical GPUs, title exits, or full Linux/Windows CI. The combined result does not make either PR depend semantically on the other.
AI-assisted: yes.
…tops The driver's static is destroyed from its DLL's atexit table at process exit, after ExitProcess has ended the async log's worker. Its stop() then flushed the log through Flush(), which takes the log's lock and waits for every queued line, so it never returned when the worker died holding that lock or with lines queued. boykopovar#6800's bounded flush runs after it, because the log registers it while the driver is being constructed. stop() now uses that bounded flush, which reads the counters without the lock and gives up after EndFlushLimit, marking the log abandoned. A new shutdown test mode holds the log's lock from another thread and checks that the driver's shutdown still returns.
…orker starts A condition variable that winpthreads initializes statically is set up on its first wait, under one global spinlock for all such variables. The async log's worker made that first wait itself, right after the driver started it, so a process that exited soon after starting the driver, such as guest_shader_link, could end with ExitProcess killing the worker inside that setup. The spinlock then stayed held, and the next destructor of a never-used condition variable, in ~Driver, spun forever. The log's constructor now waits on each of its condition variables once with no timeout before it starts the worker, so the worker's waits never take that spinlock. A new shutdown test mode exits right after the driver starts.
|
@simonbronk thank you for the Windows composition check with #6814 and #6827, and for the thread counts. Your @yabanana thank you for the review and the two test modes. They're on the branch as your commit, 8e671f4, and pass 10 of 10 on GitHub's Linux and Windows runners there, and 50 of 50 with the commits after it. @MarcZX100 since your review, the branch merges |
|
@EagleBearerYS #6814 is merged now (7a2435e), so this conflicts in |
main's 81e3db8 counts each device's active library builds (activeBuilds), so ClearPipelineLibraries waits for them. Keep it beside this branch's joinable worker and stopping flag, which replace started.
|
@oneandonlydean done: a6ff397 merges |
main 4827f36 adds the shader disk cache include to the driver lifecycle (boykopovar#6641) where this branch adds the guest arena's. Keep both.
5 participants
What
On Windows, a test process that uses the AGC driver can hang at exit, after its test has passed.
mainfailed only becauseagc_interpolant_mappingtimed out on Windows (***Timeout 120.01 sec, 1 of 1,304 tests).agc_interpolant_mappinghung in 11 and 15 of 60 rounds of eight runs, andagc_driver_readonly_resummarizehung too. Only ctest's timeout ends them.main82ddae7,guest_shader_linkhangs at exit on GitHub's Windows runner too, within a few runs (run). Commits 7 and 8 fix its two causes.exit→LibcExit→LibcRunShutdown→Driver::stop, thenExitProcess.Why it hangs.
ExitProcessterminates every other thread first, then runs each module'sDLL_PROCESS_DETACH, where the static destructors run (as in Windows: guest _Exit hangs in FlipQueue's destructor because ExitProcess still runs module detach routines #2579).cond_locked). The same lock is held across a condition variable's first wait, while that wait creates its semaphores and critical sections.~DeviceUseGatein~Driver, or one of libkernel's.pthread_spin_lock, called frompthread_cond_destroy, on that lock. Its word reads 0, meaning held, with only the main thread left.Which threads. Both tests were run under gdb, stopped at
RtlExitUserProcess, and every live thread was dumped. In 2 of the 6 hangs over 160 exits, the lock was already held at that point by a shader preparation worker that had just started:RegistrationPreparePool::Main's firstready.wait;PerformanceTrace::Configured's static-initialization guard, which waits on libstdc++'s guard condition variable.Starting preparation workers were alive at 27 of the 160 exits. They came from the shutdown itself:
CloseRegistrationPreparationtookRegistrationPreparePool::Instance(), which creates the pool, with half the hardware threads and at least two, when nothing had been queued.Closethen returned at once, andmainreturned while the workers were still starting.guest_shader_link: two more causes. That test constructs the driver in its firstsceAgcLinkShaders, a few milliseconds before it exits.main, the hung process's only thread waits inpthread_mutex_lockon the async log's lock, insideDriver::stop→FlushAsyncLog_nid_no_patch→AsyncLog::Flush. The driver's static is destroyed from its DLL's atexit table afterExitProcesshas ended the log's worker, andFlushtakes the log's lock and waits for every queued line. fix(agc): bound the async log's exit flush #6800's bounded flush runs too late there: the log registers it while the driver is being constructed, so it runs after the driver's destructor.~Driver's destructor of a condition variable, spinning on winpthreads' lock as above. Stopped atRtlExitUserProcess60 times, the process always had one thread of ours alive, the async log's worker, and in 38 of the 60 exits it was inside its first wait's setup of a condition variable (pthread_cond_initunderpthread_cond_timedwait), which holds that lock.The fix, in eight commits (the branch also merges
main82ddae7, for #6800's bounded flush):fix(agc): shutdown closes only the preparation pools that exist, as it already did for the prebuild pool. A pool first used after shutdown starts no workers, so later preparations run on the caller, as they did on a closed pool.fix(agc): the preparation workers are joinable. They leave once their pool is closed and idle, andClosejoins them, so after shutdown no preparation worker is running or starting.fix(agc): the two other detached driver threads are stopped and joined at shutdown, the pipeline library's job thread before the device is reset and the parallel compare helpers after it:fix(agc): the pipeline library's job thread is stopped before the device is reset, not after (from review). Stopping it after the reset was already safe, since the device's teardown waits for the device's library jobs before it destroys the pipelines and the device, but now no library job runs once that teardown starts. The compare helpers stay after the reset: they compare guest memory only.fix(agc): the draw pipeline's committer (pipelined draws, on by default) is stopped and joined right after the queue workers, before the GPU lock is taken. It runs the commits still queued, so a deferred completion no longer runs after the device is reset, and a commit enqueued after shutdown runs on the caller.fix(agc): the shader disk cache writer is stopped and joined after the compare helpers. Entries still queued at exit are now written instead of lost, a store after shutdown is written on the caller, and a cache first used after shutdown starts no writer. The shader cache key check's reviewed names gainShaderDiskCache::Shutdown, which changes nothing a recompile produces.fix(agc):Driver::stopflushes the async log through fix(agc): bound the async log's exit flush #6800's bounded flush instead ofFlush. It reads the log's counters without its lock, gives up after 2 s and marks the log abandoned. Other callers keep the full flush. A title's exit stops the driver through libc's shutdown while the log's worker still runs, so there the bound matters only if the worker stalls. The profile output'sStopalso takes a lock at that point; it exists only whenAPS5_PROFILE_*output is on, and is left as is.fix(agc): the async log's constructor waits once, with a zero timeout, on each of its three condition variables before it starts the worker, so they are set up on the constructing thread and the worker's waits never take winpthreads' lock.Linux and macOS: the pools are stopped the same way, but the hang needs winpthreads' lock, so it is Windows-only.
Threads outside the driver: commits 5 and 6 come from an after-the-fact review of every thread alive at exit. Threads outside the driver can still meet the same lock: libc's exit waiter, where guest threads park after
ProcessShutdown; libkernel's APR workers; the alarm thread; and AudioOut's SDL devices, which use Win32 primitives. They're left to separate changes.Tested
agc_driver_shutdown_threads(tests/execution/ShutdownThreads.cpp), no GPU:--joined: workers of both pools that ran a preparation are gone after closing;--compare-helpers: a parallel compare's helpers are gone after stopping, and later compares give the same results on the caller.--draw-committer: a committer runs queued commits off the caller; after the driver's shutdown none is left, the queued commit ran, and a later commit runs on the caller.--shader-cache-writer: with the cache in a temporary directory, an entry stored before the driver's shutdown is on disk after it, no writer is left, and a later store is written at once.agc_driver_pipeline_optimized_readback_*: after the draws, the library worker that built the optimized pipeline is running, and it is gone after stopping.agc_driver_draw_cache_reuse, which draws through the driver with pipeline libraries: after the driver's own shutdown, no library worker is running.ctest --parallel 4, onmain5d9dcc4 with and without the fix): onmain, 11 of 60 rounds hung (run), each anagc_interpolant_mappingexit whose main thread spins in winpthreads under~DeviceUseGatein~Driver, against 0 hangs in 60 rounds (480 exits) with the fix (run), Windows. test(agc): align the indirect arguments block at run time #6452's test fix was applied to both, becausemaindoesn't build in full with MinGW since fix(agc): keep the gate run going past indirect dispatches whose arguments stay on the GPU #5747.agc_driver_shutdown_threadsand its_joinedand_compare_helpersmodes,agc_driver_prepared_shaders,agc_driver_prepared_publicationandagc_driver_preparation_scope_settle: all pass on Linux and Windows (run).precise_waiton Linux andguest_kqueueon Windows, which fail onmainon those runners too.agc_driver_pipeline_optimized_readback_d16,_d16_fast,_d32and_d32_fastpass 20 of 20, and the four colour modes skip on lavapipe; Windows skips the readback tests without a Vulkan device and passes the shutdown tests 20 of 20.agc_driver_draw_cache_reuse, the three shutdown tests and the readback tests pass 10 of 10 (the four colour readback modes skip on lavapipe). On Windows the three shutdown tests pass 10 of 10, andagc_driver_draw_cache_reuseskips without a Vulkan device. The first run's Windows job stops in the full build, atmain'sGpuIndirectArgumentsMisalignedWriterAhead.cpp(alignas(65536)under MinGW), so the second runs the named tests only. The second is also marked failed forguest_kqueueon Windows, which fails in our other Windows runs today on unrelated changes.agc_driver_draw_cache_reusepass 10 of 10 on Linux and Windows, whereagc_driver_draw_cache_reuseskips without a Vulkan device. The new tests use functions these commits add, so the before arm doesn't build. Their red is the branch with only the new call inDriver::stopremoved: "the driver's shutdown left the draw committer running" and "... the shader cache writer running" (on a Linux cloud worker). The run is also marked failed forprecise_waiton Linux andguest_kqueueon Windows, which these commits don't touch.main82ddae7 merged:agc_driver_shutdown_threads_async_log_held: another thread holds the async log's lock, and the driver's shutdown must return within 10 s. With onlystop()'s call put back toFlush, it fails 3 of 3 on Linux and Windows with "the driver's shutdown waited for the async log's lock" (run).agc_driver_shutdown_threads_exit_after_startconstructs the driver and returns at once. With the constructor's setup of the log's waits removed, it times out on Windows on the seventh run, after 6 passes, and passes on Linux, which has no winpthreads (run).--joinedand the late-cache mode (agc_driver_shutdown_threads_late_shader_cache) are yabanana's, from their review.guest_shader_linkloop (rounds of up to 400 runs on the Windows runner; a run left for over 20 s has every thread dumped, and the loop stops after 2 hangs or 4 rounds):main82ddae7 hung in 2 of 2 rounds, after 1 and 2 passes, on the async log's lock (run); with commit 7, 2 of 2 rounds, after 0 and 3 passes, in~Driver's spin (run); with all eight commits, no hang in 4 rounds of 400 runs (run).guest_shader_linkunder cdb, stopped atRtlExitUserProcess, 60 exits): the async log's worker was inside its first wait's setup in 38 of 60 exits with commit 7 (run), and in none of 60 with commit 8 (run).^agcsweeps (run), with test(agc): align the indirect arguments block at run time #6452's test fix applied for that check only:agc_driver_pipeline_optimized_readback_d32and_d16, whose library worker built the optimized pipeline and was gone after stopping.agc_driver_pm4_replay_tool(0xc0000139), also fails onmainon that runner.main52798f7 (a6ff397, which brings fix(agc): drain library prebuilds before device replacement #6814), on GitHub's runners (run): the three shutdown tests,agc_driver_prepared_shaders,agc_driver_prepared_publicationand theagc_driver_pipeline*tests pass 3 of 3 on Linux, where 4 skip, and on Windows, where the Vulkan ones skip (15). The only suite failure isguest_kqueueon Windows, which fails on that runner without this PR too.python3 tools/check_conventions.py --base origin/main: passed.Overlap: #6814 (drain library prebuilds before device replacement) is merged, and a6ff397 merges
maininto this branch. Their one conflict wasPipelineLibrary.cpp'sLibraryStore: #6814'sactiveBuildsstays beside this PR'sworkerandstopping, andstarted, which this PR replaces, is gone. Git merged the rest itself,ShaderRegistryamong it, where #6814'sAwaitLibraryPrebuildssits beside this PR'sRegistrationPreparationThreadsandCloseAll. simonbronk built that composition on Windows with #6827 (comment), and yabanana's Linux integration with #6814 takes the same resolution.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)agc_interpolant_mapping,agc_driver_readonly_resummarizeandguest_shader_linkthat hang at exit