Re: [PATCH v3 3/4] rust: sync: add `CondVar::wait_timeout`

[Boqun Feng]

On Thu, Jan 04, 2024 at 02:02:43PM +0000, Alice Ryhl wrote:
> Sleep on a condition variable with a timeout.
> 
> This is used by Rust Binder for process freezing. There, we want to
> sleep until the freeze operation completes, but we want to be able to
> abort the process freezing if it doesn't complete within some timeout.
> 
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
> Reviewed-by: Tiago Lam <tiagolam@xxxxxxxxx>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>

Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>

That said, I want to hear from Thomas on the usage of jiffies, see
below:

> ---
[...]
> -    fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guard<'_, T, B>) {
> +    fn wait_internal<T: ?Sized, B: Backend>(
> +        &self,
> +        wait_state: u32,
> +        guard: &mut Guard<'_, T, B>,
> +        timeout_in_jiffies: c_long,

This is an internal function, and it makes sense we use the same type
for durations as the C function we rely on.

> +    ) -> c_long {
>          let wait = Opaque::<bindings::wait_queue_entry>::uninit();
>  
>          // SAFETY: `wait` points to valid memory.
> @@ -113,11 +122,13 @@ fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guar
>              bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _)
>          };
>  
> -        // SAFETY: No arguments, switches to another thread.
> -        guard.do_unlocked(|| unsafe { bindings::schedule() });
> +        // SAFETY: Switches to another thread. The timeout can be any number.
> +        let ret = guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout_in_jiffies) });
>  
>          // SAFETY: Both `wait` and `wait_list` point to valid memory.
>          unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) };
> +
> +        ret
>      }
>  

[...]

> +    /// Releases the lock and waits for a notification in interruptible mode.
> +    ///
> +    /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the
> +    /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or
> +    /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal.
> +    #[must_use = "wait_interruptible_timeout returns if a signal is pending, so the caller must check the return value"]
> +    pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
> +        &self,
> +        guard: &mut Guard<'_, T, B>,
> +        jiffies: Jiffies,

This is a public interface, so it may make sense use a HZ-independent
type for durations, e.g. core::time::Duration:

	https://doc.rust-lang.org/core/time/struct.Duration.html	

but we don't have enough users to see whether there would be a need for
HZ-dependent durations, so I think it's fine that we stick with a simple
solution in Alice's patchset to get the ball rolling, we can always
remove a public interface with HZ-dependent durations whenever we want.

Thoughts?

Regards,
Boqun

> +    ) -> CondVarTimeoutResult {
> +        let jiffies = jiffies.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT);
> +        let res = self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard, jiffies);
> +
> +        match (res as Jiffies, crate::current!().signal_pending()) {
> +            (jiffies, true) => CondVarTimeoutResult::Signal { jiffies },
> +            (0, false) => CondVarTimeoutResult::Timeout,
> +            (jiffies, false) => CondVarTimeoutResult::Woken { jiffies },
> +        }
> +    }
> +
[...]
> +
> +/// The return type of `wait_timeout`.
> +pub enum CondVarTimeoutResult {
> +    /// The timeout was reached.
> +    Timeout,
> +    /// Somebody woke us up.
> +    Woken {
> +        /// Remaining sleep duration.
> +        jiffies: Jiffies,
> +    },
> +    /// A signal occurred.
> +    Signal {
> +        /// Remaining sleep duration.
> +        jiffies: Jiffies,
> +    },
> +}
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index f12a684bc957..149a5259d431 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -139,7 +139,7 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
>  unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>  
>  impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
> -    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
> +    pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
>          // SAFETY: The caller owns the lock, so it is safe to unlock it.
>          unsafe { B::unlock(self.lock.state.get(), &self.state) };
>  
> @@ -147,7 +147,7 @@ pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {
>          let _relock =
>              ScopeGuard::new(|| unsafe { B::relock(self.lock.state.get(), &mut self.state) });
>  
> -        cb();
> +        cb()
>      }
>  }
>  
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 9451932d5d86..ffb4a51eb898 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -7,6 +7,9 @@
>  use crate::{bindings, types::Opaque};
>  use core::{marker::PhantomData, ops::Deref, ptr};
>  
> +/// A sentinal value used for infinite timeouts.
> +pub const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
> +
>  /// Returns the currently running task.
>  #[macro_export]
>  macro_rules! current {
> 
> -- 
> 2.43.0.472.g3155946c3a-goog
>