From e7b9b1ff1d49e2117f2d78fa5f11b0d006c169f7 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 8 Jan 2024 14:49:59 +0000 Subject: rust: sync: add `CondVar::wait_timeout` 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. Note that it is not enough to avoid jiffies by introducing 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. Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Tiago Lam Reviewed-by: Boqun Feng Signed-off-by: Alice Ryhl Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20240108-rb-new-condvar-methods-v4-3-88e0c871cc05@google.com [ Added `CondVarTimeoutResult` re-export and fixed typo. ] Signed-off-by: Miguel Ojeda --- rust/kernel/task.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'rust/kernel/task.rs') diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs index 9451932d5d86..49a8e10d68f5 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -5,7 +5,10 @@ //! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h). use crate::{bindings, types::Opaque}; -use core::{marker::PhantomData, ops::Deref, ptr}; +use core::{ffi::c_long, marker::PhantomData, ops::Deref, ptr}; + +/// A sentinel value used for infinite timeouts. +pub const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX; /// Returns the currently running task. #[macro_export] -- cgit v1.2.3 From f090f0d0eea9666a96702b29bc9a64cbabee85c5 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 8 Jan 2024 14:50:00 +0000 Subject: rust: sync: update integer types in CondVar 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` (even though that is unlikely to ever happen in the kernel). 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 Reviewed-by: Tiago Lam Reviewed-by: Boqun Feng Reviewed-by: Benno Lossin Reviewed-by: Martin Rodriguez Reboredo Signed-off-by: Alice Ryhl Link: https://lore.kernel.org/r/20240108-rb-new-condvar-methods-v4-4-88e0c871cc05@google.com [ Added note on the unlikeliness of `sizeof(int)` changing. ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/condvar.rs | 38 ++++++++++++++++++++------------------ rust/kernel/task.rs | 15 ++++++++++++++- 2 files changed, 34 insertions(+), 19 deletions(-) (limited to 'rust/kernel/task.rs') diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index 3f148d7040bb..9f1d83589beb 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -7,11 +7,17 @@ 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}; use core::marker::PhantomPinned; +use core::ptr; use macros::pin_data; /// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class. @@ -108,7 +114,7 @@ impl CondVar { fn wait_internal( &self, - wait_state: u32, + wait_state: c_int, guard: &mut Guard<'_, T, B>, timeout_in_jiffies: c_long, ) -> c_long { @@ -119,11 +125,7 @@ impl CondVar { // SAFETY: Both `wait` and `wait_queue_head` point to valid memory. unsafe { - bindings::prepare_to_wait_exclusive( - self.wait_queue_head.get(), - wait.get(), - wait_state as _, - ) + bindings::prepare_to_wait_exclusive(self.wait_queue_head.get(), wait.get(), wait_state) }; // SAFETY: Switches to another thread. The timeout can be any number. @@ -142,7 +144,7 @@ impl CondVar { /// [`CondVar::notify_one`] or [`CondVar::notify_all`]. Note that it may also wake up /// spuriously. pub fn wait(&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. @@ -153,7 +155,7 @@ impl CondVar { /// 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(&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() } @@ -169,7 +171,7 @@ impl CondVar { 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 }, @@ -178,15 +180,15 @@ impl CondVar { } } - /// 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_queue_head` points to valid memory. unsafe { bindings::__wake_up( self.wait_queue_head.get(), - bindings::TASK_NORMAL, + TASK_NORMAL, count, - flags as _, + ptr::null_mut(), ) }; } @@ -198,7 +200,7 @@ impl CondVar { /// CPU. pub fn notify_sync(&self) { // SAFETY: `wait_queue_head` points to valid memory. - unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), bindings::TASK_NORMAL) }; + unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), TASK_NORMAL) }; } /// Wakes a single waiter up, if any. @@ -206,7 +208,7 @@ impl CondVar { /// 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. @@ -214,7 +216,7 @@ impl CondVar { /// 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 49a8e10d68f5..a3a4007db682 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -5,11 +5,24 @@ //! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h). use crate::{bindings, types::Opaque}; -use core::{ffi::c_long, marker::PhantomData, ops::Deref, ptr}; +use core::{ + ffi::{c_int, c_long, c_uint}, + marker::PhantomData, + ops::Deref, + ptr, +}; /// A sentinel 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 { -- cgit v1.2.3 From ebf2b8a75a05a1683596aa73c24c1b5bcd5132ae Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Wed, 31 Jan 2024 21:23:29 +0100 Subject: rust: kernel: unify spelling of refcount in docs Replace instances of 'ref-count[ed]' with 'refcount[ed]' to increase consistency within the Rust documentation. The latter form is used more widely in the rest of the kernel: ```console $ rg '(\*|//).*?\srefcount(|ed)[\s,.]' | wc -l 1605 $ rg '(\*|//).*?\sref-count(|ed)[\s,.]' | wc -l 43 ``` (numbers are for commit 052d534373b7 ("Merge tag 'exfat-for-6.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat")) Signed-off-by: Valentin Obst Reviewed-by: Trevor Gross Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20240131-doc-fixes-v3-v3-7-0c8af94ed7de@valentinobst.de [ Reworded to use the kernel's commit description style. ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 8 ++++---- rust/kernel/task.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'rust/kernel/task.rs') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 77cdbcf7bd2e..6c46b1affca5 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -56,7 +56,7 @@ mod std_vendor; /// b: u32, /// } /// -/// // Create a ref-counted instance of `Example`. +/// // Create a refcounted instance of `Example`. /// let obj = Arc::try_new(Example { a: 10, b: 20 })?; /// /// // Get a new pointer to `obj` and increment the refcount. @@ -510,7 +510,7 @@ impl Deref for ArcBorrow<'_, T> { /// # test().unwrap(); /// ``` /// -/// In the following example we first allocate memory for a ref-counted `Example` but we don't +/// In the following example we first allocate memory for a refcounted `Example` but we don't /// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`], /// followed by a conversion to `Arc`. This is particularly useful when allocation happens /// in one context (e.g., sleepable) and initialisation in another (e.g., atomic): @@ -560,7 +560,7 @@ impl UniqueArc { /// Tries to allocate a new [`UniqueArc`] instance. pub fn try_new(value: T) -> Result { Ok(Self { - // INVARIANT: The newly-created object has a ref-count of 1. + // INVARIANT: The newly-created object has a refcount of 1. inner: Arc::try_new(value)?, }) } @@ -574,7 +574,7 @@ impl UniqueArc { data <- init::uninit::(), }? AllocError))?; Ok(UniqueArc { - // INVARIANT: The newly-created object has a ref-count of 1. + // INVARIANT: The newly-created object has a refcount of 1. // SAFETY: The pointer from the `Box` is valid. inner: unsafe { Arc::from_inner(Box::leak(inner).into()) }, }) diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs index a3a4007db682..6726f1056066 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -39,7 +39,7 @@ macro_rules! current { /// /// All instances are valid tasks created by the C portion of the kernel. /// -/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures +/// Instances of this type are always refcounted, that is, a call to `get_task_struct` ensures /// that the allocation remains valid at least until the matching call to `put_task_struct`. /// /// # Examples @@ -163,7 +163,7 @@ impl Task { } } -// SAFETY: The type invariants guarantee that `Task` is always ref-counted. +// SAFETY: The type invariants guarantee that `Task` is always refcounted. unsafe impl crate::types::AlwaysRefCounted for Task { fn inc_ref(&self) { // SAFETY: The existence of a shared reference means that the refcount is nonzero. -- cgit v1.2.3 From af8b18d7404bfb82aa07a09ad5b53d33ab2955d8 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Wed, 31 Jan 2024 21:23:30 +0100 Subject: rust: kernel: mark code fragments in docs with backticks Fix places where comments include code fragments that are not enclosed in backticks. Signed-off-by: Valentin Obst Reviewed-by: Trevor Gross Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20240131-doc-fixes-v3-v3-8-0c8af94ed7de@valentinobst.de Signed-off-by: Miguel Ojeda --- rust/kernel/ioctl.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/task.rs | 2 +- rust/kernel/workqueue.rs | 9 +++++---- 4 files changed, 8 insertions(+), 7 deletions(-) (limited to 'rust/kernel/task.rs') diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs index 5987ad6d38a7..cfa7d080b531 100644 --- a/rust/kernel/ioctl.rs +++ b/rust/kernel/ioctl.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 -//! ioctl() number definitions. +//! `ioctl()` number definitions. //! //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h) diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 072b8ef2a0fa..10ccc0e39147 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -28,7 +28,7 @@ pub unsafe trait Backend { /// The state required by the lock. type State; - /// The state required to be kept between lock and unlock. + /// The state required to be kept between `lock` and `unlock`. type GuardState; /// Initialises the lock. diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs index 6726f1056066..ca6e7e31d71c 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -132,7 +132,7 @@ impl Task { /// Returns the group leader of the given task. pub fn group_leader(&self) -> &Task { // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always - // have a valid group_leader. + // have a valid `group_leader`. let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) }; // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`, diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index 8775c34d12a5..d900dc911149 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -168,7 +168,7 @@ impl Queue { /// # Safety /// /// The caller must ensure that the provided raw pointer is not dangling, that it points at a - /// valid workqueue, and that it remains valid until the end of 'a. + /// valid workqueue, and that it remains valid until the end of `'a`. pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue { // SAFETY: The `Queue` type is `#[repr(transparent)]`, so the pointer cast is valid. The // caller promises that the pointer is not dangling. @@ -414,8 +414,8 @@ impl Work { /// /// # Safety /// -/// The [`OFFSET`] constant must be the offset of a field in Self of type [`Work`]. The methods on -/// this trait must have exactly the behavior that the definitions given below have. +/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work`]. The +/// methods on this trait must have exactly the behavior that the definitions given below have. /// /// [`Work`]: Work /// [`impl_has_work!`]: crate::impl_has_work @@ -428,7 +428,8 @@ pub unsafe trait HasWork { /// Returns the offset of the [`Work`] field. /// - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not Sized. + /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not + /// `Sized`. /// /// [`Work`]: Work /// [`OFFSET`]: HasWork::OFFSET -- cgit v1.2.3