Lock rank cleanup, part 2

[andyleiserson]

Progress towards #5937. Part 1 was in #9524.

This fixes some violations of the lock rank graph:

• Fix lock order violation in release_gpu_resources
• Avoid overlapping locks in initialize_texture_memory
• Avoid overlapping locks in poll_all_devices_of_api
• Avoid overlapping locks in texture write init handling (fixes Deadlock between Queue::write_texture and Queue::submit via inconsistent ordering of trackers and Texture::initialization_status #9460)
• Fix lock release order in PendingSubmission and Queue::write_buffer

Testing
No explicit testing in this PR, but at the tip of my branch, the lock validation build can pass our test suite, see #9479.

Squash or Rebase? Rebase

Checklist

• I self-reviewed and fully understand this PR.
• WebGPU implementations built with wgpu may be affected behaviorally.
• Validation and feature gates are in place to confine behavioral changes.
• Tests demonstrate the validation and altered logic works.
• CHANGELOG.md entries for the user-facing effects of this change are present.
• The PR is minimal, and doesn't make sense to land as multiple PRs.
• Commits are logically scoped and individually reviewable.
• The PR description has enough context to understand the motivation and solution implemented.

[andyleiserson]

The purpose of the changes in memory_init.rs and queue.rs is to drop the initialization status lock before acquiring the tracker lock in clear_texture.

[jimblandy]

@andyleiserson I just pushed some doc comments and a minor tweak to the code; could you see if those seem correct?

[jimblandy]

This commit looks good:

Fix lock release order in PendingSubmission and Queue::write_buffer

[andyleiserson]

Added comments look good, I just noted a few typos and have minor additional clarifications. I will rebase & incorporate these later.

[andyleiserson]

I feel like I wrote this in a comment already, so it may exist somewhere else, but it might be worth noting here that the PendingWrites lock should be acquired at some point between (1) and (3) (generally, as late as is possible while supporting the necessary PendingWrites activity).

[andyleiserson]

The comment I was thinking of is the one on allocate_submission. The doc comments on PendingSubmission and allocate_submission have a fair amount of overlap, but they are presenting things from slightly different perspectives so I didn't try to consolidate them.

I did add a note here (on PendingSubmission) about the timing of acquiring the pending writes lock.

[jimblandy]

Oh, right, I was thinking I would trim down the comments on allocate_submission, and refer the reader to PendingSubmission's comments for details. I think I forgot about that.

[andyleiserson]

I changed my approach in poll_all_devices_of_api (now InstanceDevices::poll_all_devices) a bit. When I originally worked on the lock ranks, there was one registry lock which had a bunch of special locking behavior, and I excluded it from the analysis. But in the current version, the HUB_* locks function like any other.

Previously, I had made poll_all_devices acquire HUB_OTHER, collect all devices, drop the lock, and then poll the collected devices. (Which means that if new devices were created concurrently with polling all devices, the new devices would not be considered.)

Now I've made a dedicated HUB_DEVICES rank, and kept the poll_all_devices code as it was originally: acquire the lock and hold it for the duration of polling.

To actually pass tests with lock validation, HUB_DEVICES needs to be declared as:

    rank HUB_DEVICES "Hub::devices" followed by {
        DEVICE_SNATCHABLE_LOCK,
        QUEUE_PENDING_WRITES,
        DEVICE_TRACKERS,
        QUEUE_LIFE_TRACKER,
        BUFFER_BIND_GROUPS,
        DEVICE_TRACE,
        TEXTURE_BIND_GROUPS,
        TEXTURE_CLEAR_MODE,
        TEXTURE_VIEWS,
        DEVICE_DEFERRED_DESTROY,
        DEVICE_LOST_CLOSURE,
    }

This is a mouthful, I'd be happy to restore the "collect devices then unlock" approach if that seems preferable. I don't think it's much of a functional risk to wgpu, it's more of a functional (or confusion) risk to wgpu users to permit poll_all_devices to run concurrently with device creation and itself.

[jimblandy]

I don't think keeping "followed by" lists short is very important; am I missing something? It seems to me that the overall ordering is what people will be thinking about.

[jimblandy]

So I think having poll_all_devices hold a distinguished HUB_DEVICES for the duration of the polling is fine.

[andyleiserson]

I don't think keeping "followed by" lists short is very important; am I missing something?

My concern is about maintainability. There are a lot of details of behavior of other parts of wgpu captured in rank.rs. This probably wouldn't happen for you or me because we've worked on this code, but I can see a developer being frustrated upon making a change elsewhere in wgpu that seems reasonable, but throws a hard-to-understand error from the lock rank checker that they're not sure what to do with. How much that happens is a cost/risk to weigh against the structural deadlock prevention that the lock ranking can give us.

The general guidance we can offer to avoid the complexity is "don't acquire more than one lock at a time", which if we wanted to put in practice here, we could do by dropping HUB_DEVICE before acquiring the other locks.

That said, I wrote a comment here because it was a notable change from the earlier version of the PR, and to call out a consequence of choices made now that will show up later. I think the way it is now is fine.