[PATCH v2 0/4] Additional CondVar methods needed by Rust Binder

[PATCH v2 0/4] Additional CondVar methods needed by Rust Binder

[Alice Ryhl]

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/20231101-rust-binder-v1-0-08ba9197f637@google.com/ [1]
Link: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html [2]
Link: https://lore.kernel.org/all/20231214200421.690629-1-boqun.feng@gmail.com/ [3]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
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/20231206-rb-new-condvar-methods-v1-0-33a4cab7fdaa@google.com

---
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     | 86 ++++++++++++++++++++++++++++++++---------
 rust/kernel/sync/lock.rs        |  4 +-
 rust/kernel/time.rs             | 17 ++++++++
 5 files changed, 89 insertions(+), 20 deletions(-)
---
base-commit: 2a76b6e08193d2997689011321bcf230f0c8d4fe
change-id: 20231205-rb-new-condvar-methods-27ba95df5d41

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v2 1/4] rust: sync: add `CondVar::notify_sync`

[Alice Ryhl]

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 <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 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 8630faa29b78..9331eb606738 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

Re: [PATCH v2 1/4] rust: sync: add `CondVar::notify_sync`

[Tiago Lam]

On 16/12/2023 15:31, 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 <benno.lossin@proton.me>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Tiago Lam <tiagolam@gmail.com>

[PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Alice Ryhl]

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 <boqun.feng@gmail.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/lib.rs              |  1 +
 rust/kernel/time.rs             | 17 +++++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 85f013ed4ca4..c482c8018f3d 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..23c4d1a74f68
--- /dev/null
+++ b/rust/kernel/time.rs
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Timers.
+
+/// 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

Re: [PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Martin Rodriguez Reboredo]

On 12/16/23 12:31, Alice Ryhl wrote:
> 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 <boqun.feng@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Tiago Lam]

On 16/12/2023 15:31, Alice Ryhl wrote:
> 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 <boqun.feng@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Tiago Lam <tiagolam@gmail.com>

Re: [PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Benno Lossin]

On 12/16/23 16:31, Alice Ryhl wrote:
> 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 <boqun.feng@gmail.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

-- 
Cheers,
Benno

Re: [PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Boqun Feng]

On Sat, Dec 16, 2023 at 03:31:40PM +0000, Alice Ryhl wrote:
[...]
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> new file mode 100644
> index 000000000000..23c4d1a74f68
> --- /dev/null
> +++ b/rust/kernel/time.rs
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Timers.
> +

Please consider the following mod level description:

//! 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.

Otherwise it looks fine to me.

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

Regards,
Boqun

> +/// 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
> 
> 

Re: [PATCH v2 2/4] rust: time: add msecs to jiffies conversion

[Alice Ryhl]

On Mon, Dec 18, 2023 at 10:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Sat, Dec 16, 2023 at 03:31:40PM +0000, Alice Ryhl wrote:
> [...]
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > new file mode 100644
> > index 000000000000..23c4d1a74f68
> > --- /dev/null
> > +++ b/rust/kernel/time.rs
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Timers.
> > +
>
> Please consider the following mod level description:
>
> //! 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.
>
> Otherwise it looks fine to me.
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Sure, that sounds good to me. I'll update the module description and
add your tag.

Alice

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

[Alice Ryhl]

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.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/condvar.rs | 59 ++++++++++++++++++++++++++++++++++++++++-----
 rust/kernel/sync/lock.rs    |  4 +--
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 9331eb606738..0176cdfced6c 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -6,7 +6,8 @@
 //! 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, time::Jiffies, types::Opaque};
+use core::ffi::c_long;
 use core::marker::PhantomPinned;
 use macros::pin_data;
 
@@ -18,6 +19,8 @@ macro_rules! new_condvar {
     };
 }
 
+const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
+
 /// A conditional variable.
 ///
 /// Exposes the kernel's [`struct wait_queue_head`] as a condition variable. It allows the caller to
@@ -102,7 +105,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: c_long,
+    ) -> c_long {
         let wait = Opaque::<bindings::wait_queue_entry>::uninit();
 
         // SAFETY: `wait` points to valid memory.
@@ -113,11 +121,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) });
 
         // 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 +137,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 +148,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 +208,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()
     }
 }
 

-- 
2.43.0.472.g3155946c3a-goog

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

[Martin Rodriguez Reboredo]

On 12/16/23 12:31, 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

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

[Tiago Lam]

On 16/12/2023 15:31, 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Tiago Lam <tiagolam@gmail.com>

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

[Boqun Feng]

