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@gmail.com>
Reviewed-by: Tiago Lam <tiagolam@gmail.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/sync/condvar.rs | 60 ++++++++++++++++++++++++++++++++++++++++-----
rust/kernel/sync/lock.rs | 4 +--
rust/kernel/task.rs | 3 +++
3 files changed, 59 insertions(+), 8 deletions(-)
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 1a3f7b8e03dc..45ba57468717 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -6,7 +6,11 @@
//! variable.
use super::{lock::Backend, lock::Guard, LockClassKey};
-use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
+use crate::{
+ bindings, init::PinInit, pin_init, str::CStr, task::MAX_SCHEDULE_TIMEOUT, time::Jiffies,
+ types::Opaque,
+};
+use core::ffi::c_long;
use core::marker::PhantomPinned;
use macros::pin_data;
@@ -102,7 +106,12 @@ pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self
})
}
- 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,
+ ) -> 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 uninterruptible mode.
@@ -127,7 +138,7 @@ fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guar
/// [`CondVar::notify_one`] or [`CondVar::notify_all`]. Note that it may also wake up
/// spuriously.
pub fn wait<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) {
- self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard);
+ self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
}
/// Releases the lock and waits for a notification in interruptible mode.
@@ -138,10 +149,31 @@ pub fn wait<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) {
/// Returns whether there is a signal pending.
#[must_use = "wait_interruptible returns if a signal is pending, so the caller must check the return value"]
pub fn wait_interruptible<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) -> bool {
- self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard);
+ self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
crate::current!().signal_pending()
}
+ /// 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,
+ ) -> 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 },
+ }
+ }
+
/// Calls the kernel function to notify the appropriate number of threads with the given flags.
fn notify(&self, count: i32, flags: u32) {
// SAFETY: `wait_list` points to valid memory.
@@ -177,3 +209,19 @@ pub fn notify_all(&self) {
self.notify(0, 0);
}
}
+
+/// 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-googOn Thu, Jan 04, 2024 at 02:02:43PM +0000, Alice Ryhl wrote:
[...]
> 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};
>
Missing:
use core::ffi::c_long;
here.
Regards,
Boqun
> +/// 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
>
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@gmail.com>
> Reviewed-by: Tiago Lam <tiagolam@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
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
>
Boqun Feng <boqun.feng@gmail.com> writes:
> 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@gmail.com>
>> Reviewed-by: Tiago Lam <tiagolam@gmail.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>
> 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.
>
>> + /// 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?
I tried to justify why I did not do that in the commit message for patch
2. Let me include it here:
Note that we need to convert to jiffies in Binder. It is not enough to
introduce a variant of `CondVar::wait_timeout` that takes the timeout in
msecs because we need to be able to restart the sleep with the remaining
sleep duration if it is interrupted, and if the API takes msecs rather
than jiffies, then that would require a conversion roundtrip jiffies->
msecs->jiffies that is best avoided.
Alice
© 2016 - 2025 Red Hat, Inc.