fix(device): fence-keyed deferred resource-deletion queue (#101) #102
6 changed files with 312 additions and 1 deletions
|
|
@ -1000,4 +1000,39 @@ bool Device::PreferDirectDeviceWrite(VkDeviceSize size) {
|
|||
// directWriteBudget caches the size-independent decision (CacheUploadStrategy):
|
||||
// 0 -> never direct, max -> ReBAR/UMA so any size, else the small-window cap.
|
||||
return directWriteBudget != 0 && size <= directWriteBudget;
|
||||
}
|
||||
|
||||
void Device::EnqueueDeletion(VkBuffer buffer, VkDeviceMemory memory) {
|
||||
// Nothing to free for an already-null handle — avoids parking dead
|
||||
// entries that ReclaimDeletions would have to skip over.
|
||||
if (buffer == VK_NULL_HANDLE) return;
|
||||
deletionQueue.push_back({ frameCounter, buffer, memory });
|
||||
}
|
||||
|
||||
void Device::ReclaimDeletions() {
|
||||
// Free every entry whose retire frame has been reached, compacting the
|
||||
// survivors down in place. An entry tagged at frame F retires at
|
||||
// F + framesInFlight: by the time the CPU has begun that many later
|
||||
// frames (each gated by a per-image fence wait), single-queue submission
|
||||
// order guarantees all GPU work from frame F is complete.
|
||||
std::size_t kept = 0;
|
||||
for (PendingDeletion& entry : deletionQueue) {
|
||||
if (entry.retireAfter + framesInFlight <= frameCounter) {
|
||||
vkDestroyBuffer(device, entry.buffer, nullptr);
|
||||
vkFreeMemory(device, entry.memory, nullptr);
|
||||
} else {
|
||||
deletionQueue[kept++] = entry;
|
||||
}
|
||||
}
|
||||
deletionQueue.resize(kept);
|
||||
}
|
||||
|
||||
void Device::DrainDeletions() {
|
||||
// Unconditional: the caller has just wait-idled, so no in-flight GPU work
|
||||
// can still reference any queued resource.
|
||||
for (PendingDeletion& entry : deletionQueue) {
|
||||
vkDestroyBuffer(device, entry.buffer, nullptr);
|
||||
vkFreeMemory(device, entry.memory, nullptr);
|
||||
}
|
||||
deletionQueue.clear();
|
||||
}
|
||||
|
|
@ -452,6 +452,11 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height
|
|||
Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &imageAcquiredSemaphores[i]));
|
||||
}
|
||||
|
||||
// The deferred-deletion queue (issue #101) retires a resource after this
|
||||
// many recorded frames — exactly the depth the per-frame fences (#40)
|
||||
// allow in flight. Set it here, where numFrames is in scope.
|
||||
Device::framesInFlight = numFrames;
|
||||
|
||||
// Per-frame info structs: everything that never varies between frames is
|
||||
// set here once. Render() patches the barriers' image/oldLayout and the
|
||||
// sync handles that follow currentBuffer/frameCounter (submitInfo's
|
||||
|
|
@ -542,6 +547,9 @@ void Window::Resize(std::uint32_t newWidth, std::uint32_t newHeight) {
|
|||
// Caller (configure handler / WM_SIZE) runs between frames, but be
|
||||
// defensive: ensure no in-flight commands reference the old swapchain.
|
||||
Device::CheckVkResult(vkQueueWaitIdle(Device::queue));
|
||||
// The queue is idle, so every deferred resource is now safe to free
|
||||
// regardless of its retire frame (issue #101).
|
||||
Device::DrainDeletions();
|
||||
|
||||
#ifdef CRAFTER_GRAPHICS_WINDOW_WAYLAND
|
||||
if (wpViewport) {
|
||||
|
|
@ -768,6 +776,7 @@ void Window::Render() {
|
|||
imageAcquired, (VkFence)nullptr, ¤tBuffer);
|
||||
if (acquire == VK_ERROR_OUT_OF_DATE_KHR) {
|
||||
Device::CheckVkResult(vkQueueWaitIdle(Device::queue));
|
||||
Device::DrainDeletions();
|
||||
RecreateSwapchainAndImages();
|
||||
onResize.Invoke();
|
||||
acquire = vkAcquireNextImageKHR(Device::device, swapChain, UINT64_MAX,
|
||||
|
|
@ -786,6 +795,12 @@ void Window::Render() {
|
|||
Device::CheckVkResult(vkWaitForFences(Device::device, 1, &waitFences[currentBuffer], VK_TRUE, UINT64_MAX));
|
||||
Device::CheckVkResult(vkResetFences(Device::device, 1, &waitFences[currentBuffer]));
|
||||
|
||||
// Now that a frame's fence has been waited, free any deferred resources
|
||||
// whose retire frame has been reached (issue #101). frameCounter is bumped
|
||||
// at end of frame, so it still holds this frame's index here — exactly the
|
||||
// value entries enqueued this frame are tagged with.
|
||||
Device::ReclaimDeletions();
|
||||
|
||||
Device::CheckVkResult(vkBeginCommandBuffer(drawCmdBuffers[currentBuffer], &cmdBufInfo));
|
||||
|
||||
// On an image's first use after (re)creating the swapchain it is still in
|
||||
|
|
@ -902,6 +917,7 @@ void Window::Render() {
|
|||
// frame. The wait-idle here also re-settles every per-frame fence to a
|
||||
// signaled state, so the next Render()'s fence wait passes through.
|
||||
Device::CheckVkResult(vkQueueWaitIdle(Device::queue));
|
||||
Device::DrainDeletions();
|
||||
RecreateSwapchainAndImages();
|
||||
onResize.Invoke();
|
||||
} else {
|
||||
|
|
@ -911,6 +927,10 @@ void Window::Render() {
|
|||
// command-buffer reuse, so the CPU is free to acquire/record frame N+1
|
||||
// while the GPU is still executing frame N.
|
||||
frameCounter++;
|
||||
// Advance the global deferred-deletion clock too (issue #101). Kept
|
||||
// separate from the per-window frameCounter above (which also keys the
|
||||
// acquire-semaphore slot) so non-window code can enqueue against it.
|
||||
Device::frameCounter++;
|
||||
}
|
||||
|
||||
#ifdef CRAFTER_TIMING
|
||||
|
|
|
|||
|
|
@ -294,6 +294,44 @@ export namespace Crafter {
|
|||
inline static VkDeviceSize directWriteBudget = 0;
|
||||
static void CacheUploadStrategy();
|
||||
|
||||
// ─── Fence-keyed deferred resource deletion (issue #101) ────────
|
||||
// Since #40 dropped the per-frame vkQueueWaitIdle, frames are
|
||||
// pipelined: a buffer the CPU is "done" with may still be read by the
|
||||
// GPU for up to framesInFlight-1 more frames. Destroying it
|
||||
// immediately (as Clear() does) is then a use-after-free. Callers that
|
||||
// free a buffer which may still be in flight — VulkanBuffer::Resize's
|
||||
// reallocate path, VulkanBuffer::DeferredClear — hand the handles here
|
||||
// instead, tagged with the current frameCounter; ReclaimDeletions frees
|
||||
// an entry only once framesInFlight frames have elapsed, by which point
|
||||
// single-queue submission order guarantees all GPU work that could
|
||||
// reference it has completed.
|
||||
struct PendingDeletion {
|
||||
std::uint64_t retireAfter; // frameCounter value at enqueue time
|
||||
VkBuffer buffer;
|
||||
VkDeviceMemory memory;
|
||||
};
|
||||
// Monotonic recorded-frame index, bumped once per Window::Render. Not
|
||||
// the same as Window::frameCounter (which is per-window and also drives
|
||||
// the acquire-semaphore slot) — this one is global so Mesh/Device code
|
||||
// can enqueue too, not just the window loop.
|
||||
inline static std::uint64_t frameCounter = 0;
|
||||
// = Window::numFrames; set at window init. The number of frames that
|
||||
// must elapse after an enqueue before the resource is safe to free.
|
||||
// Zero until set, which only makes ReclaimDeletions more eager — and it
|
||||
// is never called before a Window (which sets this) starts rendering.
|
||||
inline static std::uint8_t framesInFlight = 0;
|
||||
inline static std::vector<PendingDeletion> deletionQueue;
|
||||
// Tag {buffer, memory} for deletion after framesInFlight more frames.
|
||||
// A VK_NULL_HANDLE buffer is ignored (nothing to free).
|
||||
static void EnqueueDeletion(VkBuffer buffer, VkDeviceMemory memory);
|
||||
// Free every entry whose retire frame has been reached
|
||||
// (retireAfter + framesInFlight <= frameCounter). Call once per frame
|
||||
// after waiting that frame's fence.
|
||||
static void ReclaimDeletions();
|
||||
// Free every queued entry unconditionally. Call only after a wait-idle
|
||||
// (resize / teardown), when no in-flight GPU work can reference them.
|
||||
static void DrainDeletions();
|
||||
|
||||
// ─── Wayland key repeat ────────────────────────────────────────
|
||||
// TickKeyRepeats fires onRawKeyDown / onRawKeyHold / onTextInput on
|
||||
// the focused window for whichever key is currently repeating.
|
||||
|
|
|
|||
|
|
@ -155,6 +155,18 @@ namespace Crafter {
|
|||
buffer = VK_NULL_HANDLE;
|
||||
}
|
||||
|
||||
// Like Clear(), but hands the destroy+free to Device's fence-keyed
|
||||
// deletion queue instead of doing it immediately. Use when the buffer
|
||||
// may still be read by an in-flight frame's GPU work — destroying it
|
||||
// now would be a use-after-free, since frames are pipelined up to
|
||||
// framesInFlight deep (issue #101). The handle is nulled immediately so
|
||||
// this VulkanBuffer no longer owns it; vkFreeMemory (deferred) implicitly
|
||||
// unmaps mapped memory, so no explicit vkUnmapMemory is needed here.
|
||||
void DeferredClear() {
|
||||
Device::EnqueueDeletion(buffer, memory);
|
||||
buffer = VK_NULL_HANDLE;
|
||||
}
|
||||
|
||||
void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) {
|
||||
// Reuse the existing allocation in place when the request still fits
|
||||
// and the fixed-at-create properties match: usage flags are
|
||||
|
|
@ -172,8 +184,12 @@ namespace Crafter {
|
|||
size = requestedSize;
|
||||
return;
|
||||
}
|
||||
// Defer the old allocation's destruction: an in-flight frame may
|
||||
// still reference it (the #63 hazard, no longer masked by a
|
||||
// per-frame wait-idle since #40). DeferredClear nulls the handle,
|
||||
// so the Create below starts from a clean slate.
|
||||
if(buffer != VK_NULL_HANDLE) {
|
||||
Clear();
|
||||
DeferredClear();
|
||||
}
|
||||
Create(usageFlags, memoryPropertyFlags, count, preferredPropertyFlags);
|
||||
}
|
||||
|
|
|
|||
31
project.cpp
31
project.cpp
|
|
@ -586,6 +586,37 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> a
|
|||
rc.GetInterfacesAndImplementations(ifaces, resizeImpls);
|
||||
cfg.tests.push_back(std::move(resizeTest));
|
||||
|
||||
// Issue #101: fence-keyed deferred resource-deletion queue. Since #40
|
||||
// dropped the per-frame wait-idle, destroying a buffer the GPU may
|
||||
// still read (Resize's reallocate path) is a use-after-free.
|
||||
// VulkanBuffer::DeferredClear / Resize now hand handles to Device's
|
||||
// queue, which ReclaimDeletions frees only after framesInFlight frames
|
||||
// and DrainDeletions frees on a wait-idle. The retire timing is driven
|
||||
// on a real headless device (real buffers so the frees execute and the
|
||||
// validation layer can object) by stepping Device::frameCounter — no
|
||||
// swapchain/window needed, so it shares the native build settings.
|
||||
Test deferredTest;
|
||||
Configuration& dc = deferredTest.config;
|
||||
dc.path = cfg.path;
|
||||
dc.name = "DeferredDeletion";
|
||||
dc.outputName = "DeferredDeletion";
|
||||
dc.type = ConfigurationType::Executable;
|
||||
dc.target = cfg.target;
|
||||
dc.march = cfg.march;
|
||||
dc.mtune = cfg.mtune;
|
||||
dc.debug = cfg.debug;
|
||||
dc.sysroot = cfg.sysroot;
|
||||
dc.dependencies = cfg.dependencies;
|
||||
dc.externalDependencies = cfg.externalDependencies;
|
||||
dc.compileFlags = cfg.compileFlags;
|
||||
dc.linkFlags = cfg.linkFlags;
|
||||
dc.defines = cfg.defines;
|
||||
dc.cFiles = cfg.cFiles;
|
||||
std::vector<fs::path> deferredImpls(impls.begin(), impls.end());
|
||||
deferredImpls.emplace_back("tests/DeferredDeletion/main");
|
||||
dc.GetInterfacesAndImplementations(ifaces, deferredImpls);
|
||||
cfg.tests.push_back(std::move(deferredTest));
|
||||
|
||||
// Issue #89: Device::PreferDirectDeviceWrite chooses the upload strategy
|
||||
// for a CPU-written, GPU-read buffer — direct HOST_VISIBLE|DEVICE_LOCAL
|
||||
// map+write on ReBAR/UMA vs. staged-into-pure-DEVICE_LOCAL on a small
|
||||
|
|
|
|||
171
tests/DeferredDeletion/main.cpp
Normal file
171
tests/DeferredDeletion/main.cpp
Normal file
|
|
@ -0,0 +1,171 @@
|
|||
/*
|
||||
Crafter®.Graphics
|
||||
Copyright (C) 2026 Catcrafts®
|
||||
catcrafts.net
|
||||
|
||||
This library is free software; you can redistribute it and/or
|
||||
modify it under the terms of the GNU Lesser General Public
|
||||
License version 3.0 as published by the Free Software Foundation;
|
||||
|
||||
This library is distributed in the hope that it will be useful,
|
||||
but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
||||
Lesser General Public License for more details.
|
||||
|
||||
You should have received a copy of the GNU Lesser General Public
|
||||
License along with this library; if not, write to the Free Software
|
||||
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
|
||||
*/
|
||||
|
||||
// Issue #101: fence-keyed deferred resource-deletion queue. Since #40 dropped
|
||||
// the per-frame vkQueueWaitIdle, a buffer the CPU is done with may still be
|
||||
// read by the GPU for up to framesInFlight-1 more frames — so destroying it
|
||||
// immediately (VulkanBuffer::Clear / the old Resize path) is a use-after-free.
|
||||
// VulkanBuffer::DeferredClear() / Resize() now hand the handles to Device's
|
||||
// deletion queue, tagged with the current frameCounter; ReclaimDeletions frees
|
||||
// an entry only once framesInFlight frames have elapsed, and DrainDeletions
|
||||
// (called after a wait-idle) frees everything unconditionally.
|
||||
//
|
||||
// The retire-timing is the load-bearing logic: free too early and it's the
|
||||
// very UAF the queue exists to prevent; free too late and resources leak. This
|
||||
// test drives it on a real headless Vulkan device (real VkBuffer/VkDeviceMemory
|
||||
// so the frees actually execute and the validation layer can object), manually
|
||||
// stepping Device::frameCounter to pin down the exact frame an entry is
|
||||
// reclaimed on. It needs no swapchain/window — the queue itself is window-free.
|
||||
|
||||
#include "vulkan/vulkan.h"
|
||||
#include <cstdlib>
|
||||
|
||||
import Crafter.Graphics;
|
||||
import std;
|
||||
|
||||
using namespace Crafter;
|
||||
|
||||
namespace {
|
||||
|
||||
int failures = 0;
|
||||
|
||||
void Check(bool ok, std::string_view what) {
|
||||
std::println("{} {}", ok ? "PASS" : "FAIL", what);
|
||||
if (!ok) ++failures;
|
||||
}
|
||||
|
||||
constexpr VkBufferUsageFlags2 USAGE =
|
||||
VK_BUFFER_USAGE_2_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_2_SHADER_DEVICE_ADDRESS_BIT;
|
||||
constexpr auto DEVICE_LOCAL = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT;
|
||||
|
||||
} // namespace
|
||||
|
||||
int main() {
|
||||
Device::Initialize();
|
||||
|
||||
// No Window here, so set the in-flight depth ourselves (Window::Render
|
||||
// normally does this at init). Use the real window depth so the timing
|
||||
// matches production.
|
||||
Device::framesInFlight = Window::numFrames;
|
||||
Device::frameCounter = 0;
|
||||
Device::deletionQueue.clear();
|
||||
|
||||
// ── DeferredClear enqueues, nulls the handle, and retires after exactly
|
||||
// framesInFlight frames ──────────────────────────────────────────────
|
||||
{
|
||||
VulkanBuffer<float, false> buf;
|
||||
buf.Create(USAGE, DEVICE_LOCAL, 16);
|
||||
Check(buf.buffer != VK_NULL_HANDLE, "Create produced a real buffer handle");
|
||||
|
||||
buf.DeferredClear();
|
||||
Check(buf.buffer == VK_NULL_HANDLE,
|
||||
"DeferredClear nulls the handle so the buffer no longer owns it");
|
||||
Check(Device::deletionQueue.size() == 1,
|
||||
"DeferredClear enqueues exactly one pending deletion");
|
||||
|
||||
// Walk right up to the retire frame: still queued at every frame before
|
||||
// frameCounter reaches retireAfter + framesInFlight.
|
||||
for (std::uint64_t f = 0; f < Window::numFrames; ++f) {
|
||||
Device::frameCounter = f;
|
||||
Device::ReclaimDeletions();
|
||||
Check(Device::deletionQueue.size() == 1,
|
||||
std::format("entry survives ReclaimDeletions at frame {} "
|
||||
"(< retire frame {})", f, Window::numFrames));
|
||||
}
|
||||
|
||||
// At retireAfter(0) + framesInFlight it is finally freed.
|
||||
Device::frameCounter = Window::numFrames;
|
||||
Device::ReclaimDeletions();
|
||||
Check(Device::deletionQueue.empty(),
|
||||
std::format("entry is reclaimed once frameCounter reaches {}",
|
||||
Window::numFrames));
|
||||
}
|
||||
|
||||
// ── Resize's reallocate path defers the OLD allocation, not the new one ──
|
||||
{
|
||||
Device::frameCounter = 100;
|
||||
Device::deletionQueue.clear();
|
||||
|
||||
VulkanBuffer<float, false> buf;
|
||||
buf.Create(USAGE, DEVICE_LOCAL, 4);
|
||||
VkBuffer original = buf.buffer;
|
||||
|
||||
// Grow past capacity → forces destroy+recreate, which must DEFER the old
|
||||
// buffer (the #63 UAF this whole queue exists to close).
|
||||
buf.Resize(USAGE, DEVICE_LOCAL, 64);
|
||||
Check(buf.buffer != VK_NULL_HANDLE && buf.buffer != original,
|
||||
"Resize past capacity allocates a fresh buffer");
|
||||
Check(Device::deletionQueue.size() == 1
|
||||
&& Device::deletionQueue.front().buffer == original,
|
||||
"Resize defers the OLD allocation to the deletion queue");
|
||||
Check(Device::deletionQueue.front().retireAfter == 100,
|
||||
"deferred entry is tagged with the frameCounter at enqueue time");
|
||||
|
||||
// Not yet due (tagged at 100, retires at 100 + framesInFlight).
|
||||
Device::frameCounter = 100 + Window::numFrames - 1;
|
||||
Device::ReclaimDeletions();
|
||||
Check(Device::deletionQueue.size() == 1,
|
||||
"old allocation is not freed before its retire frame");
|
||||
|
||||
Device::frameCounter = 100 + Window::numFrames;
|
||||
Device::ReclaimDeletions();
|
||||
Check(Device::deletionQueue.empty(),
|
||||
"old allocation is freed on its retire frame");
|
||||
|
||||
buf.Clear(); // immediate cleanup of the live buffer (no GPU work pending)
|
||||
}
|
||||
|
||||
// ── DrainDeletions frees everything regardless of retire frame ───────────
|
||||
{
|
||||
Device::frameCounter = 0;
|
||||
Device::deletionQueue.clear();
|
||||
|
||||
VulkanBuffer<float, false> a;
|
||||
VulkanBuffer<float, false> b;
|
||||
a.Create(USAGE, DEVICE_LOCAL, 8);
|
||||
b.Create(USAGE, DEVICE_LOCAL, 8);
|
||||
a.DeferredClear();
|
||||
b.DeferredClear();
|
||||
Check(Device::deletionQueue.size() == 2, "two entries queued");
|
||||
|
||||
// Far from any retire frame — ReclaimDeletions would free nothing here.
|
||||
Device::DrainDeletions();
|
||||
Check(Device::deletionQueue.empty(),
|
||||
"DrainDeletions frees every queued entry unconditionally");
|
||||
}
|
||||
|
||||
// ── EnqueueDeletion ignores a null handle (nothing to free) ──────────────
|
||||
{
|
||||
Device::deletionQueue.clear();
|
||||
Device::EnqueueDeletion(VK_NULL_HANDLE, VK_NULL_HANDLE);
|
||||
Check(Device::deletionQueue.empty(),
|
||||
"EnqueueDeletion drops a VK_NULL_HANDLE buffer");
|
||||
}
|
||||
|
||||
Check(Device::validationErrorCount == 0,
|
||||
std::format("no Vulkan validation errors across the run ({} seen)",
|
||||
Device::validationErrorCount));
|
||||
|
||||
if (failures != 0) {
|
||||
std::println("{} check(s) failed", failures);
|
||||
return EXIT_FAILURE;
|
||||
}
|
||||
std::println("all checks passed");
|
||||
return EXIT_SUCCESS;
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue