This patchset contains some CondVar methods that Rust Binder needs.
The CondVar type implements a condition variable, and tries to mirror
the API of the CondVar type provided by the Rust standard library [2].
It is implemented using a `wait_queue_head`.
Please see the Rust Binder RFC for usage examples [1].
Users of rust: sync: add `CondVar::notify_sync`:
[PATCH RFC 04/20] rust_binder: add work lists
[PATCH RFC 07/20] rust_binder: add epoll support
[PATCH RFC 08/20] rust_binder: add non-oneway transactions
Users of rust: time: add msecs to jiffies conversion:
[PATCH v2 3/3] rust: sync: add `CondVar::wait_timeout`
[PATCH RFC 15/20] rust_binder: add process freezing
Users of rust: sync: add `CondVar::wait_timeout`:
[PATCH RFC 15/20] rust_binder: add process freezing
This patchset is based on top of Boqun's patch [3] that renames the
existing wait methods to follow the C convention of using the
_interruptable suffix.
Link: https://lore.kernel.org/rust-for-linux/[email protected]/ [1]
Link: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html [2]
Link: https://lore.kernel.org/all/[email protected]/ [3]
Signed-off-by: Alice Ryhl <[email protected]>
---
Changes in v3:
- Update module description for new `time` module.
- Move constants to `rust/kernel/task.rs`.
- Fix incorrect doc-comment on `CondVar::notify`.
- Rename c_long argument to `timeout_in_jiffies`.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Introduce "rust: time: add msecs to jiffies conversion" patch.
- Introduce "rust: sync: update integer types in CondVar" patch.
- Merge wait_internal and wait_internal_timeout.
- Use new Jiffies type alias instead of u64.
- Update names to use _interruptable suffix (and base patchset on top of [3]).
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Alice Ryhl (4):
rust: sync: add `CondVar::notify_sync`
rust: time: add msecs to jiffies conversion
rust: sync: add `CondVar::wait_timeout`
rust: sync: update integer types in CondVar
rust/bindings/bindings_helper.h | 1 +
rust/kernel/lib.rs | 1 +
rust/kernel/sync/condvar.rs | 90 ++++++++++++++++++++++++++++++++---------
rust/kernel/sync/lock.rs | 4 +-
rust/kernel/task.rs | 11 +++++
rust/kernel/time.rs | 20 +++++++++
6 files changed, 106 insertions(+), 21 deletions(-)
---
base-commit: 711cbfc717650532624ca9f56fbaf191bed56e67
change-id: 20231205-rb-new-condvar-methods-27ba95df5d41
Best regards,
--
Alice Ryhl <[email protected]>
Wake up another thread synchronously.
This method behaves like `notify_one`, except that it hints to the
scheduler that the current thread is about to go to sleep, so it should
schedule the target thread on the same CPU.
This is used by Rust Binder as a performance optimization. When sending
a transaction to a different process, we usually know which thread will
handle it, so we can schedule that thread for execution next on this
CPU for better cache locality.
Reviewed-by: Benno Lossin <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Tiago Lam <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/sync/condvar.rs | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index f65e19d5a37c..1a3f7b8e03dc 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -155,6 +155,12 @@ fn notify(&self, count: i32, flags: u32) {
};
}
+ /// Calls the kernel function to notify one thread synchronously.
+ pub fn notify_sync(&self) {
+ // SAFETY: `wait_list` points to valid memory.
+ unsafe { bindings::__wake_up_sync(self.wait_list.get(), bindings::TASK_NORMAL) };
+ }
+
/// Wakes a single waiter up, if any.
///
/// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
--
2.43.0.472.g3155946c3a-goog
Defines type aliases and conversions for msecs and jiffies.
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.
The freeze timeout is supplied in msecs.
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.
Suggested-by: Boqun Feng <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Tiago Lam <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/lib.rs | 1 +
rust/kernel/time.rs | 20 ++++++++++++++++++++
3 files changed, 22 insertions(+)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index b5714fb69fe3..227dc9240f3f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
#include <kunit/test.h>
#include <linux/errname.h>
+#include <linux/jiffies.h>
#include <linux/slab.h>
#include <linux/refcount.h>
#include <linux/wait.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e6aff80b521f..d4f90acdd517 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,7 @@
pub mod str;
pub mod sync;
pub mod task;
+pub mod time;
pub mod types;
pub mod workqueue;
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
new file mode 100644
index 000000000000..25a896eed468
--- /dev/null
+++ b/rust/kernel/time.rs
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Time related primitives.
+//!
+//! This module contains the kernel APIs related to time and timers that
+//! have been ported or wrapped for usage by Rust code in the kernel.
+
+/// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
+pub type Jiffies = core::ffi::c_ulong;
+
+/// The millisecond time unit.
+pub type Msecs = core::ffi::c_uint;
+
+/// Converts milliseconds to jiffies.
+#[inline]
+pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
+ // SAFETY: The `__msecs_to_jiffies` function is always safe to call no
+ // matter what the argument is.
+ unsafe { bindings::__msecs_to_jiffies(msecs) }
+}
--
2.43.0.472.g3155946c3a-goog
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 <[email protected]>
Reviewed-by: Tiago Lam <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
---
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-goog
Reduce the chances of compilation failures due to integer type
mismatches in `CondVar`.
When an integer is defined using a #define in C, bindgen doesn't know
which integer type it is supposed to be, so it will just use `u32` by
default (if it fits in an u32). Whenever the right type is something
else, we insert a cast in Rust. However, this means that the code has a
lot of extra casts, and sometimes the code will be missing casts if u32
happens to be correct on the developer's machine, even though the type
might be something else on a different platform.
This patch updates all uses of such constants in
`rust/kernel/sync/condvar.rs` to use constants defined with the right
type. This allows us to remove various unnecessary casts, while also
future-proofing for the case where `unsigned int != u32`.
I wrote this patch at the suggestion of Benno in [1].
Link: https://lore.kernel.org/all/nAEg-6vbtX72ZY3oirDhrSEf06TBWmMiTt73EklMzEAzN4FD4mF3TPEyAOxBZgZtjzoiaBYtYr3s8sa9wp1uYH9vEWRf2M-Lf4I0BY9rAgk=@proton.me/ [1]
Suggested-by: Benno Lossin <[email protected]>
Reviewed-by: Tiago Lam <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/sync/condvar.rs | 38 ++++++++++++++++++--------------------
rust/kernel/task.rs | 8 ++++++++
2 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 45ba57468717..d907b8cb29eb 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -7,10 +7,15 @@
use super::{lock::Backend, lock::Guard, LockClassKey};
use crate::{
- bindings, init::PinInit, pin_init, str::CStr, task::MAX_SCHEDULE_TIMEOUT, time::Jiffies,
+ bindings,
+ init::PinInit,
+ pin_init,
+ str::CStr,
+ task::{MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE},
+ time::Jiffies,
types::Opaque,
};
-use core::ffi::c_long;
+use core::ffi::{c_int, c_long, c_uint};
use core::marker::PhantomPinned;
use macros::pin_data;
@@ -108,7 +113,7 @@ pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self
fn wait_internal<T: ?Sized, B: Backend>(
&self,
- wait_state: u32,
+ wait_state: c_int,
guard: &mut Guard<'_, T, B>,
timeout_in_jiffies: c_long,
) -> c_long {
@@ -119,7 +124,7 @@ fn wait_internal<T: ?Sized, B: Backend>(
// SAFETY: Both `wait` and `wait_list` point to valid memory.
unsafe {
- bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _)
+ bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state)
};
// SAFETY: Switches to another thread. The timeout can be any number.
@@ -138,7 +143,7 @@ fn wait_internal<T: ?Sized, B: Backend>(
/// [`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, MAX_SCHEDULE_TIMEOUT);
+ self.wait_internal(TASK_UNINTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
}
/// Releases the lock and waits for a notification in interruptible mode.
@@ -149,7 +154,7 @@ 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, MAX_SCHEDULE_TIMEOUT);
+ self.wait_internal(TASK_INTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
crate::current!().signal_pending()
}
@@ -165,7 +170,7 @@ pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
jiffies: Jiffies,
) -> CondVarTimeoutResult {
let jiffies = jiffies.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT);
- let res = self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard, jiffies);
+ let res = self.wait_internal(TASK_INTERRUPTIBLE, guard, jiffies);
match (res as Jiffies, crate::current!().signal_pending()) {
(jiffies, true) => CondVarTimeoutResult::Signal { jiffies },
@@ -174,23 +179,16 @@ pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
}
}
- /// Calls the kernel function to notify the appropriate number of threads with the given flags.
- fn notify(&self, count: i32, flags: u32) {
+ /// Calls the kernel function to notify the appropriate number of threads.
+ fn notify(&self, count: c_int) {
// SAFETY: `wait_list` points to valid memory.
- unsafe {
- bindings::__wake_up(
- self.wait_list.get(),
- bindings::TASK_NORMAL,
- count,
- flags as _,
- )
- };
+ unsafe { bindings::__wake_up(self.wait_list.get(), TASK_NORMAL, count, ptr::null_mut()) };
}
/// Calls the kernel function to notify one thread synchronously.
pub fn notify_sync(&self) {
// SAFETY: `wait_list` points to valid memory.
- unsafe { bindings::__wake_up_sync(self.wait_list.get(), bindings::TASK_NORMAL) };
+ unsafe { bindings::__wake_up_sync(self.wait_list.get(), TASK_NORMAL) };
}
/// Wakes a single waiter up, if any.
@@ -198,7 +196,7 @@ pub fn notify_sync(&self) {
/// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
/// completely (as opposed to automatically waking up the next waiter).
pub fn notify_one(&self) {
- self.notify(1, 0);
+ self.notify(1);
}
/// Wakes all waiters up, if any.
@@ -206,7 +204,7 @@ pub fn notify_one(&self) {
/// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
/// completely (as opposed to automatically waking up the next waiter).
pub fn notify_all(&self) {
- self.notify(0, 0);
+ self.notify(0);
}
}
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index ffb4a51eb898..37468613ac0d 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -10,6 +10,14 @@
/// A sentinal value used for infinite timeouts.
pub const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
+/// Bitmask for tasks that are sleeping in an interruptible state.
+pub const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
+/// Bitmask for tasks that are sleeping in an uninterruptible state.
+pub const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
+/// Convenience constant for waking up tasks regardless of whether they are in interruptible or
+/// uninterruptible sleep.
+pub const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
+
/// Returns the currently running task.
#[macro_export]
macro_rules! current {
--
2.43.0.472.g3155946c3a-goog
On Thu, Jan 4, 2024 at 3:03 PM Alice Ryhl <[email protected]> wrote:
> This patchset is based on top of Boqun's patch [3] that renames the
> existing wait methods to follow the C convention of using the
> _interruptable suffix.
I forgot to update this part. It's actually based on top of rust-next
(which has patch [3]).
On 1/4/24 11:02, Alice Ryhl wrote:
> Reduce the chances of compilation failures due to integer type
> mismatches in `CondVar`.
>
> When an integer is defined using a #define in C, bindgen doesn't know
> which integer type it is supposed to be, so it will just use `u32` by
> default (if it fits in an u32). Whenever the right type is something
> else, we insert a cast in Rust. However, this means that the code has a
> lot of extra casts, and sometimes the code will be missing casts if u32
> happens to be correct on the developer's machine, even though the type
> might be something else on a different platform.
>
> This patch updates all uses of such constants in
> `rust/kernel/sync/condvar.rs` to use constants defined with the right
> type. This allows us to remove various unnecessary casts, while also
> future-proofing for the case where `unsigned int != u32`.
>
> I wrote this patch at the suggestion of Benno in [1].
>
> Link: https://lore.kernel.org/all/nAEg-6vbtX72ZY3oirDhrSEf06TBWmMiTt73EklMzEAzN4FD4mF3TPEyAOxBZgZtjzoiaBYtYr3s8sa9wp1uYH9vEWRf2M-Lf4I0BY9rAgk=@proton.me/ [1]
> Suggested-by: Benno Lossin <[email protected]>
> Reviewed-by: Tiago Lam <[email protected]>
> Reviewed-by: Boqun Feng <[email protected]>
> Reviewed-by: Benno Lossin <[email protected]>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
On Thu, Jan 04, 2024 at 02:02:41PM +0000, Alice Ryhl wrote:
> Wake up another thread synchronously.
>
> This method behaves like `notify_one`, except that it hints to the
> scheduler that the current thread is about to go to sleep, so it should
> schedule the target thread on the same CPU.
>
> This is used by Rust Binder as a performance optimization. When sending
> a transaction to a different process, we usually know which thread will
> handle it, so we can schedule that thread for execution next on this
> CPU for better cache locality.
>
> Reviewed-by: Benno Lossin <[email protected]>
> Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
> Reviewed-by: Tiago Lam <[email protected]>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/sync/condvar.rs | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
> index f65e19d5a37c..1a3f7b8e03dc 100644
> --- a/rust/kernel/sync/condvar.rs
> +++ b/rust/kernel/sync/condvar.rs
> @@ -155,6 +155,12 @@ fn notify(&self, count: i32, flags: u32) {
> };
> }
>
> + /// Calls the kernel function to notify one thread synchronously.
I feel like "synchronously" needs some explanation here (e.g. using the
same description in the commit log), but let's see if other future users
really need this ;-)
Reviewed-by: Boqun Feng <[email protected]>
Regards,
Boqun
> + pub fn notify_sync(&self) {
> + // SAFETY: `wait_list` points to valid memory.
> + unsafe { bindings::__wake_up_sync(self.wait_list.get(), bindings::TASK_NORMAL) };
> + }
> +
> /// Wakes a single waiter up, if any.
> ///
> /// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
>
> --
> 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 <[email protected]>
> Reviewed-by: Tiago Lam <[email protected]>
> Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
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
>
On 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:44PM +0000, Alice Ryhl wrote:
[...]
> -use core::ffi::c_long;
> +use core::ffi::{c_int, c_long, c_uint};
core::ffi::c_uint is not needed here. And
use core::ptr;
is missing (for the `ptr::mut_null()` below)
> use core::marker::PhantomPinned;
> use macros::pin_data;
>
[...]
> @@ -174,23 +179,16 @@ pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
> }
> }
>
> - /// Calls the kernel function to notify the appropriate number of threads with the given flags.
> - fn notify(&self, count: i32, flags: u32) {
> + /// Calls the kernel function to notify the appropriate number of threads.
> + fn notify(&self, count: c_int) {
> // SAFETY: `wait_list` points to valid memory.
> - unsafe {
> - bindings::__wake_up(
> - self.wait_list.get(),
> - bindings::TASK_NORMAL,
> - count,
> - flags as _,
> - )
> - };
> + unsafe { bindings::__wake_up(self.wait_list.get(), TASK_NORMAL, count, ptr::null_mut()) };
> }
>
[...]
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index ffb4a51eb898..37468613ac0d 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -10,6 +10,14 @@
Missing:
use core::ffi::{c_long, c_int, c_uint};
here.
Regards,
Boqun
> /// A sentinal value used for infinite timeouts.
> pub const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
>
> +/// Bitmask for tasks that are sleeping in an interruptible state.
> +pub const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
> +/// Bitmask for tasks that are sleeping in an uninterruptible state.
> +pub const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
> +/// Convenience constant for waking up tasks regardless of whether they are in interruptible or
> +/// uninterruptible sleep.
> +pub const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
> +
> /// Returns the currently running task.
> #[macro_export]
> macro_rules! current {
>
> --
> 2.43.0.472.g3155946c3a-goog
>
On Thu, Jan 4, 2024 at 11:48 PM Boqun Feng <[email protected]> wrote:
>
> On Thu, Jan 04, 2024 at 02:02:44PM +0000, Alice Ryhl wrote:
> [...]
> > -use core::ffi::c_long;
> > +use core::ffi::{c_int, c_long, c_uint};
>
> core::ffi::c_uint is not needed here. And
>
> use core::ptr;
>
> is missing (for the `ptr::mut_null()` below)
Oops, looks like I messed up this patch. Sorry about that.
Boqun Feng <[email protected]> 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 <[email protected]>
>> Reviewed-by: Tiago Lam <[email protected]>
>> Signed-off-by: Alice Ryhl <[email protected]>
>
> Reviewed-by: Boqun Feng <[email protected]>
>
> 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