reland: 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.
The first time this CL landed introduced a lock-order-inversion when the
DescriptorSetAllocator lock and mDescriptorAllocatorsPendingDeallocation
locks were held at the time but acquired in different orders. Release
DescriptorSetAllocator lock before calling into
EnqueueDeferredDeallocation() to avoid this.
Bug: 372651189
Change-Id: Ie8df192528776e538eb7f4b1c5d28dd8e3553778
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/216114
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Kyle Charbonneau <kylechar@google.com>
Reviewed-by: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/native/vulkan/DescriptorSetAllocator.cpp b/src/dawn/native/vulkan/DescriptorSetAllocator.cpp
index 123718d..6b70b4f 100644
--- a/src/dawn/native/vulkan/DescriptorSetAllocator.cpp
+++ b/src/dawn/native/vulkan/DescriptorSetAllocator.cpp
@@ -118,25 +118,36 @@
}
void DescriptorSetAllocator::Deallocate(DescriptorSetAllocation* allocationInfo) {
- Mutex::AutoLock lock(&mMutex);
-
- DAWN_ASSERT(allocationInfo != nullptr);
- DAWN_ASSERT(allocationInfo->set != VK_NULL_HANDLE);
-
- // We can't reuse the descriptor set right away because the Vulkan spec says in the
- // documentation for vkCmdBindDescriptorSets that the set may be consumed any time between
- // host execution of the command and the end of the draw/dispatch.
+ bool enqueueDeferredDeallocation = false;
Device* device = ToBackend(GetDevice());
- const ExecutionSerial serial = device->GetQueue()->GetPendingCommandSerial();
- mPendingDeallocations.Enqueue({allocationInfo->poolIndex, allocationInfo->setIndex}, serial);
- if (mLastDeallocationSerial != serial) {
- device->EnqueueDeferredDeallocation(this);
- mLastDeallocationSerial = serial;
+ {
+ Mutex::AutoLock lock(&mMutex);
+
+ DAWN_ASSERT(allocationInfo != nullptr);
+ DAWN_ASSERT(allocationInfo->set != VK_NULL_HANDLE);
+
+ // We can't reuse the descriptor set right away because the Vulkan spec says in the
+ // documentation for vkCmdBindDescriptorSets that the set may be consumed any time between
+ // host execution of the command and the end of the draw/dispatch.
+ const ExecutionSerial serial = device->GetQueue()->GetPendingCommandSerial();
+ mPendingDeallocations.Enqueue({allocationInfo->poolIndex, allocationInfo->setIndex},
+ serial);
+
+ if (mLastDeallocationSerial != serial) {
+ enqueueDeferredDeallocation = true;
+ mLastDeallocationSerial = serial;
+ }
+
+ // Clear the content of allocation so that use after frees are more visible.
+ *allocationInfo = {};
}
- // Clear the content of allocation so that use after frees are more visible.
- *allocationInfo = {};
+ if (enqueueDeferredDeallocation) {
+ // Release lock before calling EnqueueDeferredDeallocation() to avoid lock acquisition
+ // order inversion with lock used there.
+ device->EnqueueDeferredDeallocation(this);
+ }
}
void DescriptorSetAllocator::FinishDeallocation(ExecutionSerial completedSerial) {
diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp
index 22c080c..7d0e740 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -332,11 +332,12 @@
ExecutionSerial completedSerial = queue->GetCompletedCommandSerial();
queue->RecycleCompletedCommands(completedSerial);
- for (Ref<DescriptorSetAllocator>& allocator :
- mDescriptorAllocatorsPendingDeallocation.IterateUpTo(completedSerial)) {
- allocator->FinishDeallocation(completedSerial);
- }
- mDescriptorAllocatorsPendingDeallocation.ClearUpTo(completedSerial);
+ mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
+ for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(completedSerial)) {
+ allocator->FinishDeallocation(completedSerial);
+ }
+ pending->ClearUpTo(completedSerial);
+ });
GetResourceMemoryAllocator()->Tick(completedSerial);
GetFencedDeleter()->Tick(completedSerial);
@@ -383,8 +384,8 @@
}
void Device::EnqueueDeferredDeallocation(DescriptorSetAllocator* allocator) {
- mDescriptorAllocatorsPendingDeallocation.Enqueue(allocator,
- GetQueue()->GetPendingCommandSerial());
+ mDescriptorAllocatorsPendingDeallocation->Enqueue(allocator,
+ GetQueue()->GetPendingCommandSerial());
}
ResultOrError<VulkanDeviceKnobs> Device::CreateDevice(VkPhysicalDevice vkPhysicalDevice) {
@@ -898,11 +899,12 @@
ToBackend(GetPhysicalDevice())->GetVulkanInstance()->StopListeningForDeviceMessages(this);
- for (Ref<DescriptorSetAllocator>& allocator :
- mDescriptorAllocatorsPendingDeallocation.IterateUpTo(kMaxExecutionSerial)) {
- allocator->FinishDeallocation(kMaxExecutionSerial);
- }
- mDescriptorAllocatorsPendingDeallocation.ClearUpTo(kMaxExecutionSerial);
+ mDescriptorAllocatorsPendingDeallocation.Use([&](auto pending) {
+ for (Ref<DescriptorSetAllocator>& allocator : pending->IterateUpTo(kMaxExecutionSerial)) {
+ allocator->FinishDeallocation(kMaxExecutionSerial);
+ }
+ pending->ClearUpTo(kMaxExecutionSerial);
+ });
// Releasing the uploader enqueues buffers to be released.
// Call Tick() again to clear them before releasing the deleter.
diff --git a/src/dawn/native/vulkan/DeviceVk.h b/src/dawn/native/vulkan/DeviceVk.h
index 75df610..355a704 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 appended without holding the device mutex.
+ MutexProtected<SerialQueue<ExecutionSerial, Ref<DescriptorSetAllocator>>>
mDescriptorAllocatorsPendingDeallocation;
std::unique_ptr<MutexProtected<FencedDeleter>> mDeleter;
std::unique_ptr<MutexProtected<ResourceMemoryAllocator>> mResourceMemoryAllocator;
