[core] Enforce a deadlock-free locking order for Mutexes.

[jimblandy]

If debug_assertions or the "validate-locks" feature are enabled, change wgpu-core to use a wrapper around parking_lot::Mutex that checks for potential deadlocks.

[jimblandy]

This has already found a cycle, although it might take a lot of threads to hit it. If you uncomment out the indicated code in wgpu-core/src/lock/rank.rs, then you'll get this error mesage:

error[E0391]: cycle detected when const checking `lock::rank::COMMAND_BUFFER_DATA`
  --> wgpu-core/src/lock/rank.rs:28:35
   |
28 |                   followers: 0 $( | $follower.bit )*,
   |                                     ^^^^^^^^^
...
39 | / define_lock_ranks! {
40 | |     rank COMMAND_BUFFER_DATA 0 followed by {
41 | |         DEVICE_USAGE_SCOPES,
42 | |         SHARED_TRACKER_INDEX_ALLOCATOR_INNER,
...  |
83 | |     rank SURFACE_PRESENTATION 22 followed by { };
84 | | }
   | |_- in this macro invocation
   |
note: ...which requires const checking `lock::rank::BUFFER_MAP_STATE`...
  --> wgpu-core/src/lock/rank.rs:28:35
   |
28 |                   followers: 0 $( | $follower.bit )*,
   |                                     ^^^^^^^^^
...
39 | / define_lock_ranks! {
40 | |     rank COMMAND_BUFFER_DATA 0 followed by {
41 | |         DEVICE_USAGE_SCOPES,
42 | |         SHARED_TRACKER_INDEX_ALLOCATOR_INNER,
...  |
83 | |     rank SURFACE_PRESENTATION 22 followed by { };
84 | | }
   | |_- in this macro invocation
note: ...which requires const checking `lock::rank::DEVICE_PENDING_WRITES`...
  --> wgpu-core/src/lock/rank.rs:28:35
   |
28 |                   followers: 0 $( | $follower.bit )*,
   |                                     ^^^^^^^^^
...
39 | / define_lock_ranks! {
40 | |     rank COMMAND_BUFFER_DATA 0 followed by {
41 | |         DEVICE_USAGE_SCOPES,
42 | |         SHARED_TRACKER_INDEX_ALLOCATOR_INNER,
...  |
83 | |     rank SURFACE_PRESENTATION 22 followed by { };
84 | | }
   | |_- in this macro invocation
note: ...which requires const checking `lock::rank::DEVICE_LIFE_TRACKER`...
  --> wgpu-core/src/lock/rank.rs:28:35
   |
28 |                   followers: 0 $( | $follower.bit )*,
   |                                     ^^^^^^^^^
...
39 | / define_lock_ranks! {
40 | |     rank COMMAND_BUFFER_DATA 0 followed by {
41 | |         DEVICE_USAGE_SCOPES,
42 | |         SHARED_TRACKER_INDEX_ALLOCATOR_INNER,
...  |
83 | |     rank SURFACE_PRESENTATION 22 followed by { };
84 | | }
   | |_- in this macro invocation
note: ...which requires const checking `lock::rank::DEVICE_TEMP_SUSPECTED`...
  --> wgpu-core/src/lock/rank.rs:28:35
   |
28 |                   followers: 0 $( | $follower.bit )*,
   |                                     ^^^^^^^^^
...
39 | / define_lock_ranks! {
40 | |     rank COMMAND_BUFFER_DATA 0 followed by {
41 | |         DEVICE_USAGE_SCOPES,
42 | |         SHARED_TRACKER_INDEX_ALLOCATOR_INNER,
...  |
83 | |     rank SURFACE_PRESENTATION 22 followed by { };
84 | | }
   | |_- in this macro invocation
   = note: ...which again requires const checking `lock::rank::COMMAND_BUFFER_DATA`, completing the cycle
   = note: cycle used when running analysis passes on this crate
   = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
   = note: this error originates in the macro `define_lock_ranks` (in Nightly builds, run with -Z macro-backtrace for more info)

[ErichDonGubler]

I'll take on review for this one. I've already done quite a bit of review with Jim as he's explained how the locking mechanism works. We'll both work to polish this up, and document and test properly. 🫡

[jimblandy]

One wrinkle: This has already detected a deadlock, and I haven't pushed very far yet, so I'd guess there'd be more to come. We'll have to fix them before we can actually land this.

[Edit: no, we can just make it only enabled when the feature is turned on, leaving both debug and release builds unaffected. This means we're not getting test coverage for the instrumented lock types, but at least we can have it in tree, rather than sitting on a branch as an indefinitely long list of deadlocks gets addressed.]

[ErichDonGubler]

Not gonna commit to a review outcome yet, since this isn't out of draft yet. :^)

[cwfitzgerald]

This is absolutely amazing stuff!

Have a docs nit, but I love the docs, they are very clear

[jimblandy]

@cwfitzgerald Thanks for the review! @ErichDonGubler and I agreed this should have unit tests, so I'll work on that tomorrow and then ask Erich to take another look.

[cwfitzgerald]

Sounds good, could @ErichDonGubler just mark it as request changes so I don't accidentally merge something I shouldn't :)

[ErichDonGubler]

Just waiting on feedback I've already left. 😊

[jimblandy]

@ErichDonGubler, I took most of your suggestions but you had one that I didn't want to take. I've explained and left it unresolved; let me know what you think.

[ErichDonGubler]

Only nitpick feedback left. Approving to enable progress, since I trust @jimblandy to duly consider the feedback I've left.

[jimblandy]

@ErichDonGubler Hmm. Plot twist.

1. Just today I remarked on Rust's wisdom in flagging the use of feature = "foo" in the code when there's no "foo" feature in Cargo.toml. This is completely untrue: rustc just treats them as if they're not enabled.

2. We want our CI to run tests with --all-features, which would enable "validate-locks", which causes a panic in ordinary operation.

I'm not sure what the best way forward is.

• The easiest is to omit the feature from Cargo.toml. If people want to experiment it, they'll have to add it back themselves.

• We could just comment it out, but that has the disadvantage that it's not clear what you need to uncomment to make things work. The nice thing about putting feature = "validate-locks", even without a Cargo.toml entry, is that its use marks everything that needs to be adjusted, and it's pretty obvious what to do if you want to try it out.

Other ideas??

[jimblandy]

In chat we decided to go with cfg(validate_locks).

Edit: Err, cfgs are compilation-wide, so actually it's cfg(wgpu_validate_locks).