Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v0.19] Fix Buffer Mapping Deadlock #5517

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Apr 11, 2024

Found this while using wgpu-profiler.

Subtle temporary extension caused a deadlock in buffer mapping code.

@cwfitzgerald cwfitzgerald requested a review from a team as a code owner April 11, 2024 00:26
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh gawd. this is insanely evil
you're missing a changelog note though, imho worth pointing out that there's a fix (we should give people reasons to update ;-))

@Wumpf
Copy link
Member

Wumpf commented Apr 17, 2024

changelog got promised to be done upon the release processing :)

@Wumpf Wumpf merged commit 2516bc2 into gfx-rs:v0.19 Apr 17, 2024
22 of 25 checks passed
// This _cannot_ be inlined into the match. If it is, the lock will be held
// open through the whole match, resulting in a deadlock when we try to re-lock
// the buffer back to active.
let mapping = std::mem::replace(
&mut *buffer.map_state.lock(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ReentrantMutex used to prevent double locking causing deadlock?

A mutex which can be recursively locked by a single thread.
This type is identical to Mutex except for the following points:

- Locking multiple times from the same thread will work correctly instead of deadlocking.
- ReentrantMutexGuard does not give mutable references to the locked data. Use a RefCell if you need this.

Copy link
Member Author

@cwfitzgerald cwfitzgerald Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - there is a cost to reentrant mutexes and none of our mutexes require reentrency.

We have a PR open which adds a debug check to make sure we don't lock locks in a way that could cause a deadlock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwfitzgerald cwfitzgerald deleted the fix-buffer-mapping-deadlock branch April 20, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants