Skip to content

Commit

Permalink
Revert "Use mutex with DeviceVk deallocation queue"
Browse files Browse the repository at this point in the history
This reverts commit ae9acbd.

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 <[email protected]>
> Commit-Queue: Kyle Charbonneau <[email protected]>

# 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 <[email protected]>
Auto-Submit: Kyle Charbonneau <[email protected]>
Reviewed-by: Loko Kung <[email protected]>
Commit-Queue: Loko Kung <[email protected]>
  • Loading branch information
Kyle Charbonneau authored and Dawn LUCI CQ committed Nov 16, 2024
1 parent 43e7da5 commit 43b18d2
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 16 deletions.
26 changes: 12 additions & 14 deletions src/dawn/native/vulkan/DeviceVk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -332,15 +332,14 @@ MaybeError Device::TickImpl() {
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());
Expand Down Expand Up @@ -384,8 +383,8 @@ external_semaphore::Service* Device::GetExternalSemaphoreService() const {
}

void Device::EnqueueDeferredDeallocation(DescriptorSetAllocator* allocator) {
mDescriptorAllocatorsPendingDeallocation->Enqueue(allocator,
GetQueue()->GetPendingCommandSerial());
mDescriptorAllocatorsPendingDeallocation.Enqueue(allocator,
GetQueue()->GetPendingCommandSerial());
}

ResultOrError<VulkanDeviceKnobs> Device::CreateDevice(VkPhysicalDevice vkPhysicalDevice) {
Expand Down Expand Up @@ -909,16 +908,15 @@ void Device::DestroyImpl() {

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();
Expand Down
3 changes: 1 addition & 2 deletions src/dawn/native/vulkan/DeviceVk.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,7 @@ class Device final : public DeviceBase {
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;
Expand Down

0 comments on commit 43b18d2

Please sign in to comment.