Skip to content

Commit a981c28

Browse files
committed
fix(core): gate QueueEmpty on the wait outcome instead of re-reading the fence
Per review feedback, the extra fence read existed only to service the defensive assert. Instead, capture the previously ignored `Ok(bool)` wait outcome and only report `QueueEmpty` when the wait did not time out. A timed-out wait racing a concurrent poller resolves to `WaitSucceeded` or `Timeout` instead, and the next poll observes the empty queue. This makes the documented `PollStatus::QueueEmpty` contract ("implies that the given Wait was satisfied") actually hold. The gate excludes only the timed-out case (`!= Some(false)`) rather than requiring a successful wait, so `PollType::Poll` keeps its ability to report `QueueEmpty`, which `poll_all(force_wait: false)` and non-blocking drain loops rely on. With the gate in place, the existing fence sample is provably at or above the wait index whenever the assert runs: it was read after the successful wait, and the fence is monotonic.
1 parent 9f9f2d4 commit a981c28

2 files changed

Lines changed: 29 additions & 27 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ Bottom level categories:
8080

8181
- Zero-initialize padding (if any) at the end of a buffer allocation. This was application-visible in rare cases on Vulkan when a shader read beyond the valid range of a vertex buffer. By @andyleiserson in [#9791](https://github.com/gfx-rs/wgpu/pull/9791).
8282
- Fix required immediate slots calculation and remove `naga::valid::FunctionInfo::immediate_slots_used`. By @beicause in [#9725](https://github.com/gfx-rs/wgpu/pull/9725).
83-
- Fix a panic on the queue-empty assert in `Device::maintain` when more than one thread polls the same device. By @AdrianEddy in [#9958](https://github.com/gfx-rs/wgpu/pull/9958).
83+
- Fix a panic on the queue-empty assert in `Device::maintain` when more than one thread polls the same device. A timed-out `Wait` poll that races a concurrent poll on another thread now returns `WaitSucceeded` or `Timeout` instead of `QueueEmpty`; `QueueEmpty` is only returned when the poll itself proved it, as its documentation already promised. Polling again reports the empty queue. By @AdrianEddy in [#9958](https://github.com/gfx-rs/wgpu/pull/9958).
8484

8585
#### naga
8686

wgpu-core/src/device/resource.rs

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -993,8 +993,10 @@ impl Device {
993993
wgt::PollType::Poll => None,
994994
};
995995

996-
// Wait for the submission index if requested.
997-
if let Some(target_submission_index) = wait_submission_index {
996+
// Wait for the submission index if requested. `None` if no wait was requested,
997+
// otherwise `Some(succeeded)` with whether the wait was satisfied before the
998+
// timeout. The outcome is consulted below when interpreting `queue_empty`.
999+
let wait_succeeded = if let Some(target_submission_index) = wait_submission_index {
9981000
log::trace!("Device::maintain: waiting for submission index {target_submission_index}");
9991001

10001002
let wait_timeout = match poll_type {
@@ -1009,13 +1011,16 @@ impl Device {
10091011
.wait(self.fence.as_ref(), target_submission_index, wait_timeout)
10101012
};
10111013

1012-
// This error match is only about `DeviceErrors`. At this stage we do not care if
1013-
// the wait succeeded or not, and the `Ok(bool)`` variant is ignored.
1014-
if let Err(e) = wait_result {
1015-
let hal_error: WaitIdleError = self.handle_hal_error(e).into();
1016-
return (user_closures, Err(hal_error));
1014+
match wait_result {
1015+
Ok(succeeded) => Some(succeeded),
1016+
Err(e) => {
1017+
let hal_error: WaitIdleError = self.handle_hal_error(e).into();
1018+
return (user_closures, Err(hal_error));
1019+
}
10171020
}
1018-
}
1021+
} else {
1022+
None
1023+
};
10191024

10201025
// Get the currently finished submission index. This may be higher than the requested
10211026
// wait, or it may be less than the requested wait if the wait failed.
@@ -1071,33 +1076,30 @@ impl Device {
10711076

10721077
// Based on the queue empty status, and the current finished submission index, determine
10731078
// the result of the poll.
1074-
let result = if queue_empty {
1079+
//
1080+
// `queue_empty` alone does not justify reporting `QueueEmpty` after a timed-out
1081+
// wait: `current_finished_submission` is sampled before `Queue::maintain`, but
1082+
// `queue_empty` is observed inside it, and another thread may retire and triage
1083+
// submissions in between (`Device::poll` takes `&self`). Reporting `QueueEmpty`
1084+
// there would claim more than this thread's fence sample proves; `QueueEmpty`
1085+
// is documented to imply the wait was satisfied. Such a race reports
1086+
// `WaitSucceeded` or `Timeout` instead, and a subsequent poll observes the
1087+
// empty queue.
1088+
let result = if queue_empty && wait_succeeded != Some(false) {
10751089
if let Some(wait_submission_index) = wait_submission_index {
10761090
// Assert to ensure that if we received a queue empty status, the fence shows the
10771091
// correct value. This is defensive, as this should never be hit.
10781092
//
1079-
// Re-read the fence rather than reusing `current_finished_submission`:
1080-
// that was sampled before `Queue::maintain`, but `queue_empty` is
1081-
// observed inside it, and another thread may retire and triage
1082-
// submissions in between (`Device::poll` takes `&self`). A fresh read
1083-
// is sound because an empty tracker implies some thread triaged past
1084-
// `wait_submission_index`, which requires a fence read at least that
1085-
// high, and the fence is monotonic.
1086-
let finished_submission =
1087-
match unsafe { self.raw().get_fence_value(self.fence.as_ref()) } {
1088-
Ok(fence_value) => fence_value,
1089-
Err(e) => {
1090-
let hal_error: WaitIdleError = self.handle_hal_error(e).into();
1091-
return (user_closures, Err(hal_error));
1092-
}
1093-
};
1093+
// This branch is reached with a wait index only when the wait succeeded,
1094+
// and `current_finished_submission` was sampled after that wait, so a
1095+
// monotonic fence must show at least `wait_submission_index`.
10941096
assert!(
1095-
finished_submission >= wait_submission_index,
1097+
current_finished_submission >= wait_submission_index,
10961098
concat!(
10971099
"If the queue is empty, the current submission index ",
10981100
"({}) should be at least the wait submission index ({})",
10991101
),
1100-
finished_submission,
1102+
current_finished_submission,
11011103
wait_submission_index,
11021104
);
11031105
}

0 commit comments

Comments
 (0)