commit 43b18d23f1be9ea4e01da9f164d1dbbb510978df
author Kyle Charbonneau <kylechar@google.com> Sat Nov 16 00:17:54 2024 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Sat Nov 16 00:17:54 2024 +0000
tree 2754022b7d0de7d728af3832ba5af146a858c540
parent 43e7da53d189d2da034ac6ffcdb391eb8633e72f

Revert "Use mutex with DeviceVk deallocation queue"

This reverts commit ae9acbdf215860b1474735cd00abffc2ecd79f8f.

Reason for revert: Causes mutex order inversion

Original change's description:
> 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>

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 372651189
Change-Id: I6f8a187c7dd07d8e91d93b23be9304b97dbdf465
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/215254
Commit-Queue: Kyle Charbonneau <kylechar@google.com>
Auto-Submit: Kyle Charbonneau <kylechar@google.com>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Loko Kung <lokokung@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 6fc69c7..2e6d2dc 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -332,15 +332,14 @@
     ExecutionSerial completedSerial = queue->GetCompletedCommandSerial();
     queue->RecycleCompletedCommands(completedSerial);
 
-    mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
-        for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(completedSerial)) {
-            allocator->FinishDeallocation(completedSerial);
-        }
-    });
+    for (Ref<DescriptorSetAllocator>& allocator :
+         mDescriptorAllocatorsPendingDeallocation.IterateUpTo(completedSerial)) {
+        allocator->FinishDeallocation(completedSerial);
+    }
 
     GetResourceMemoryAllocator()->Tick(completedSerial);
     GetFencedDeleter()->Tick(completedSerial);
-    mDescriptorAllocatorsPendingDeallocation->ClearUpTo(completedSerial);
+    mDescriptorAllocatorsPendingDeallocation.ClearUpTo(completedSerial);
 
     DAWN_TRY(queue->SubmitPendingCommands());
     DAWN_TRY(CheckDebugLayerAndGenerateErrors());
@@ -384,8 +383,8 @@
 }
 
 void Device::EnqueueDeferredDeallocation(DescriptorSetAllocator* allocator) {
-    mDescriptorAllocatorsPendingDeallocation->Enqueue(allocator,
-                                                      GetQueue()->GetPendingCommandSerial());
+    mDescriptorAllocatorsPendingDeallocation.Enqueue(allocator,
+                                                     GetQueue()->GetPendingCommandSerial());
 }
 
 ResultOrError<VulkanDeviceKnobs> Device::CreateDevice(VkPhysicalDevice vkPhysicalDevice) {
@@ -909,16 +908,15 @@
 
     ToBackend(GetPhysicalDevice())->GetVulkanInstance()->StopListeningForDeviceMessages(this);
 
-    mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
-        for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(kMaxExecutionSerial)) {
-            allocator->FinishDeallocation(kMaxExecutionSerial);
-        }
-    });
+    for (Ref<DescriptorSetAllocator>& allocator :
+         mDescriptorAllocatorsPendingDeallocation.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 aa713c7..75df610 100644
--- a/src/dawn/native/vulkan/DeviceVk.h
+++ b/src/dawn/native/vulkan/DeviceVk.h
@@ -187,8 +187,7 @@
     VkDevice mVkDevice = VK_NULL_HANDLE;
     uint32_t mMainQueueFamily = 0;
 
-    // Entries can be append without holding the device mutex.
-    MutexProtected<SerialQueue<ExecutionSerial, Ref<DescriptorSetAllocator>>>
+    SerialQueue<ExecutionSerial, Ref<DescriptorSetAllocator>>
         mDescriptorAllocatorsPendingDeallocation;
     std::unique_ptr<MutexProtected<FencedDeleter>> mDeleter;
     std::unique_ptr<MutexProtected<ResourceMemoryAllocator>> mResourceMemoryAllocator;