-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove AtomicBool from Waker #18
Comments
This logic is not correct as a function can call a waker prior to returning Pending and expect to be re-enqueued immediately. The proposed use of wfe and sev wouldn't handle this correctly. |
I was planning on taking some time to dig into this. One reason for having an atomic bool there is to provide better performance on spuriously exiting I think removing it could still be logically correct in the current implementations, as far as I'm aware |
You are correct, I did wonder why I couldn't produce the "expected" behaviour. See here. I would have thought that in most cases the underlying poll operation will simply be checking a bit in a register and so I'm not sure that the AtomicBool would necessarily perform better. Happy for you to reopen |
This might be a conceptual misunderstanding on my part, but I believe you can remove the AtomicBool from Waker and this would remove the static lifetime requirement, as the Waker would then have no state - it could be a nullptr and the vtable static.
The key observation is it is not illegal to call poll again on a future that has returned Pending but not invoked the waker, just ill-advised if you can do better. In this case we can't - all the AtomicBool does is move the location of the busy wait.
On armv6m and armv7m the AtomicBool is ultimately duplicating the functionality already provided by the wfe - assuming it isn't actually treated as a noop, in which case busy waiting is the best you can do anyway.
If my reasoning is wrong, I'd be very interested to know as I'm currently working on a PoC embedded_hal, see here, that somewhat relies on this logic being correct...
The text was updated successfully, but these errors were encountered: