commit ae9acbdf215860b1474735cd00abffc2ecd79f8f
author Kyle Charbonneau <kylechar@google.com> Thu Nov 14 17:46:34 2024 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Nov 14 17:46:34 2024 +0000
tree 2dde76ef508915807b44f0e3b44e563d766c74f4
parent 4e3843e3d51cad247ace0eabf3a35aab90b91fbe

Use mutex with DeviceVk deallocation queue

DeviceVk::mDescriptorAllocatorsPendingDeallocation can be accessed from
an arbitrary thread without holding the device mutex. BindGroup
destruction ends up appending values to the queue in
Device::EnqueueDeferredDeallocation(). Add a mutex to protect
mDescriptorAllocatorsPendingDeallocation against data races.

Adding MutexProtected wouldn't work with the existing iteration through
the queue. This is because mutex would only be held for the duration of
IterateUpTo() function call which builds BeginEnd. BeginEnd holds
iterators into the container and if the container is modified while
BeginEnd exists those iterators may be invalidated. Instead, add
SerialStorage::TakeUpTo() which makes a new container that contains all
the elements up to specific serial and removes them from the original
container.

Bug: 372651189
Change-Id: I125a575e8b2ee632a681685ede744ae57f790bfa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/214734
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Kyle Charbonneau <kylechar@google.com>

src/dawn/native/vulkan/DeviceVk.cpp

diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp
index f7b74d3..9abe3f7 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -335,14 +335,15 @@
     ExecutionSerial completedSerial = queue->GetCompletedCommandSerial();
     queue->RecycleCompletedCommands(completedSerial);
 
-    for (Ref<DescriptorSetAllocator>& allocator :
-         mDescriptorAllocatorsPendingDeallocation.IterateUpTo(completedSerial)) {
-        allocator->FinishDeallocation(completedSerial);
-    }
+    mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
+        for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(completedSerial)) {
+            allocator->FinishDeallocation(completedSerial);
+        }
+    });
 
     GetResourceMemoryAllocator()->Tick(completedSerial);
     GetFencedDeleter()->Tick(completedSerial);
-    mDescriptorAllocatorsPendingDeallocation.ClearUpTo(completedSerial);
+    mDescriptorAllocatorsPendingDeallocation->ClearUpTo(completedSerial);
 
     DAWN_TRY(queue->SubmitPendingCommands());
     DAWN_TRY(CheckDebugLayerAndGenerateErrors());
@@ -386,8 +387,8 @@
 }
 
 void Device::EnqueueDeferredDeallocation(DescriptorSetAllocator* allocator) {
-    mDescriptorAllocatorsPendingDeallocation.Enqueue(allocator,
-                                                     GetQueue()->GetPendingCommandSerial());
+    mDescriptorAllocatorsPendingDeallocation->Enqueue(allocator,
+                                                      GetQueue()->GetPendingCommandSerial());
 }
 
 ResultOrError<VulkanDeviceKnobs> Device::CreateDevice(VkPhysicalDevice vkPhysicalDevice) {
@@ -911,15 +912,16 @@
 
     ToBackend(GetPhysicalDevice())->GetVulkanInstance()->StopListeningForDeviceMessages(this);
 
-    for (Ref<DescriptorSetAllocator>& allocator :
-         mDescriptorAllocatorsPendingDeallocation.IterateUpTo(kMaxExecutionSerial)) {
-        allocator->FinishDeallocation(kMaxExecutionSerial);
-    }
+    mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
+        for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(kMaxExecutionSerial)) {
+            allocator->FinishDeallocation(kMaxExecutionSerial);
+        }
+    });
 
     // Releasing the uploader enqueues buffers to be released.
     // Call Tick() again to clear them before releasing the deleter.
     GetResourceMemoryAllocator()->Tick(kMaxExecutionSerial);
-    mDescriptorAllocatorsPendingDeallocation.ClearUpTo(kMaxExecutionSerial);
+    mDescriptorAllocatorsPendingDeallocation->ClearUpTo(kMaxExecutionSerial);
 
     // Allow recycled memory to be deleted.
     GetResourceMemoryAllocator()->DestroyPool();

src/dawn/native/vulkan/DeviceVk.h

diff --git a/src/dawn/native/vulkan/DeviceVk.h b/src/dawn/native/vulkan/DeviceVk.h
index 75df610..aa713c7 100644
--- a/src/dawn/native/vulkan/DeviceVk.h
+++ b/src/dawn/native/vulkan/DeviceVk.h
@@ -187,7 +187,8 @@
     VkDevice mVkDevice = VK_NULL_HANDLE;
     uint32_t mMainQueueFamily = 0;
 
-    SerialQueue<ExecutionSerial, Ref<DescriptorSetAllocator>>
+    // Entries can be append without holding the device mutex.
+    MutexProtected<SerialQueue<ExecutionSerial, Ref<DescriptorSetAllocator>>>
         mDescriptorAllocatorsPendingDeallocation;
     std::unique_ptr<MutexProtected<FencedDeleter>> mDeleter;
     std::unique_ptr<MutexProtected<ResourceMemoryAllocator>> mResourceMemoryAllocator;