On Sat, Dec 16, 2023 at 03:31:41PM +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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/sync/condvar.rs | 59 ++++++++++++++++++++++++++++++++++++++++-----
>  rust/kernel/sync/lock.rs    |  4 +--
>  2 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
> index 9331eb606738..0176cdfced6c 100644
> --- a/rust/kernel/sync/condvar.rs
> +++ b/rust/kernel/sync/condvar.rs
> @@ -6,7 +6,8 @@
>  //! 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, time::Jiffies, types::Opaque};
> +use core::ffi::c_long;
>  use core::marker::PhantomPinned;
>  use macros::pin_data;
>  
> @@ -18,6 +19,8 @@ macro_rules! new_condvar {
>      };
>  }
>  
> +const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
> +

I'd like to put this in rust/kernel/time.rs or rust/kernel/task.rs, but
it's not a blocker.

>  /// A conditional variable.
>  ///
>  /// Exposes the kernel's [`struct wait_queue_head`] as a condition variable. It allows the caller to
> @@ -102,7 +105,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: c_long,

Nit: maybe `timeout_in_jiffies` instead of `timeout`? Or we have another
data type:

	pub type DeltaJiffies = c_long;

or 

	pub type JiffyDelta = c_long;

because a "c_long timeout" really hurts the readability.

Regards,
Boqun

> +    ) -> c_long {
>          let wait = Opaque::<bindings::wait_queue_entry>::uninit();
>  
>          // SAFETY: `wait` points to valid memory.
> @@ -113,11 +121,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 _)
>          };
>  
[...]

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

[Alice Ryhl]

On Mon, Dec 18, 2023 at 10:15 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Sat, Dec 16, 2023 at 03:31:41PM +0000, Alice Ryhl wrote:
> > +const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
> > +
>
> I'd like to put this in rust/kernel/time.rs or rust/kernel/task.rs, but
> it's not a blocker.

I'll move it to task.rs.

> > -    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: c_long,
>
> Nit: maybe `timeout_in_jiffies` instead of `timeout`? Or we have another
> data type:
>
>         pub type DeltaJiffies = c_long;
>
> or
>
>         pub type JiffyDelta = c_long;
>
> because a "c_long timeout" really hurts the readability.

I will rename this to timeout_in_jiffies.

Alice

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

[Benno Lossin]

On 12/16/23 16:31, Alice Ryhl wrote:
> @@ -102,7 +105,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: c_long,
> +    ) -> c_long {
>          let wait = Opaque::<bindings::wait_queue_entry>::uninit();
> 
>          // SAFETY: `wait` points to valid memory.
> @@ -113,11 +121,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) });

I am not sure what exactly the safety requirements of `schedule_timeout`
are. I looked at the function and saw that the timout should not be
negative. But aside from that only the the context switching should be
relevant. What things are not allowed to do when calling `schedule`
(aside from the stuff that klint catches)?
Because if there are none, then I would put the "switches to another
thread" part into a normal comment.

-- 
Cheers,
Benno

> 
>          // SAFETY: Both `wait` and `wait_list` point to valid memory.
>          unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) };
> +
> +        ret
>      }

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

[Boqun Feng]

On Wed, Dec 20, 2023 at 11:31:05AM +0000, Benno Lossin wrote:
> On 12/16/23 16:31, Alice Ryhl wrote:
> > @@ -102,7 +105,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: c_long,
> > +    ) -> c_long {
> >          let wait = Opaque::<bindings::wait_queue_entry>::uninit();
> > 
> >          // SAFETY: `wait` points to valid memory.
> > @@ -113,11 +121,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) });
> 
> I am not sure what exactly the safety requirements of `schedule_timeout`
> are. I looked at the function and saw that the timout should not be
> negative. But aside from that only the the context switching should be
> relevant. What things are not allowed to do when calling `schedule`
> (aside from the stuff that klint catches)?

One thing is that you probably don't want to call `schedule` with task
state being TASK_DEAD, if so the `schedule` would be counted as
`ARef<Task>::drop()`, see __schedule() -> context_switch() ->
finish_context_switch(), and the task may be freed after that, which
free the stack of the task, and anything that references a object on the
stack would be a UAF. On the other hand, if the task state is not
TASK_DEAD, `schedule*()` should be a no-op regarding memory safety.

> Because if there are none, then I would put the "switches to another
> thread" part into a normal comment.
> 

I think it's possible to make schedule_timeout() a safe function: we can
define setting task state TASK_DEAD as an unsafe operation, whose safety
requirement is something like: "Must ensure that if some code can
reference a memory object that belongs to the task (e.g. a stack
variable) after the task calls a followed `schedule()`, the code must
also hold an additional reference count to the task."

Yes, it might be out of the scope of this patchset though.

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> > 
> >          // SAFETY: Both `wait` and `wait_list` point to valid memory.
> >          unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) };
> > +
> > +        ret
> >      }
> 

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

