Skip to content
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

Draft
wants to merge 2 commits into
base: rust
Choose a base branch
from

Conversation

nbdd0121
Copy link
Member

@nbdd0121 nbdd0121 commented Feb 3, 2023

This is purely experimental and for demonstration purpose!

How to test it out:

  • Compile klint (clone and cargo build --release)
  • Execute export LD_LIBRARY_PATH=$(rustup run 1.66.0 bash -c "echo \$LD_LIBRARY_PATH")
  • Run make RUSTC=<path to klint binary> on kernel tree

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
#[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.
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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 ;-)

Copy link
Member

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.

Copy link
Member

@fbq fbq Feb 3, 2023

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;
            }
        |

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants