-
Notifications
You must be signed in to change notification settings - Fork 431
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
Experimental PR for introducing klint #958
base: rust
Are you sure you want to change the base?
Conversation
@@ -185,6 +185,7 @@ impl<'a, Out, F: FnMut() -> Result<Out> + Send + 'a> SocketFuture<'a, Out, F> { | |||
/// | |||
/// If the state matches the one we're waiting on, we wake up the task so that the future can be | |||
/// polled again. | |||
#[klint::preempt_count(expect = 0..)] // This function can be called from `wake_up` which must noy sleep. |
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.
nit:
#[klint::preempt_count(expect = 0..)] // This function can be called from `wake_up` which must noy sleep. | |
#[klint::preempt_count(expect = 0..)] // This function can be called from `wake_up` which must not sleep. |
@@ -83,18 +85,21 @@ pub fn ref_waker<T: 'static + ArcWake>(w: Arc<T>) -> Waker { | |||
) | |||
} | |||
|
|||
#[klint::preempt_count(expect = 0..)] // Required as `Waker::clone` must not sleep. |
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.
Why Waker::clone
must not sleep, could you elaborate?
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.
It is called by the callback passed to init_waitqueue_func_entry
, which isn't allowed to sleep.
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.
Make senses, however, why do we need clone
in wake_callback
?
/me goes to search PR history
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.
Of course @wedsonaf may know the answer ;-)
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.
Looks like this is done to invoke the waker after unlocking the mutex protecting the waker is released. I would expect this is to prevent a deadlock or something like that.
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.
Looks to me that we can just do the following
if let Some(mut guard) = s.waker.try_lock() {
if let Some(w) = guard.take() {
drop(guard);
w.wake();
return 1;
}
|
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.
Yeah, that's what I suggested in the meeting. But I didn't make the change because I think the whole NoWaitLock<Option<Waker>>
thing should be replaced with AtomicWaker
instead.
This is purely experimental and for demonstration purpose!
How to test it out:
cargo build --release
)export LD_LIBRARY_PATH=$(rustup run 1.66.0 bash -c "echo \$LD_LIBRARY_PATH")
make RUSTC=<path to klint binary>
on kernel tree