[Alice Ryhl]

On Thu, Dec 21, 2023 at 5:54 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 11:31:05AM +0000, Benno Lossin wrote:
> > On 12/16/23 16:31, Alice Ryhl wrote:
> > > @@ -102,7 +105,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: c_long,
> > > +    ) -> c_long {
> > >          let wait = Opaque::<bindings::wait_queue_entry>::uninit();
> > >
> > >          // SAFETY: `wait` points to valid memory.
> > > @@ -113,11 +121,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) });
> >
> > I am not sure what exactly the safety requirements of `schedule_timeout`
> > are. I looked at the function and saw that the timout should not be
> > negative. But aside from that only the the context switching should be
> > relevant. What things are not allowed to do when calling `schedule`
> > (aside from the stuff that klint catches)?
>
> One thing is that you probably don't want to call `schedule` with task
> state being TASK_DEAD, if so the `schedule` would be counted as
> `ARef<Task>::drop()`, see __schedule() -> context_switch() ->
> finish_context_switch(), and the task may be freed after that, which
> free the stack of the task, and anything that references a object on the
> stack would be a UAF. On the other hand, if the task state is not
> TASK_DEAD, `schedule*()` should be a no-op regarding memory safety.
>
> > Because if there are none, then I would put the "switches to another
> > thread" part into a normal comment.
> >
>
> I think it's possible to make schedule_timeout() a safe function: we can
> define setting task state TASK_DEAD as an unsafe operation, whose safety
> requirement is something like: "Must ensure that if some code can
> reference a memory object that belongs to the task (e.g. a stack
> variable) after the task calls a followed `schedule()`, the code must
> also hold an additional reference count to the task."
>
> Yes, it might be out of the scope of this patchset though.

These things sound like they are out of scope of this patchset.
Changing it from schedule to schedule_timeout doesn't change whether
this is ok or not.

Alice

[PATCH v2 4/4] rust: sync: update integer types in CondVar

[Alice Ryhl]

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 <benno.lossin@proton.me>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/condvar.rs | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 0176cdfced6c..a0d45dc97661 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -7,7 +7,7 @@
 
 use super::{lock::Backend, lock::Guard, LockClassKey};
 use crate::{bindings, init::PinInit, pin_init, str::CStr, 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;
 
@@ -21,6 +21,10 @@ macro_rules! new_condvar {
 
 const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
 
+const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
+const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
+const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
+
 /// A conditional variable.
 ///
 /// Exposes the kernel's [`struct wait_queue_head`] as a condition variable. It allows the caller to
@@ -107,7 +111,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: c_long,
     ) -> c_long {
@@ -118,7 +122,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.
@@ -137,7 +141,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.
@@ -148,7 +152,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()
     }
 
@@ -164,7 +168,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,22 +178,15 @@ 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) {
+    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.
@@ -197,7 +194,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.
@@ -205,7 +202,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);
     }
 }
 

-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Martin Rodriguez Reboredo]

On 12/16/23 12:31, 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 <benno.lossin@proton.me>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]
> @@ -174,22 +178,15 @@ pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
>       }
>   
>       /// Calls the kernel function to notify the appropriate number of threads with the given flags.

There are no more flags, please update the comment.

> -    fn notify(&self, count: i32, flags: u32) {
> +    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()) };
>       }
> [...]

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Alice Ryhl]

On Sat, Dec 16, 2023 at 5:42 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
> On 12/16/23 12:31, Alice Ryhl wrote:
> >       /// Calls the kernel function to notify the appropriate number of threads with the given flags.
>
> There are no more flags, please update the comment.

Nice catch! Fixed.

Alice

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Tiago Lam]

On 16/12/2023 15:31, 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 <benno.lossin@proton.me>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Tiago Lam <tiagolam@gmail.com>

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Benno Lossin]

On 12/16/23 16:31, 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 <benno.lossin@proton.me>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

-- 
Cheers,
Benno

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Boqun Feng]

On Sat, Dec 16, 2023 at 03:31:42PM +0000, Alice Ryhl wrote:
[...]
>  
> +const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
> +const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
> +const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
> +

Similarly, these definitions are better in rust/kernel/task.rs.

The rest looks fine to me.

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

Regards,
Boqun

Re: [PATCH v2 4/4] rust: sync: update integer types in CondVar

[Alice Ryhl]

On Mon, Dec 18, 2023 at 10:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Sat, Dec 16, 2023 at 03:31:42PM +0000, Alice Ryhl wrote:
> [...]
> >
> > +const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
> > +const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
> > +const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
> > +
>
> Similarly, these definitions are better in rust/kernel/task.rs.
>
> The rest looks fine to me.
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Thanks! I'll move these and add your tag.

Alice