-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[core] Queue::drop is acquiring the snatch guard as well
#8691
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
Conversation
ErichDonGubler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minus some questions I'd like to fully understand before narrowing lock regions.
|
|
||
| // Don't hold the locks while calling release_gpu_resources. | ||
| drop(fence); | ||
| drop(snatch_guard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I think it's fine—but I'm not positive—to no longer be locking the snatch lock when we take the device lost closure. Tagging in @cwfitzgerald to double-check my conclusion here. Does that sound right, Connor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why this would be problematic. The snatch lock doesn't guard the device lost closure in any way.
|
|
||
| // Don't hold the locks while calling release_gpu_resources. | ||
| drop(fence); | ||
| drop(snatch_guard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why this would be problematic. The snatch lock doesn't guard the device lost closure in any way.
0b4a9af to
67781e8
Compare
cwfitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Description
Fix a deadlock in
Device::maintainTesting
Manually
Checklist
cargo fmt.Runtaplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknownRuncargo xtask testto run tests.If this contains user-facing changes, add aCHANGELOG.mdentry.