github/wgpu
1
1
Fork 0
mirror of https://github.com/gfx-rs/wgpu.git synced 2026-04-22 03:02:01 -04:00
Code Issues Packages Projects Releases Wiki Activity

Prevent a deadlock when destroying resources (#5216)

Browse Source 
It's risky to get write access through the snatchlock from a drop implementation since the snatch lock is typically held for large scopes. This commit makes it so we deffer snatching some resources to when the device is polled and we know the snatch lock is not held.

Co-authored-by: Erich Gubler <erichdongubler@gmail.com>
This commit is contained in:
Nicolas Silva
2024-02-07 17:17:04 +01:00
committed by GitHub
parent 20fda69834
commit 499bd64c29
3 changed files with 91 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
wgpu-core/src/device/global.rs
View File

@@ -2107,6 +2107,12 @@ impl Global {
) -> Result<bool, WaitIdleError> {
api_log!("Device::poll");
let hub = A::hub(self);
let device = hub
.devices
.get(device_id)
.map_err(|_| DeviceError::Invalid)?;
let (closures, queue_empty) = {
if let wgt::Maintain::WaitForSubmissionIndex(submission_index) = maintain {
if submission_index.queue_id != device_id.transmute() {
@@ -2117,16 +2123,15 @@ impl Global {
}
}
let hub = A::hub(self);
let device = hub
.devices
.get(device_id)
.map_err(|_| DeviceError::Invalid)?;
let fence = device.fence.read();
let fence = fence.as_ref().unwrap();
device.maintain(fence, maintain)?
};
// Some deferred destroys are scheduled in maintain so run this right after
// to avoid holding on to them until the next device poll.
device.deferred_resource_destruction();
closures.fire();
Ok(queue_empty)

77
wgpu-core/src/device/resource.rs
View File

@@ -1,12 +1,13 @@
#[cfg(feature = "trace")]
use crate::device::trace;
use crate::{
binding_model::{self, BindGroupLayout, BindGroupLayoutEntryError},
binding_model::{self, BindGroup, BindGroupLayout, BindGroupLayoutEntryError},
command, conv,
device::life::{LifetimeTracker, WaitIdleError},
device::queue::PendingWrites,
device::{
bgl, AttachmentData, CommandAllocator, DeviceLostInvocation, MissingDownlevelFlags,
bgl,
life::{LifetimeTracker, WaitIdleError},
queue::PendingWrites,
AttachmentData, CommandAllocator, DeviceLostInvocation, MissingDownlevelFlags,
MissingFeatures, RenderPassContext, CLEANUP_WAIT_MS,
},
hal_api::HalApi,
@@ -21,10 +22,9 @@ use crate::{
pipeline,
pool::ResourcePool,
registry::Registry,
resource::ResourceInfo,
resource::{
self, Buffer, QuerySet, Resource, ResourceType, Sampler, Texture, TextureView,
TextureViewNotRenderableReason,
self, Buffer, QuerySet, Resource, ResourceInfo, ResourceType, Sampler, Texture,
TextureView, TextureViewNotRenderableReason,
},
resource_log,
snatch::{SnatchGuard, SnatchLock, Snatchable},
@@ -48,7 +48,7 @@ use std::{
num::NonZeroU32,
sync::{
atomic::{AtomicBool, AtomicU64, Ordering},
Arc,
Arc, Weak,
},
};
@@ -129,10 +129,16 @@ pub struct Device<A: HalApi> {
pub(crate) downlevel: wgt::DownlevelCapabilities,
pub(crate) instance_flags: wgt::InstanceFlags,
pub(crate) pending_writes: Mutex<Option<PendingWrites<A>>>,
pub(crate) deferred_destroy: Mutex<Vec<DeferredDestroy<A>>>,
#[cfg(feature = "trace")]
pub(crate) trace: Mutex<Option<trace::Trace>>,
}
pub(crate) enum DeferredDestroy<A: HalApi> {
TextureView(Weak<TextureView<A>>),
BindGroup(Weak<BindGroup<A>>),
}
impl<A: HalApi> std::fmt::Debug for Device<A> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Device")
@@ -285,6 +291,7 @@ impl<A: HalApi> Device<A> {
downlevel,
instance_flags,
pending_writes: Mutex::new(Some(pending_writes)),
deferred_destroy: Mutex::new(Vec::new()),
})
}
@@ -300,6 +307,56 @@ impl<A: HalApi> Device<A> {
self.life_tracker.lock()
}
/// Run some destroy operations that were deferred.
///
/// Destroying the resources requires taking a write lock on the device's snatch lock,
/// so a good reason for deferring resource destruction is when we don't know for sure
/// how risky it is to take the lock (typically, it shouldn't be taken from the drop
/// implementation of a reference-counted structure).
/// The snatch lock must not be held while this function is called.
pub(crate) fn deferred_resource_destruction(&self) {
while let Some(item) = self.deferred_destroy.lock().pop() {
match item {
DeferredDestroy::TextureView(view) => {
let Some(view) = view.upgrade() else {
continue;
};
let Some(raw_view) = view.raw.snatch(self.snatchable_lock.write()) else {
continue;
};
resource_log!("Destroy raw TextureView (destroyed) {:?}", view.label());
#[cfg(feature = "trace")]
if let Some(t) = self.trace.lock().as_mut() {
t.add(trace::Action::DestroyTextureView(view.info.id()));
}
unsafe {
use hal::Device;
self.raw().destroy_texture_view(raw_view);
}
}
DeferredDestroy::BindGroup(bind_group) => {
let Some(bind_group) = bind_group.upgrade() else {
continue;
};
let Some(raw_bind_group) = bind_group.raw.snatch(self.snatchable_lock.write()) else {
continue;
};
resource_log!("Destroy raw BindGroup (destroyed) {:?}", bind_group.label());
#[cfg(feature = "trace")]
if let Some(t) = self.trace.lock().as_mut() {
t.add(trace::Action::DestroyBindGroup(bind_group.info.id()));
}
unsafe {
use hal::Device;
self.raw().destroy_bind_group(raw_bind_group);
}
}
}
}
}
/// Check this device for completed commands.
///
/// The `maintain` argument tells how the maintence function should behave, either
@@ -1972,7 +2029,7 @@ impl<A: HalApi> Device<A> {
layout: &Arc<BindGroupLayout<A>>,
desc: &binding_model::BindGroupDescriptor,
hub: &Hub<A>,
) -> Result<binding_model::BindGroup<A>, binding_model::CreateBindGroupError> {
) -> Result<BindGroup<A>, binding_model::CreateBindGroupError> {
use crate::binding_model::{BindingResource as Br, CreateBindGroupError as Error};
{
// Check that the number of entries in the descriptor matches
@@ -2212,7 +2269,7 @@ impl<A: HalApi> Device<A> {
.map_err(DeviceError::from)?
};
Ok(binding_model::BindGroup {
Ok(BindGroup {
raw: Snatchable::new(raw),
device: self.clone(),
layout: layout.clone(),

56
wgpu-core/src/resource.rs
View File

@@ -3,8 +3,8 @@ use crate::device::trace;
use crate::{
binding_model::BindGroup,
device::{
queue, BufferMapPendingClosure, Device, DeviceError, HostMap, MissingDownlevelFlags,
MissingFeatures,
queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError, HostMap,
MissingDownlevelFlags, MissingFeatures,
},
global::Global,
hal_api::HalApi,
@@ -604,29 +604,6 @@ impl<A: HalApi> Resource for Buffer<A> {
}
}
fn snatch_and_destroy_bind_groups<A: HalApi>(
device: &Device<A>,
bind_groups: &[Weak<BindGroup<A>>],
) {
for bind_group in bind_groups {
if let Some(bind_group) = bind_group.upgrade() {
if let Some(raw_bind_group) = bind_group.raw.snatch(device.snatchable_lock.write()) {
resource_log!("Destroy raw BindGroup (destroyed) {:?}", bind_group.label());
#[cfg(feature = "trace")]
if let Some(t) = device.trace.lock().as_mut() {
t.add(trace::Action::DestroyBindGroup(bind_group.info.id()));
}
unsafe {
use hal::Device;
device.raw().destroy_bind_group(raw_bind_group);
}
}
}
}
}
/// A buffer that has been marked as destroyed and is staged for actual deletion soon.
#[derive(Debug)]
pub struct DestroyedBuffer<A: HalApi> {
@@ -650,7 +627,11 @@ impl<A: HalApi> DestroyedBuffer<A> {
impl<A: HalApi> Drop for DestroyedBuffer<A> {
fn drop(&mut self) {
snatch_and_destroy_bind_groups(&self.device, &self.bind_groups);
let mut deferred = self.device.deferred_destroy.lock();
for bind_group in self.bind_groups.drain(..) {
deferred.push(DeferredDestroy::BindGroup(bind_group));
}
drop(deferred);
if let Some(raw) = self.raw.take() {
resource_log!("Destroy raw Buffer (destroyed) {:?}", self.label());
@@ -1038,25 +1019,16 @@ impl<A: HalApi> DestroyedTexture<A> {
impl<A: HalApi> Drop for DestroyedTexture<A> {
fn drop(&mut self) {
let device = &self.device;
snatch_and_destroy_bind_groups(device, &self.bind_groups);
let mut deferred = device.deferred_destroy.lock();
for view in self.views.drain(..) {
if let Some(view) = view.upgrade() {
if let Some(raw_view) = view.raw.snatch(device.snatchable_lock.write()) {
resource_log!("Destroy raw TextureView (destroyed) {:?}", view.label());
#[cfg(feature = "trace")]
if let Some(t) = self.device.trace.lock().as_mut() {
t.add(trace::Action::DestroyTextureView(view.info.id()));
}
unsafe {
use hal::Device;
self.device.raw().destroy_texture_view(raw_view);
}
}
}
deferred.push(DeferredDestroy::TextureView(view));
}
for bind_group in self.bind_groups.drain(..) {
deferred.push(DeferredDestroy::BindGroup(bind_group));
}
drop(deferred);
if let Some(raw) = self.raw.take() {
resource_log!("Destroy raw Texture (destroyed) {:?}", self.label());