Repository navigation
QVAC-27853 fix(vulkan): destroy devices and instance before driver unload - #148
Conversation
…load The Vulkan backend never destroyed its logical devices or its instance: each device's own buffer type holds a shared_ptr to it, so ~vk_device_struct never ran, and vk_instance_t has no destructor. On the NVIDIA Tegra driver (Jetson Orin, L4T 36.4, driver 540.4.0) that crashes the process at exit. The driver libraries register their exit handlers when vkCreateInstance loads them; libEGL_nvidia's handler unmaps libnvidia-eglcore while a driver worker thread, which only vkDestroyDevice stops, still runs its code. Parakeet and Audio8 CLIs finished their work and then died with SIGSEGV. - ggml_vk_register_exit_teardown() registers an atexit handler after the instance is created and after each device is created. Exit handlers run in reverse order of registration, so the handler destroys every device and the instance before the driver's own handlers run. A static destructor is not enough: it is registered when the library loads, before the driver, and so runs after it. Windows keeps the old behaviour, since exit handlers of a DLL run under the loader lock. - vk_device_struct::shutdown() holds the former destructor body, runs once, waits for the device to go idle first and skips a device that was never created. The VMA allocator is destroyed only when it holds no allocations, so exiting with live buffers does not trip VMA's leak asserts in builds without NDEBUG. - After the teardown, buffers, pinned host memory and backend contexts that the application frees from its own exit handlers skip their Vulkan calls instead of calling into a destroyed device. - test-vulkan-teardown initialises Vulkan0, allocates and frees a buffer and exits; a second ctest entry exits with the buffer still allocated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GustavoA1604
left a comment
There was a problem hiding this comment.
[critical][pr-review-core] src/ggml-vulkan/ggml-vulkan.cpp — line 9327 (ggml_vk_shutdown_devices();)
An application cleanup handler registered before backend initialization now runs after device destruction. If it frees a retained Vulkan event, ggml_backend_vk_device_event_free() still calls destroySemaphore() and destroyEvent() on the destroyed device, which can crash during exit. Scheduler cleanup also reaches this path. Guard event destruction with device->destroyed, while still deleting the CPU objects, and add a regression test for this ordering.
[medium][coding-standards] tests/test-vulkan-teardown.cpp — line 25 (ggml_backend_free(backend);)
Both tests free the backend before teardown; the kept-buffer variant never frees its buffer. Neither exercises the new guards for resources freed after device destruction. Add a cleanup callback registered before backend initialization that frees retained device buffers, pinned host buffers, and the backend after teardown.
An exit handler registered before backend initialization runs after the device teardown. Freeing a retained event there destroyed its semaphore and Vulkan events on the destroyed device; event_free now skips the Vulkan calls once the device is destroyed and still deletes the CPU objects. The same ordering also reached freed memory before any Vulkan call: the registry kept its device objects and contexts in function-local statics, destroyed at exit before such a handler runs. They now live for the whole process, as the CUDA backend's device contexts already do. test-vulkan-teardown --free-after-teardown registers its cleanup before backend initialization and frees a device buffer, a pinned host buffer, a recorded event and the backend after the teardown. Without the event-free guard it aborts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both findings are addressed in 03171fa. [critical][pr-review-core] src/ggml-vulkan/ggml-vulkan.cpp — line 9327 (ggml_vk_shutdown_devices();) Fixed: ggml_backend_vk_device_event_free now calls ggml_vk_destroy_event_handles (semaphore, free and submitted events, the recorded event) only while device->destroyed is false, and deletes the CPU objects in every case. The same exit ordering also reached freed memory before any Vulkan call: the registry kept its device objects and device contexts in function-local statics that the runtime destroyed before such a handler ran. They now live for the whole process (ggml_vk_never_destroyed), as the CUDA backend’s device contexts already do. The regression test is the new ctest test-vulkan-teardown-free-after-teardown (see below); without the guard it aborts. [medium][coding-standards] tests/test-vulkan-teardown.cpp — line 25 (ggml_backend_free(backend);) Fixed: test-vulkan-teardown --free-after-teardown registers free_retained with std::atexit before ggml_backend_load_all, then initializes the backend, allocates a device buffer and a pinned host buffer, records an event, and returns; the handler frees the event, both buffers and the backend after the device teardown. It is wired as its own ctest in tests/CMakeLists.txt. The kept-buffer variant keeps leaking its buffer on purpose: it is the “buffer still alive at teardown” case the first commit fixed, and the new variant is the one that exercises the guards. The test passes on the RTX 5090 box and on the Jetson Orin, as reported by the session that ran it. |
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
|
/review |
test-vulkan-teardown --keep-buffer still holds its device buffer through the device teardown, but now hands it to the exit handler registered before ggml_backend_load_all, which frees it after the teardown. No variant of the test leaks anymore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I reviewed the current head (
I attempted a local dynamic Vulkan build, but CMake configuration stopped because |
…Device The exit teardown destroyed the device while retained buffers, backends and events still owned Vulkan objects, which violates VUID-vkDestroyDevice-device-05137. Each device now tracks its live backend contexts, events and VMA buffers. shutdown() cleans up the backends, destroys the event semaphores and events and releases the remaining buffers before it destroys the device; the CPU-side wrappers stay valid and later frees skip the Vulkan calls. ggml_vk_cleanup() also destroys the perf-logger query pool, which leaked before. vk_instance is now never destroyed. With GGML_BACKEND_DL the Vulkan module loads after a cleanup handler registered earlier, so its static vk_instance was destroyed before that handler freed an event or a pinned host buffer through it. The teardown ctests run under VK_LAYER_KHRONOS_validation when the layer is installed and fail on any vkDestroy* validation error. Without the release step the kept-buffer and free-after-teardown variants report 4 and 20 VUID-vkDestroyDevice-device-05137 errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both fixed in 7cceb37. P1, children destroyed after vkDestroyDevice. Each vk_device_struct now tracks its live backend contexts, events and VMA buffers. shutdown() cleans up the backends, destroys the event semaphores and events and releases the remaining buffers before it destroys the device; the CPU-side wrappers stay valid and a later free skips the Vulkan calls. ggml_vk_cleanup() also destroys the perf-logger query pool, which leaked before. The teardown ctests now run under VK_LAYER_KHRONOS_validation when the layer is installed and fail on any vkDestroy* validation error: without the release step the kept-buffer and free-after-teardown variants report 4 and 20 VUID-vkDestroyDevice-device-05137 errors, with it none. P2, vk_instance lifetime with GGML_BACKEND_DL. vk_instance is never destroyed any more, so a cleanup handler registered before the Vulkan module loads can still free events and pinned host buffers through it. Verified: ctest -R test-vulkan under validation on the RTX 5090 and the AMD iGPU (5/5, three runs each); the teardown test with the backend loaded as a module in all three modes (exit 0, no validation errors); on Jetson Orin 10/10 parakeet and audio8 runs with complete output and unchanged speed, and 5/5 ctests under its 1.3.204 layer. Your configure failure is reproducible: with dynamic backends and tests both on, test-vulkan-status links ggml-vulkan directly and CMake stops; that predates this PR, so I built the module-loaded teardown test by hand. |
|
/review |
Summary
atexithandler registered after the instance and each device are created, so it runs before the driver's own exit handlers.vk_device_structdestructor into an idempotentshutdown()that waits for the device to go idle, skips a device that was never created, and destroys the VMA allocator only when it holds no allocations.test-vulkan-teardown: initialiseVulkan0, allocate and free a buffer, exit; a second ctest entry exits with the buffer still allocated.Error corrected
On the Jetson Orin (L4T 36.4, NVIDIA Tegra Vulkan driver 540.4.0,
nvgpu), the speech CLIs finished their work, wrote their results, and then died with SIGSEGV at exit. The weekly bench therefore recorded 5 of the Jetson's 13 Vulkan cells as failed although their measurements were complete. Affected: Parakeet TDT, Parakeet Unified, Audio8 with the F16 codec decoder, Audio8 cloning with the F16 encoder.The backend never destroyed its devices or its instance. Each device's own buffer type holds a
shared_ptrto it, so~vk_device_structnever ran, andvk_instance_thas no destructor. The NVIDIA libraries register their exit handlers whenvkCreateInstanceloads them. At exit,libEGL_nvidia's handler unmapslibnvidia-eglcorewhile a driver worker thread still runs code in it. OnlyvkDestroyDevicestops that thread. A crash-time signal handler showed the faulting pc in the unmappedlibnvidia-eglcorerange on that worker thread, right after the unmap.A static destructor does not fix it. It is registered when
libqvac-speech-ggml-vulkanloads, which is before the driver loads, so it runs after the driver's handlers. That version crashed in 20 of 20 runs. The fix registers the teardown withatexitafter the driver is loaded.Windows keeps the previous behaviour, because the exit handlers of a DLL run under the loader lock and Vulkan calls there can deadlock.
Validation
parakeet --bench, unpatchedspeech@de1d0f5cparakeet --bench, this branchaudio8-cliLM F16 + codec decoder F16, this branchctest -R test-vulkanpasses 4/4 three times on the Jetson. It also passes on an RTX 5090 (proprietary driver) and on an AMD Raphael iGPU (RADV).test-backend-ops -o MUL_MATand-o ADDpass on the RTX 5090.ADDcases (m=100,n=37,k=96, modes 1 and 2) give NaN on the AMD iGPU. The unpatchedspeechtip shows the same result.Follow-up
ggml-speechregistry cut at the merge commit onspeechfor the prebuilt addons. The weekly bench builds thespeechhead directly, so it picks this up on its next run.🤖 Generated with Claude Code