* [PATCH v2 00/14] hrtimer Rust API
@ 2024-09-17 22:27 Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 01/14] rust: time: Add Ktime::from_ns() Andreas Hindborg
` (15 more replies)
0 siblings, 16 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Hi!
This series adds support for using the `hrtimer` subsystem from Rust code.
I tried breaking up the code in some smaller patches, hopefully that will
ease the review process a bit.
The major change in this series is the use of a handle to carry ownership
of the callback target. In v1, we used the armed timer to carry ownership
of the callback target. This caused issues when the live timer became the
last owner of the callback target, because the target would be dropped in
timer callback context. That is solved by using a handle instead.
A request from Thomas on v1 was to add a more complete API. While I did add
more features, we are still missing some. In the interest of getting the
patches on list prior to LPC 2024, I am leaving out the following planned
features:
- hrtimer_sleeper, schedule_hrtimeout, hrtimer_nanosleep and friends
- introspection functions:
- try_cancel
- get_remaining
- active
- queued
- callback_running
- hrtimer_forward
- access to timer callback target through timer handle
I plan to add these features in the comming months. Adding the above
features should not cause much churn, and pending positive review, I see no
reason to not pick up this series first.
To make it absolutely clear that I am willing to maintain the code I
submit, I added a mantainer entry in the last patch. Feel free to drop it,
if you want to make other arrangements.
---
Changes from v1:
- use a handle to own the timer callback target
- add ability to for callback to reschedule timer
- improve `impl_has_timer` to allow generics
- add support for stack allocated timers
- add support for scheduling closures
- use `Ktime` for setting expiration
- use `CondVar` instead of `AtomicBool` in examples
- rebase on 6.11
- improve documentation
This series is a dependency for unmerged features of the Rust null block driver
[1], and for rkvms [2].
Link: https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/log/?h=rnull-v6.11-rc2 [1]
Link: https://gitlab.freedesktop.org/lyudess/linux/-/tree/rvkms-wip [2]
---
Andreas Hindborg (13):
rust: hrtimer: introduce hrtimer support
rust: sync: add `Arc::as_ptr`
rust: sync: add `Arc::clone_from_raw`
rust: hrtimer: implement `TimerPointer` for `Arc`
rust: hrtimer: allow timer restart from timer handler
rust: hrtimer: add `UnsafeTimerPointer`
rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>`
rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&mut T>`
rust: hrtimer: add `hrtimer::ScopedTimerPointer`
rust: hrtimer: allow specifying a distinct callback parameter
rust: hrtimer: implement `TimerPointer` for `Pin<Box<T>>`
rust: hrtimer: add `schedule_function` to schedule closures
rust: hrtimer: add maintainer entry
Lyude Paul (1):
rust: time: Add Ktime::from_ns()
MAINTAINERS | 10 +
rust/kernel/hrtimer.rs | 550 +++++++++++++++++++++++++++++++++
rust/kernel/hrtimer/arc.rs | 86 ++++++
rust/kernel/hrtimer/closure.rs | 72 +++++
rust/kernel/hrtimer/pin.rs | 97 ++++++
rust/kernel/hrtimer/pin_mut.rs | 99 ++++++
rust/kernel/hrtimer/tbox.rs | 95 ++++++
rust/kernel/lib.rs | 1 +
rust/kernel/sync/arc.rs | 28 ++
rust/kernel/time.rs | 8 +
10 files changed, 1046 insertions(+)
create mode 100644 rust/kernel/hrtimer.rs
create mode 100644 rust/kernel/hrtimer/arc.rs
create mode 100644 rust/kernel/hrtimer/closure.rs
create mode 100644 rust/kernel/hrtimer/pin.rs
create mode 100644 rust/kernel/hrtimer/pin_mut.rs
create mode 100644 rust/kernel/hrtimer/tbox.rs
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
--
2.46.0
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 01/14] rust: time: Add Ktime::from_ns()
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support Andreas Hindborg
` (14 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Lyude Paul, Andreas Hindborg, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
From: Lyude Paul <lyude@redhat.com>
A simple function to turn the provided value in nanoseconds into a Ktime
value. We allow any type which implements Into<bindings::ktime_t>, which
resolves to Into<i64>.
This is useful for some of the older DRM APIs that never got moved to Ktime
Signed-off-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/time.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index e3bb5e89f88d..1e02eee09f22 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -8,6 +8,8 @@
//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
+use core::convert::Into;
+
/// The number of nanoseconds per millisecond.
pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
@@ -63,6 +65,12 @@ pub fn to_ns(self) -> i64 {
pub fn to_ms(self) -> i64 {
self.divns_constant::<NSEC_PER_MSEC>()
}
+
+ /// Creates a new Ktime from the given duration in nanoseconds
+ #[inline]
+ pub fn from_ns(ns: impl Into<bindings::ktime_t>) -> Self {
+ Self { inner: ns.into() }
+ }
}
/// Returns the number of milliseconds between two ktimes.
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 01/14] rust: time: Add Ktime::from_ns() Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-18 18:13 ` Benno Lossin
2024-09-17 22:27 ` [PATCH v2 03/14] rust: sync: add `Arc::as_ptr` Andreas Hindborg
` (13 subsequent siblings)
15 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
This patch adds support for intrusive use of the hrtimer system. For now,
only one timer can be embedded in a Rust struct.
The hrtimer Rust API is based on the intrusive style pattern introduced by
the Rust workqueue API.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 231 +++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
2 files changed, 232 insertions(+)
create mode 100644 rust/kernel/hrtimer.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
new file mode 100644
index 000000000000..5c92afd8eb2c
--- /dev/null
+++ b/rust/kernel/hrtimer.rs
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Intrusive high resolution timers.
+//!
+//! Allows scheduling timer callbacks without doing allocations at the time of
+//! scheduling. For now, only one timer per type is allowed.
+
+use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
+use core::marker::PhantomData;
+
+/// A timer backed by a C `struct hrtimer`.
+///
+/// # Invariants
+///
+/// * `self.timer` is initialized by `bindings::hrtimer_init`.
+#[repr(transparent)]
+#[pin_data]
+pub struct Timer<U> {
+ #[pin]
+ timer: Opaque<bindings::hrtimer>,
+ _t: PhantomData<U>,
+}
+
+// SAFETY: A `Timer` can be moved to other threads and used/dropped from there.
+unsafe impl<U> Send for Timer<U> {}
+
+// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
+// timer from multiple threads
+unsafe impl<U> Sync for Timer<U> {}
+
+impl<T> Timer<T> {
+ /// Return an initializer for a new timer instance.
+ pub fn new() -> impl PinInit<Self>
+ where
+ T: TimerCallback,
+ {
+ pin_init!( Self {
+ // INVARIANTS: We initialize `timer` with `hrtimer_init` below.
+ timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
+ // SAFETY: By design of `pin_init!`, `place` is a pointer live
+ // allocation. hrtimer_init will initialize `place` and does not
+ // require `place` to be initialized prior to the call.
+ unsafe {
+ bindings::hrtimer_init(
+ place,
+ bindings::CLOCK_MONOTONIC as i32,
+ bindings::hrtimer_mode_HRTIMER_MODE_REL,
+ );
+ }
+
+ // SAFETY: `place` is pointing to a live allocation, so the deref
+ // is safe.
+ let function: *mut Option<_> =
+ unsafe { core::ptr::addr_of_mut!((*place).function) };
+
+ // SAFETY: `function` points to a valid allocation and we have
+ // exclusive access.
+ unsafe { core::ptr::write(function, Some(T::CallbackTarget::run)) };
+ }),
+ _t: PhantomData,
+ })
+ }
+
+ /// Get a pointer to the contained `bindings::hrtimer`.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a live allocation of at least the size of `Self`.
+ unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
+ // SAFETY: The field projection to `timer` does not go out of bounds,
+ // because the caller of this function promises that `ptr` points to an
+ // allocation of at least the size of `Self`.
+ unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
+ }
+}
+
+/// Implemented by pointer types that point to structs that embed a [`Timer`].
+///
+/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
+/// has a field of type `Timer`.
+///
+/// Target must be [`Sync`] because timer callbacks happen in another thread of
+/// execution (hard or soft interrupt context).
+///
+/// Scheduling a timer returns a [`TimerHandle`] that can be used to manipulate
+/// the timer. Note that it is OK to call the schedule function repeatedly, and
+/// that more than one [`TimerHandle`] associated with a `TimerPointer` may
+/// exist. A timer can be manipulated through any of the handles, and a handle
+/// may represent a cancelled timer.
+///
+/// [`Box<T>`]: Box
+/// [`Arc<T>`]: crate::sync::Arc
+/// [`ARef<T>`]: crate::types::ARef
+pub trait TimerPointer: Sync + Sized {
+ /// A handle representing a scheduled timer.
+ ///
+ /// If the timer is armed or if the timer callback is running when the
+ /// handle is dropped, the drop method of `TimerHandle` should not return
+ /// until the timer is unarmed and the callback has completed.
+ ///
+ /// Note: It must be safe to leak the handle.
+ type TimerHandle: TimerHandle;
+
+ /// Schedule the timer after `expires` time units. If the timer was already
+ /// scheduled, it is rescheduled at the new expiry time.
+ fn schedule(self, expires: Ktime) -> Self::TimerHandle;
+}
+
+/// Implemented by [`TimerPointer`] implementers to give the C timer callback a
+/// function to call.
+// This is split from `TimerPointer` to make it easier to specify trait bounds.
+pub trait RawTimerCallback {
+ /// Callback to be called from C when timer fires.
+ ///
+ /// # Safety
+ ///
+ /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
+ /// the `bindings::hrtimer` structure that was used to schedule the timer.
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+}
+
+/// Implemented by structs that can the target of a timer callback.
+pub trait TimerCallback {
+ /// The type that was used for scheduling the timer.
+ type CallbackTarget<'a>: RawTimerCallback;
+
+ /// Called by the timer logic when the timer fires.
+ fn run(this: Self::CallbackTarget<'_>)
+ where
+ Self: Sized;
+}
+
+/// A handle representing a potentially armed timer.
+///
+/// More than one handle representing the same timer might exist.
+///
+/// # Safety
+///
+/// When dropped, the timer represented by this handle must be cancelled, if it
+/// is armed. If the timer handler is running when the handle is dropped, the
+/// drop method must wait for the handler to finish before returning.
+pub unsafe trait TimerHandle {}
+
+/// Implemented by structs that contain timer nodes.
+///
+/// Clients of the timer API would usually safely implement this trait by using
+/// the [`impl_has_timer`] macro.
+///
+/// # Safety
+///
+/// Implementers of this trait must ensure that the implementer has a [`Timer`]
+/// field at the offset specified by `OFFSET` and that all trait methods are
+/// implemented according to their documentation.
+///
+/// [`impl_has_timer`]: crate::impl_has_timer
+pub unsafe trait HasTimer<U> {
+ /// Offset of the [`Timer`] field within `Self`
+ const OFFSET: usize;
+
+ /// Return a pointer to the [`Timer`] within `Self`.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a valid struct of type `Self`.
+ unsafe fn raw_get_timer(ptr: *const Self) -> *const Timer<U> {
+ // SAFETY: By the safety requirement of this trait, the trait
+ // implementor will have a `Timer` field at the specified offset.
+ unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<Timer<U>>() }
+ }
+
+ /// Return a pointer to the struct that is embedding the [`Timer`] pointed
+ /// to by `ptr`.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a [`Timer<U>`] field in a struct of type `Self`.
+ unsafe fn timer_container_of(ptr: *mut Timer<U>) -> *mut Self
+ where
+ Self: Sized,
+ {
+ // SAFETY: By the safety requirement of this function and the `HasTimer`
+ // trait, the following expression will yield a pointer to the `Self`
+ // containing the timer addressed by `ptr`.
+ unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
+ }
+
+ /// Get pointer to embedded `bindings::hrtimer` struct.
+ ///
+ /// # Safety
+ ///
+ /// `self_ptr` must point to a valid `Self`.
+ unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
+ // SAFETY: `self_ptr` is a valid pointer to a `Self`.
+ let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
+
+ // SAFETY: timer_ptr points to an allocation of at least `Timer` size.
+ unsafe { Timer::raw_get(timer_ptr) }
+ }
+}
+
+/// Use to implement the [`HasTimer<T>`] trait.
+///
+/// See [`module`] documentation for an example.
+///
+/// [`module`]: crate::hrtimer
+#[macro_export]
+macro_rules! impl_has_timer {
+ (
+ impl$({$($generics:tt)*})?
+ HasTimer<$timer_type:ty>
+ for $self:ty
+ { self.$field:ident }
+ $($rest:tt)*
+ ) => {
+ // SAFETY: This implementation of `raw_get_timer` only compiles if the
+ // field has the right type.
+ unsafe impl$(<$($generics)*>)? $crate::hrtimer::HasTimer<$timer_type> for $self {
+ const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
+
+ #[inline]
+ unsafe fn raw_get_timer(ptr: *const Self) ->
+ *const $crate::hrtimer::Timer<$timer_type>
+ {
+ // SAFETY: The caller promises that the pointer is not dangling.
+ unsafe {
+ ::core::ptr::addr_of!((*ptr).$field)
+ }
+ }
+ }
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 274bdc1b0a82..55f846c5a849 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -34,6 +34,7 @@
pub mod error;
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
+pub mod hrtimer;
pub mod init;
pub mod ioctl;
#[cfg(CONFIG_KUNIT)]
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 01/14] rust: time: Add Ktime::from_ns() Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-19 14:03 ` Benno Lossin
2024-09-17 22:27 ` [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw` Andreas Hindborg
` (12 subsequent siblings)
15 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add a method to get a pointer to the data contained in an `Arc`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync/arc.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3673496c2363..a57ea3e2b44c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
unsafe { core::ptr::addr_of!((*ptr).data) }
}
+ /// Return a raw pointer to the data in this arc.
+ pub fn as_ptr(&self) -> *const T {
+ let ptr = self.ptr.as_ptr();
+ // SAFETY: As we derive the pointer from a reference above, the pointer
+ // must be valid.
+ unsafe { core::ptr::addr_of!((*ptr).data) }
+ }
+
/// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
///
/// # Safety
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (2 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 03/14] rust: sync: add `Arc::as_ptr` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-18 18:19 ` Benno Lossin
2024-09-17 22:27 ` [PATCH v2 05/14] rust: hrtimer: implement `TimerPointer` for `Arc` Andreas Hindborg
` (11 subsequent siblings)
15 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add a method to clone an arc from a pointer to the data managed by the
`Arc`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index a57ea3e2b44c..2c95712d12a2 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
unsafe { Self::from_inner(ptr) }
}
+ /// Clones an [`Arc`] instance from a pointer to the contained data.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
+ pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
+ // SAFETY: The caller promises that this pointer points to data
+ // contained in an `Arc` that is still valid.
+ let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
+
+ // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
+ // overflow to zero. SAFETY: By the function safety requirement, there
+ // is necessarily a reference to the object, so it is safe to increment
+ // the refcount.
+ unsafe { bindings::refcount_inc(inner.refcount.get()) };
+
+ // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
+ unsafe { Self::from_inner(inner.into()) }
+ }
+
/// Returns an [`ArcBorrow`] from the given [`Arc`].
///
/// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 05/14] rust: hrtimer: implement `TimerPointer` for `Arc`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (3 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler Andreas Hindborg
` (10 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
This patch allows the use of intrusive `hrtimer` fields in structs that are
managed by an `Arc`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 102 ++++++++++++++++++++++++++++++++++++-
rust/kernel/hrtimer/arc.rs | 87 +++++++++++++++++++++++++++++++
2 files changed, 188 insertions(+), 1 deletion(-)
create mode 100644 rust/kernel/hrtimer/arc.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 5c92afd8eb2c..fd1520ba9fba 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -4,6 +4,64 @@
//!
//! Allows scheduling timer callbacks without doing allocations at the time of
//! scheduling. For now, only one timer per type is allowed.
+//!
+//! # Example
+//!
+//! ```
+//! use kernel::{
+//! hrtimer::{Timer, TimerCallback, TimerPointer},
+//! impl_has_timer, new_condvar, new_mutex,
+//! prelude::*,
+//! sync::{Arc, CondVar, Mutex},
+//! time::Ktime,
+//! };
+//!
+//! #[pin_data]
+//! struct ArcIntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
+//!
+//! impl ArcIntrusiveTimer {
+//! fn new() -> impl PinInit<Self, kernel::error::Error> {
+//! try_pin_init!(Self {
+//! timer <- Timer::new(),
+//! flag <- new_mutex!(false),
+//! cond <- new_condvar!(),
+//! })
+//! }
+//! }
+//!
+//! impl TimerCallback for ArcIntrusiveTimer {
+//! type CallbackTarget<'a> = Arc<Self>;
+//!
+//! fn run(this: Self::CallbackTarget<'_>) {
+//! pr_info!("Timer called\n");
+//! *this.flag.lock() = true;
+//! this.cond.notify_all();
+//! }
+//! }
+//!
+//! impl_has_timer! {
+//! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
+//! }
+//!
+//!
+//! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
+//! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
+//! let mut guard = has_timer.flag.lock();
+//!
+//! while !*guard {
+//! has_timer.cond.wait(&mut guard);
+//! }
+//!
+//! pr_info!("Flag raised\n");
+//! # Ok::<(), kernel::error::Error>(())
+//! ```
use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
use core::marker::PhantomData;
@@ -72,6 +130,25 @@ unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
// allocation of at least the size of `Self`.
unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
}
+
+ /// Cancel an initialized and potentially armed timer.
+ ///
+ /// If the timer handler is running, this will block until the handler is
+ /// finished.
+ ///
+ /// # Safety
+ ///
+ /// `self_ptr` must point to a valid `Self`.
+ unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
+ // SAFETY: timer_ptr points to an allocation of at least `Timer` size.
+ let c_timer_ptr = unsafe { Timer::raw_get(self_ptr) };
+
+ // If handler is running, this will wait for handler to finish before
+ // returning.
+ // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
+ // handled on C side.
+ unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
+ }
}
/// Implemented by pointer types that point to structs that embed a [`Timer`].
@@ -139,7 +216,11 @@ fn run(this: Self::CallbackTarget<'_>)
/// When dropped, the timer represented by this handle must be cancelled, if it
/// is armed. If the timer handler is running when the handle is dropped, the
/// drop method must wait for the handler to finish before returning.
-pub unsafe trait TimerHandle {}
+pub unsafe trait TimerHandle {
+ /// Cancel the timer, if it is armed. If the timer handler is running, block
+ /// till the handler has finished.
+ fn cancel(&mut self) -> bool;
+}
/// Implemented by structs that contain timer nodes.
///
@@ -196,6 +277,23 @@ unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
// SAFETY: timer_ptr points to an allocation of at least `Timer` size.
unsafe { Timer::raw_get(timer_ptr) }
}
+
+ /// Schedule the timer contained in the `Self` pointed to by `self_ptr`. If
+ /// it is already scheduled it is removed and inserted.
+ ///
+ /// # Safety
+ ///
+ /// `self_ptr` must point to a valid `Self`.
+ unsafe fn schedule(self_ptr: *const Self, expires: Ktime) {
+ unsafe {
+ bindings::hrtimer_start_range_ns(
+ Self::c_timer_ptr(self_ptr).cast_mut(),
+ expires.to_ns(),
+ 0,
+ bindings::hrtimer_mode_HRTIMER_MODE_REL,
+ );
+ }
+ }
}
/// Use to implement the [`HasTimer<T>`] trait.
@@ -229,3 +327,5 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
}
}
}
+
+mod arc;
diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
new file mode 100644
index 000000000000..80f6c20f95a9
--- /dev/null
+++ b/rust/kernel/hrtimer/arc.rs
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasTimer;
+use super::RawTimerCallback;
+use super::Timer;
+use super::TimerCallback;
+use super::TimerHandle;
+use super::TimerPointer;
+use crate::sync::Arc;
+use crate::time::Ktime;
+
+/// A handle for an `Arc<HasTimer<U>>` returned by a call to
+/// [`TimerPointer::schedule`].
+pub struct ArcTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ pub(crate) inner: Arc<U>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<U> TimerHandle for ArcTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ fn cancel(&mut self) -> bool {
+ let self_ptr = self.inner.as_ptr();
+
+ // SAFETY: As we obtained `self_ptr` from a valid reference above, it
+ // must point to a valid `U`.
+ let timer_ptr = unsafe { <U as HasTimer<U>>::raw_get_timer(self_ptr) };
+
+ // SAFETY: As `timer_ptr` points into `U` and `U` is valid, `timer_ptr`
+ // must point to a valid `Timer` instance.
+ unsafe { Timer::<U>::raw_cancel(timer_ptr) }
+ }
+}
+
+impl<U> Drop for ArcTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ fn drop(&mut self) {
+ self.cancel();
+ }
+}
+
+impl<U> TimerPointer for Arc<U>
+where
+ U: Send + Sync,
+ U: HasTimer<U>,
+ U: for<'a> TimerCallback<CallbackTarget<'a> = Self>,
+{
+ type TimerHandle = ArcTimerHandle<U>;
+
+ fn schedule(self, expires: Ktime) -> ArcTimerHandle<U> {
+ // SAFETY: Since we generate the pointer passed to `schedule` from a
+ // valid reference, it is a valid pointer.
+ unsafe { U::schedule(self.as_ptr(), expires) };
+
+ ArcTimerHandle { inner: self }
+ }
+}
+
+impl<U> RawTimerCallback for Arc<U>
+where
+ U: HasTimer<U>,
+ U: for<'a> TimerCallback<CallbackTarget<'a> = Self>,
+{
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+ // `Timer` is `repr(transparent)`
+ let timer_ptr = ptr.cast::<kernel::hrtimer::Timer<U>>();
+
+ // SAFETY: By C API contract `ptr` is the pointer we passed when
+ // queuing the timer, so it is a `Timer<T>` embedded in a `T`.
+ let data_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+ // SAFETY: `data_ptr` points to the `U` that was used to queue the
+ // timer. This `U` is contained in an `Arc`.
+ let receiver = unsafe { Arc::clone_from_raw(data_ptr) };
+
+ U::run(receiver);
+
+ bindings::hrtimer_restart_HRTIMER_NORESTART
+ }
+}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (4 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 05/14] rust: hrtimer: implement `TimerPointer` for `Arc` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-20 14:25 ` kernel test robot
2024-09-17 22:27 ` [PATCH v2 07/14] rust: hrtimer: add `UnsafeTimerPointer` Andreas Hindborg
` (9 subsequent siblings)
15 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
This patch allows timer handlers to report that they want a timer to be
restarted after the timer handler has finished executing.
Also update the `hrtimer` documentation to showcase the new feature.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 50 ++++++++++++++++++++++++++++++++------
rust/kernel/hrtimer/arc.rs | 4 +--
2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index fd1520ba9fba..d6c3fa89f77e 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -9,7 +9,7 @@
//!
//! ```
//! use kernel::{
-//! hrtimer::{Timer, TimerCallback, TimerPointer},
+//! hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
//! impl_has_timer, new_condvar, new_mutex,
//! prelude::*,
//! sync::{Arc, CondVar, Mutex},
@@ -21,7 +21,7 @@
//! #[pin]
//! timer: Timer<Self>,
//! #[pin]
-//! flag: Mutex<bool>,
+//! flag: Mutex<u64>,
//! #[pin]
//! cond: CondVar,
//! }
@@ -30,7 +30,7 @@
//! fn new() -> impl PinInit<Self, kernel::error::Error> {
//! try_pin_init!(Self {
//! timer <- Timer::new(),
-//! flag <- new_mutex!(false),
+//! flag <- new_mutex!(0),
//! cond <- new_condvar!(),
//! })
//! }
@@ -39,10 +39,18 @@
//! impl TimerCallback for ArcIntrusiveTimer {
//! type CallbackTarget<'a> = Arc<Self>;
//!
-//! fn run(this: Self::CallbackTarget<'_>) {
+//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
-//! *this.flag.lock() = true;
+//! let mut guard = this.flag.lock();
+//! *guard += 1;
//! this.cond.notify_all();
+//! if *guard == 5 {
+//! TimerRestart::NoRestart
+//! }
+//! else {
+//! TimerRestart::Restart
+//!
+//! }
//! }
//! }
//!
@@ -55,11 +63,11 @@
//! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
//! let mut guard = has_timer.flag.lock();
//!
-//! while !*guard {
+//! while *guard != 5 {
//! has_timer.cond.wait(&mut guard);
//! }
//!
-//! pr_info!("Flag raised\n");
+//! pr_info!("Counted to 5\n");
//! # Ok::<(), kernel::error::Error>(())
//! ```
@@ -202,7 +210,7 @@ pub trait TimerCallback {
type CallbackTarget<'a>: RawTimerCallback;
/// Called by the timer logic when the timer fires.
- fn run(this: Self::CallbackTarget<'_>)
+ fn run(this: Self::CallbackTarget<'_>) -> TimerRestart
where
Self: Sized;
}
@@ -296,6 +304,32 @@ unsafe fn schedule(self_ptr: *const Self, expires: Ktime) {
}
}
+/// Restart policy for timers.
+pub enum TimerRestart {
+ /// Timer should not be restarted.
+ NoRestart,
+ /// Timer should be restarted.
+ Restart,
+}
+
+impl From<u32> for TimerRestart {
+ fn from(value: bindings::hrtimer_restart) -> Self {
+ match value {
+ 0 => Self::NoRestart,
+ _ => Self::Restart,
+ }
+ }
+}
+
+impl From<TimerRestart> for bindings::hrtimer_restart {
+ fn from(value: TimerRestart) -> Self {
+ match value {
+ TimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
+ TimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
+ }
+ }
+}
+
/// Use to implement the [`HasTimer<T>`] trait.
///
/// See [`module`] documentation for an example.
diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
index 80f6c20f95a9..fb8a40484add 100644
--- a/rust/kernel/hrtimer/arc.rs
+++ b/rust/kernel/hrtimer/arc.rs
@@ -80,8 +80,6 @@ impl<U> RawTimerCallback for Arc<U>
// timer. This `U` is contained in an `Arc`.
let receiver = unsafe { Arc::clone_from_raw(data_ptr) };
- U::run(receiver);
-
- bindings::hrtimer_restart_HRTIMER_NORESTART
+ U::run(receiver).into()
}
}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 07/14] rust: hrtimer: add `UnsafeTimerPointer`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (5 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 08/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>` Andreas Hindborg
` (8 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add a trait to allow unsafely queuing stack allocated timers.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index d6c3fa89f77e..bb6349f440e2 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -191,6 +191,39 @@ pub trait TimerPointer: Sync + Sized {
fn schedule(self, expires: Ktime) -> Self::TimerHandle;
}
+/// Unsafe version of [`TimerPointer`] for situations where leaking the
+/// `TimerHandle` returned by `schedule` would be unsound. This is the case for
+/// stack allocated timers.
+///
+/// Typical implementers are pinned references such as [`Pin<&T>].
+///
+/// # Safety
+///
+/// Implementers of this trait must ensure that instances of types implementing
+/// [`UnsafeTimerPointer`] outlives any associated [`TimerPointer::TimerHandle`]
+/// instances.
+///
+/// [`Pin<&T>`]: Box
+pub unsafe trait UnsafeTimerPointer: Sync + Sized {
+ /// A handle representing a scheduled timer.
+ ///
+ /// # Safety
+ ///
+ /// If the timer is armed, or if the timer callback is running when the
+ /// handle is dropped, the drop method of `TimerHandle` must not return
+ /// until the timer is unarmed and the callback has completed.
+ type TimerHandle: TimerHandle;
+
+ /// Schedule the timer after `expires` time units. If the timer was already
+ /// scheduled, it is rescheduled at the new expiry time.
+ ///
+ /// # Safety
+ ///
+ /// Caller promises keep the timer structure alive until the timer is dead.
+ /// Caller can ensure this by not leaking the returned `Self::TimerHandle`.
+ unsafe fn schedule(self, expires: Ktime) -> Self::TimerHandle;
+}
+
/// Implemented by [`TimerPointer`] implementers to give the C timer callback a
/// function to call.
// This is split from `TimerPointer` to make it easier to specify trait bounds.
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 08/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (6 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 07/14] rust: hrtimer: add `UnsafeTimerPointer` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 09/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&mut T>` Andreas Hindborg
` (7 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Allow pinned references to structs that contain a `Timer` node to be
scheduled with the `hrtimer` subsystem.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 1 +
rust/kernel/hrtimer/pin.rs | 96 ++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
create mode 100644 rust/kernel/hrtimer/pin.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index bb6349f440e2..25d3702d0d05 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -396,3 +396,4 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
}
mod arc;
+mod pin;
diff --git a/rust/kernel/hrtimer/pin.rs b/rust/kernel/hrtimer/pin.rs
new file mode 100644
index 000000000000..f9ce0498a0d2
--- /dev/null
+++ b/rust/kernel/hrtimer/pin.rs
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasTimer;
+use super::RawTimerCallback;
+use super::Timer;
+use super::TimerCallback;
+use super::TimerHandle;
+use super::UnsafeTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&HasTimer>`. When the handle exists, the timer might be
+/// armed.
+pub struct PinTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ pub(crate) inner: Pin<&'a U>,
+}
+
+// SAFETY: We cancel the timer when the handle is dropped. The implementation of
+// the `cancel` method will block if the timer handler is running.
+unsafe impl<'a, U> TimerHandle for PinTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ fn cancel(&mut self) -> bool {
+ let self_ptr = self.inner.get_ref() as *const U;
+
+ // SAFETY: As we got `self_ptr` from a reference above, it must point to
+ // a valid `U`.
+ let timer_ptr = unsafe { <U as HasTimer<U>>::raw_get_timer(self_ptr) };
+
+ // SAFETY: As `timer_ptr` is derived from a reference, it must point to
+ // a valid and initialized `Timer`.
+ unsafe { Timer::<U>::raw_cancel(timer_ptr) }
+ }
+}
+
+impl<'a, U> Drop for PinTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ fn drop(&mut self) {
+ self.cancel();
+ }
+}
+
+// SAFETY: We capture the lifetime of `Self` when we create a `PinTimerHandle`,
+// so `Self` will outlive the handle.
+unsafe impl<'a, U> UnsafeTimerPointer for Pin<&'a U>
+where
+ U: Send + Sync,
+ U: HasTimer<U>,
+ U: TimerCallback<CallbackTarget<'a> = Self>,
+{
+ type TimerHandle = PinTimerHandle<'a, U>;
+
+ unsafe fn schedule(self, expires: Ktime) -> Self::TimerHandle {
+ use core::ops::Deref;
+
+ // Cast to pointer
+ let self_ptr = self.deref() as *const U;
+
+ // SAFETY: As we derive `self_ptr` from a reference above, it must point
+ // to a valid `U`.
+ unsafe { U::schedule(self_ptr, expires) };
+
+ PinTimerHandle { inner: self }
+ }
+}
+
+impl<'a, U> RawTimerCallback for Pin<&'a U>
+where
+ U: HasTimer<U>,
+ U: TimerCallback<CallbackTarget<'a> = Self>,
+{
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+ // `Timer` is `repr(transparent)`
+ let timer_ptr = ptr as *mut Timer<U>;
+
+ // SAFETY: By the safety requirement of this function, `timer_ptr`
+ // points to a `Timer<U>` contained in an `U`.
+ let receiver_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+ // SAFETY: By the safety requirement of this function, `timer_ptr`
+ // points to a `Timer<U>` contained in an `U`.
+ let receiver_ref = unsafe { &*receiver_ptr };
+
+ // SAFETY: `receiver_ref` only exists as pinned, so it is safe to pin it
+ // here.
+ let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
+
+ U::run(receiver_pin).into()
+ }
+}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 09/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&mut T>`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (7 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 08/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 10/14] rust: hrtimer: add `hrtimer::ScopedTimerPointer` Andreas Hindborg
` (6 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Allow pinned mutable references to structs that contain a `Timer` node to
be scheduled with the `hrtimer` subsystem.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 1 +
rust/kernel/hrtimer/pin_mut.rs | 98 ++++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+)
create mode 100644 rust/kernel/hrtimer/pin_mut.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 25d3702d0d05..09fb674993c2 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -397,3 +397,4 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
mod arc;
mod pin;
+mod pin_mut;
diff --git a/rust/kernel/hrtimer/pin_mut.rs b/rust/kernel/hrtimer/pin_mut.rs
new file mode 100644
index 000000000000..e25c7158ae4f
--- /dev/null
+++ b/rust/kernel/hrtimer/pin_mut.rs
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasTimer;
+use super::RawTimerCallback;
+use super::Timer;
+use super::TimerCallback;
+use super::TimerHandle;
+use super::UnsafeTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&mut HasTimer>`. When the handle exists, the timer might
+/// be armed.
+pub struct PinMutTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ pub(crate) inner: Pin<&'a mut U>,
+}
+
+// SAFETY: We cancel the timer when the handle is dropped. The implementation of
+// the `cancel` method will block if the timer handler is running.
+unsafe impl<'a, U> TimerHandle for PinMutTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ fn cancel(&mut self) -> bool {
+ // SAFETY: We are not moving out of `self` or handing out mutable
+ // references to `self`.
+ let self_ptr = unsafe { self.inner.as_mut().get_unchecked_mut() as *mut U };
+
+ // SAFETY: As we got `self_ptr` from a reference above, it must point to
+ // a valid `U`.
+ let timer_ptr = unsafe { <U as HasTimer<U>>::raw_get_timer(self_ptr) };
+
+ // SAFETY: As `timer_ptr` is derived from a reference, it must point to
+ // a valid and initialized `Timer`.
+ unsafe { Timer::<U>::raw_cancel(timer_ptr) }
+ }
+}
+
+impl<'a, U> Drop for PinMutTimerHandle<'a, U>
+where
+ U: HasTimer<U>,
+{
+ fn drop(&mut self) {
+ self.cancel();
+ }
+}
+
+// SAFETY: We capture the lifetime of `Self` when we create a
+// `PinMutTimerHandle`, so `Self` will outlive the handle.
+unsafe impl<'a, U> UnsafeTimerPointer for Pin<&'a mut U>
+where
+ U: Send + Sync,
+ U: HasTimer<U>,
+ U: TimerCallback<CallbackTarget<'a> = Self>,
+{
+ type TimerHandle = PinMutTimerHandle<'a, U>;
+
+ unsafe fn schedule(self, expires: Ktime) -> Self::TimerHandle {
+ use core::ops::Deref;
+
+ // Cast to pointer
+ let self_ptr = self.deref() as *const U;
+
+ // SAFETY: As we derive `self_ptr` from a reference above, it must point
+ // to a valid `U`.
+ unsafe { U::schedule(self_ptr, expires) };
+
+ PinMutTimerHandle { inner: self }
+ }
+}
+
+impl<'a, U> RawTimerCallback for Pin<&'a mut U>
+where
+ U: HasTimer<U>,
+ U: TimerCallback<CallbackTarget<'a> = Self>,
+{
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+ // `Timer` is `repr(transparent)`
+ let timer_ptr = ptr as *mut Timer<U>;
+
+ // SAFETY: By the safety requirement of this function, `timer_ptr`
+ // points to a `Timer<U>` contained in an `U`.
+ let receiver_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+ // SAFETY: By the safety requirement of this function, `timer_ptr`
+ // points to a `Timer<U>` contained in an `U`.
+ let receiver_ref = unsafe { &mut *receiver_ptr };
+
+ // SAFETY: `receiver_ref` only exists as pinned, so it is safe to pin it
+ // here.
+ let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
+
+ U::run(receiver_pin).into()
+ }
+}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 10/14] rust: hrtimer: add `hrtimer::ScopedTimerPointer`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (8 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 09/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&mut T>` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 11/14] rust: hrtimer: allow specifying a distinct callback parameter Andreas Hindborg
` (5 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add the trait `ScopedTimerPointer` to allow safe use of stack allocated
timers. Safety is achieved by pinning the stack in place while timers are
running.
Implement the trait for all types that implement `UnsafeTimerPointer`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 93 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 09fb674993c2..6254fa584464 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -70,6 +70,66 @@
//! pr_info!("Counted to 5\n");
//! # Ok::<(), kernel::error::Error>(())
//! ```
+//!
+//! Using a stack based timer:
+//! ```
+//! use kernel::{
+//! hrtimer::{Timer, TimerCallback, ScopedTimerPointer, TimerRestart},
+//! impl_has_timer, new_condvar, new_mutex,
+//! prelude::*,
+//! stack_try_pin_init,
+//! sync::{CondVar, Mutex},
+//! time::Ktime,
+//! };
+//!
+//! #[pin_data]
+//! struct IntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
+//!
+//! impl IntrusiveTimer {
+//! fn new() -> impl PinInit<Self, kernel::error::Error> {
+//! try_pin_init!(Self {
+//! timer <- Timer::new(),
+//! flag <- new_mutex!(false),
+//! cond <- new_condvar!(),
+//! })
+//! }
+//! }
+//!
+//! impl TimerCallback for IntrusiveTimer {
+//! type CallbackTarget<'a> = Pin<&'a Self>;
+//!
+//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
+//! pr_info!("Timer called\n");
+//! *this.flag.lock() = true;
+//! this.cond.notify_all();
+//! TimerRestart::NoRestart
+//! }
+//! }
+//!
+//! impl_has_timer! {
+//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
+//! }
+//!
+//!
+//! stack_try_pin_init!( let has_timer =? IntrusiveTimer::new() );
+//! has_timer.as_ref().schedule_scoped(Ktime::from_ns(200_000_000), || {
+//! let mut guard = has_timer.flag.lock();
+//!
+//! while !*guard {
+//! has_timer.cond.wait(&mut guard);
+//! }
+//! });
+//!
+//! pr_info!("Flag raised\n");
+//! # Ok::<(), kernel::error::Error>(())
+//! ```
use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
use core::marker::PhantomData;
@@ -224,6 +284,39 @@ pub unsafe trait UnsafeTimerPointer: Sync + Sized {
unsafe fn schedule(self, expires: Ktime) -> Self::TimerHandle;
}
+/// A trait for stack allocated timers.
+///
+/// # Safety
+///
+/// Implementers must ensure that `schedule_scoped` does not until the timer is
+/// dead and the timer handler is not running.
+pub unsafe trait ScopedTimerPointer {
+ /// Schedule the timer to run after `expires` time units and immediately
+ /// after call `f`. When `f` returns, the timer is cancelled.
+ fn schedule_scoped<T, F>(self, expires: Ktime, f: F) -> T
+ where
+ F: FnOnce() -> T;
+}
+
+// SAFETY: By the safety requirement of `UnsafeTimerPointer`, dropping the
+// handle returned by `UnsafeTimerPointer::schedule` ensures that the timer is
+// killed.
+unsafe impl<U> ScopedTimerPointer for U
+where
+ U: UnsafeTimerPointer,
+{
+ fn schedule_scoped<T, F>(self, expires: Ktime, f: F) -> T
+ where
+ F: FnOnce() -> T,
+ {
+ // SAFETY: We drop the timer handle below before returning.
+ let handle = unsafe { UnsafeTimerPointer::schedule(self, expires) };
+ let t = f();
+ drop(handle);
+ t
+ }
+}
+
/// Implemented by [`TimerPointer`] implementers to give the C timer callback a
/// function to call.
// This is split from `TimerPointer` to make it easier to specify trait bounds.
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 11/14] rust: hrtimer: allow specifying a distinct callback parameter
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (9 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 10/14] rust: hrtimer: add `hrtimer::ScopedTimerPointer` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 12/14] rust: hrtimer: implement `TimerPointer` for `Pin<Box<T>>` Andreas Hindborg
` (4 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
For some pointer types it is helpful to be able to differentiate between
the `TimerPointer` and the type passed to the timer callback.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 7 ++++++-
rust/kernel/hrtimer/arc.rs | 1 +
rust/kernel/hrtimer/pin.rs | 1 +
rust/kernel/hrtimer/pin_mut.rs | 1 +
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 6254fa584464..38160221f93e 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -38,6 +38,7 @@
//!
//! impl TimerCallback for ArcIntrusiveTimer {
//! type CallbackTarget<'a> = Arc<Self>;
+//! type CallbackPointer<'a> = Arc<Self>;
//!
//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
@@ -104,6 +105,7 @@
//!
//! impl TimerCallback for IntrusiveTimer {
//! type CallbackTarget<'a> = Pin<&'a Self>;
+//! type CallbackPointer<'a> = Pin<&'a Self>;
//!
//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
@@ -335,8 +337,11 @@ pub trait TimerCallback {
/// The type that was used for scheduling the timer.
type CallbackTarget<'a>: RawTimerCallback;
+ /// The type passed to the timer callback function.
+ type CallbackPointer<'a>;
+
/// Called by the timer logic when the timer fires.
- fn run(this: Self::CallbackTarget<'_>) -> TimerRestart
+ fn run(this: Self::CallbackPointer<'_>) -> TimerRestart
where
Self: Sized;
}
diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
index fb8a40484add..ff04b0b75bb3 100644
--- a/rust/kernel/hrtimer/arc.rs
+++ b/rust/kernel/hrtimer/arc.rs
@@ -67,6 +67,7 @@ impl<U> RawTimerCallback for Arc<U>
where
U: HasTimer<U>,
U: for<'a> TimerCallback<CallbackTarget<'a> = Self>,
+ U: for<'a> TimerCallback<CallbackPointer<'a> = Self>,
{
unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
// `Timer` is `repr(transparent)`
diff --git a/rust/kernel/hrtimer/pin.rs b/rust/kernel/hrtimer/pin.rs
index f9ce0498a0d2..d34e0885f0f6 100644
--- a/rust/kernel/hrtimer/pin.rs
+++ b/rust/kernel/hrtimer/pin.rs
@@ -74,6 +74,7 @@ impl<'a, U> RawTimerCallback for Pin<&'a U>
where
U: HasTimer<U>,
U: TimerCallback<CallbackTarget<'a> = Self>,
+ U: TimerCallback<CallbackPointer<'a> = Self>,
{
unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
// `Timer` is `repr(transparent)`
diff --git a/rust/kernel/hrtimer/pin_mut.rs b/rust/kernel/hrtimer/pin_mut.rs
index e25c7158ae4f..2589720df233 100644
--- a/rust/kernel/hrtimer/pin_mut.rs
+++ b/rust/kernel/hrtimer/pin_mut.rs
@@ -76,6 +76,7 @@ impl<'a, U> RawTimerCallback for Pin<&'a mut U>
where
U: HasTimer<U>,
U: TimerCallback<CallbackTarget<'a> = Self>,
+ U: TimerCallback<CallbackPointer<'a> = Self>,
{
unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
// `Timer` is `repr(transparent)`
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 12/14] rust: hrtimer: implement `TimerPointer` for `Pin<Box<T>>`
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (10 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 11/14] rust: hrtimer: allow specifying a distinct callback parameter Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 13/14] rust: hrtimer: add `schedule_function` to schedule closures Andreas Hindborg
` (3 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Allow `Pin<Box<T>>` to be the target of a timer callback.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 3 ++
rust/kernel/hrtimer/tbox.rs | 95 +++++++++++++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
create mode 100644 rust/kernel/hrtimer/tbox.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 38160221f93e..1750016b2b22 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -493,6 +493,9 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
}
}
+// `box` is a reserved keyword, so prefix with `t` for timer
+mod tbox;
+
mod arc;
mod pin;
mod pin_mut;
diff --git a/rust/kernel/hrtimer/tbox.rs b/rust/kernel/hrtimer/tbox.rs
new file mode 100644
index 000000000000..089e6ba97801
--- /dev/null
+++ b/rust/kernel/hrtimer/tbox.rs
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasTimer;
+use super::RawTimerCallback;
+use super::Timer;
+use super::TimerCallback;
+use super::TimerHandle;
+use super::TimerPointer;
+use crate::prelude::*;
+use crate::time::Ktime;
+use core::mem::ManuallyDrop;
+
+/// A handle for a `Box<HasTimer<U>>` returned by a call to
+/// [`TimerPointer::schedule`].
+pub struct BoxTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ pub(crate) inner: *mut U,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<U> TimerHandle for BoxTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ fn cancel(&mut self) -> bool {
+ // SAFETY: As we obtained `self.inner` from a valid reference when we
+ // created `self`, it must point to a valid `U`.
+ let timer_ptr = unsafe { <U as HasTimer<U>>::raw_get_timer(self.inner) };
+
+ // SAFETY: As `timer_ptr` points into `U` and `U` is valid, `timer_ptr`
+ // must point to a valid `Timer` instance.
+ unsafe { Timer::<U>::raw_cancel(timer_ptr) }
+ }
+}
+
+impl<U> Drop for BoxTimerHandle<U>
+where
+ U: HasTimer<U>,
+{
+ fn drop(&mut self) {
+ self.cancel();
+ }
+}
+
+impl<U> TimerPointer for Pin<Box<U>>
+where
+ U: Send + Sync,
+ U: HasTimer<U>,
+ U: for<'a> TimerCallback<CallbackTarget<'a> = Pin<Box<U>>>,
+ U: for<'a> TimerCallback<CallbackPointer<'a> = &'a U>,
+{
+ type TimerHandle = BoxTimerHandle<U>;
+
+ fn schedule(self, expires: Ktime) -> Self::TimerHandle {
+ use core::ops::Deref;
+ let self_ptr = self.deref() as *const U;
+
+ // SAFETY: Since we generate the pointer passed to `schedule` from a
+ // valid reference, it is a valid pointer.
+ unsafe { U::schedule(self_ptr, expires) };
+
+ // SAFETY: We will not move out of this box during timer callback (we
+ // pass an immutable reference to the callback).
+ let inner = unsafe { Pin::into_inner_unchecked(self) };
+
+ BoxTimerHandle {
+ inner: Box::into_raw(inner),
+ }
+ }
+}
+
+impl<U> RawTimerCallback for Pin<Box<U>>
+where
+ U: HasTimer<U>,
+ U: for<'a> TimerCallback<CallbackTarget<'a> = Pin<Box<U>>>,
+ U: for<'a> TimerCallback<CallbackPointer<'a> = &'a U>,
+{
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+ // `Timer` is `repr(transparent)`
+ let timer_ptr = ptr.cast::<kernel::hrtimer::Timer<U>>();
+
+ // SAFETY: By C API contract `ptr` is the pointer we passed when
+ // queuing the timer, so it is a `Timer<T>` embedded in a `T`.
+ let data_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+ // SAFETY: We called `Box::into_raw` when we queued the timer.
+ let tbox = ManuallyDrop::new(unsafe { Box::from_raw(data_ptr) });
+
+ use core::ops::Deref;
+ U::run(tbox.deref()).into()
+ }
+}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 13/14] rust: hrtimer: add `schedule_function` to schedule closures
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (11 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 12/14] rust: hrtimer: implement `TimerPointer` for `Pin<Box<T>>` Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 14/14] rust: hrtimer: add maintainer entry Andreas Hindborg
` (2 subsequent siblings)
15 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add a way to quickly schedule functions for execution after a certain
duration has elapsed.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/hrtimer.rs | 49 +++++++++++++++++++++++
rust/kernel/hrtimer/closure.rs | 72 ++++++++++++++++++++++++++++++++++
2 files changed, 121 insertions(+)
create mode 100644 rust/kernel/hrtimer/closure.rs
diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
index 1750016b2b22..2fd3e68368da 100644
--- a/rust/kernel/hrtimer.rs
+++ b/rust/kernel/hrtimer.rs
@@ -132,6 +132,52 @@
//! pr_info!("Flag raised\n");
//! # Ok::<(), kernel::error::Error>(())
//! ```
+//!
+//! Using a helper:
+//! ```
+//! use kernel::{
+//! hrtimer::schedule_function,
+//! impl_has_timer, new_condvar, new_mutex,
+//! prelude::*,
+//! stack_try_pin_init,
+//! sync::{Arc, CondVar, Mutex},
+//! time::Ktime,
+//! };
+//!
+//! #[pin_data]
+//! struct Data {
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
+//!
+//! impl Data {
+//! fn new() -> impl PinInit<Self, kernel::error::Error> {
+//! try_pin_init!(Self {
+//! flag <- new_mutex!(false),
+//! cond <- new_condvar!(),
+//! })
+//! }
+//! }
+//!
+//! let data = Arc::pin_init(Data::new(), GFP_KERNEL)?;
+//! let data2 = data.clone();
+//!
+//! let handle = schedule_function(Ktime::from_ns(200_000_000), move || {
+//! pr_info!("Hello from the future");
+//! *data2.flag.lock() = true;
+//! data2.cond.notify_all();
+//! });
+//!
+//! let mut guard = data.flag.lock();
+//! while !*guard {
+//! data.cond.wait(&mut guard);
+//! }
+//!
+//! pr_info!("Flag raised\n");
+//! # Ok::<(), kernel::error::Error>(())
+//! ```
use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
use core::marker::PhantomData;
@@ -497,5 +543,8 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
mod tbox;
mod arc;
+mod closure;
mod pin;
mod pin_mut;
+
+pub use closure::schedule_function;
diff --git a/rust/kernel/hrtimer/closure.rs b/rust/kernel/hrtimer/closure.rs
new file mode 100644
index 000000000000..96286a12e87a
--- /dev/null
+++ b/rust/kernel/hrtimer/closure.rs
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::{pin_init, tbox::BoxTimerHandle, Timer, TimerCallback, TimerPointer, TimerRestart};
+use crate::{
+ alloc::{flags, Flags},
+ impl_has_timer, new_mutex,
+ prelude::*,
+ sync::Mutex,
+ time::Ktime,
+};
+use macros::pin_data;
+
+#[pin_data]
+pub struct ClosureTimer<T> {
+ #[pin]
+ timer: Timer<ClosureTimer<T>>,
+ #[pin]
+ callback: Mutex<Option<T>>,
+}
+
+impl_has_timer! {
+ impl{T} HasTimer<Self> for ClosureTimer<T> { self.timer }
+}
+
+impl<T> TimerCallback for ClosureTimer<T>
+where
+ T: FnOnce() + 'static,
+{
+ type CallbackTarget<'a> = Pin<Box<ClosureTimer<T>>>;
+ type CallbackPointer<'a> = &'a ClosureTimer<T>;
+
+ fn run(this: Self::CallbackPointer<'_>) -> TimerRestart
+ where
+ Self: Sized,
+ {
+ if let Some(callback) = this.callback.lock().take() {
+ callback();
+ }
+ TimerRestart::NoRestart
+ }
+}
+
+impl<T> ClosureTimer<T>
+where
+ T: FnOnce() + 'static,
+ T: Send,
+ T: Sync,
+{
+ fn new(f: T, flags: Flags) -> Result<Pin<Box<Self>>> {
+ Box::pin_init(
+ pin_init!(
+ Self {
+ timer <- Timer::new(),
+ callback <- new_mutex!(Some(f)),
+ }
+ ),
+ flags,
+ )
+ }
+}
+
+/// Schedule `f` for execution after `expires` time.
+pub fn schedule_function<T>(expires: Ktime, f: T) -> Result<BoxTimerHandle<ClosureTimer<T>>>
+where
+ T: FnOnce() + 'static,
+ T: Send,
+ T: Sync,
+{
+ let timer = ClosureTimer::<T>::new(f, flags::GFP_KERNEL)?;
+ let handle = <Pin<Box<ClosureTimer<T>>> as TimerPointer>::schedule(timer, expires);
+ Ok(handle)
+}
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 14/14] rust: hrtimer: add maintainer entry
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (12 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 13/14] rust: hrtimer: add `schedule_function` to schedule closures Andreas Hindborg
@ 2024-09-17 22:27 ` Andreas Hindborg
2024-10-12 15:19 ` Boqun Feng
2024-09-30 9:36 ` [PATCH v2 00/14] hrtimer Rust API Anna-Maria Behnsen
2024-10-01 12:37 ` Dirk Behme
15 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-17 22:27 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
add Boqun Feng as reviewer.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
MAINTAINERS | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index cc40a9d9b8cd..018847269dd3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10035,6 +10035,16 @@ F: kernel/time/timer_list.c
F: kernel/time/timer_migration.*
F: tools/testing/selftests/timers/
+HIGH-RESOLUTION TIMERS [RUST]
+M: Andreas Hindborg <a.hindborg@kernel.org>
+R: Boqun Feng <boqun.feng@gmail.com>
+L: rust-for-linux@vger.kernel.org
+S: Supported
+W: https://rust-for-linux.com
+B: https://github.com/Rust-for-Linux/linux/issues
+F: rust/kernel/hrtimer.rs
+F: rust/kernel/hrtimer/
+
HIGH-SPEED SCC DRIVER FOR AX.25
L: linux-hams@vger.kernel.org
S: Orphan
--
2.46.0
^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-17 22:27 ` [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support Andreas Hindborg
@ 2024-09-18 18:13 ` Benno Lossin
2024-09-19 5:43 ` Andreas Hindborg
0 siblings, 1 reply; 59+ messages in thread
From: Benno Lossin @ 2024-09-18 18:13 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
rust-for-linux, linux-kernel
On 18.09.24 00:27, Andreas Hindborg wrote:
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/hrtimer.rs | 231 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 2 files changed, 232 insertions(+)
> create mode 100644 rust/kernel/hrtimer.rs
>
> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
> new file mode 100644
> index 000000000000..5c92afd8eb2c
> --- /dev/null
> +++ b/rust/kernel/hrtimer.rs
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Intrusive high resolution timers.
> +//!
> +//! Allows scheduling timer callbacks without doing allocations at the time of
> +//! scheduling. For now, only one timer per type is allowed.
> +
> +use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// A timer backed by a C `struct hrtimer`.
> +///
> +/// # Invariants
> +///
> +/// * `self.timer` is initialized by `bindings::hrtimer_init`.
> +#[repr(transparent)]
> +#[pin_data]
> +pub struct Timer<U> {
> + #[pin]
> + timer: Opaque<bindings::hrtimer>,
> + _t: PhantomData<U>,
> +}
> +
> +// SAFETY: A `Timer` can be moved to other threads and used/dropped from there.
> +unsafe impl<U> Send for Timer<U> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
> +unsafe impl<U> Sync for Timer<U> {}
> +
> +impl<T> Timer<T> {
> + /// Return an initializer for a new timer instance.
> + pub fn new() -> impl PinInit<Self>
> + where
> + T: TimerCallback,
> + {
> + pin_init!( Self {
I would remove the space after the `(`.
Would be great if we had rustfmt support for custom macros.
> + // INVARIANTS: We initialize `timer` with `hrtimer_init` below.
> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> + // SAFETY: By design of `pin_init!`, `place` is a pointer live
> + // allocation. hrtimer_init will initialize `place` and does not
> + // require `place` to be initialized prior to the call.
> + unsafe {
> + bindings::hrtimer_init(
> + place,
> + bindings::CLOCK_MONOTONIC as i32,
> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
> + );
> + }
> +
> + // SAFETY: `place` is pointing to a live allocation, so the deref
> + // is safe.
> + let function: *mut Option<_> =
Do you really need this type hint?
> + unsafe { core::ptr::addr_of_mut!((*place).function) };
> +
> + // SAFETY: `function` points to a valid allocation and we have
> + // exclusive access.
> + unsafe { core::ptr::write(function, Some(T::CallbackTarget::run)) };
> + }),
> + _t: PhantomData,
> + })
> + }
> +
> + /// Get a pointer to the contained `bindings::hrtimer`.
> + ///
> + /// # Safety
> + ///
> + /// `ptr` must point to a live allocation of at least the size of `Self`.
> + unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> + // SAFETY: The field projection to `timer` does not go out of bounds,
> + // because the caller of this function promises that `ptr` points to an
> + // allocation of at least the size of `Self`.
> + unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> + }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`Timer`].
> +///
> +/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
> +/// has a field of type `Timer`.
> +///
> +/// Target must be [`Sync`] because timer callbacks happen in another thread of
> +/// execution (hard or soft interrupt context).
> +///
> +/// Scheduling a timer returns a [`TimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the schedule function repeatedly, and
> +/// that more than one [`TimerHandle`] associated with a `TimerPointer` may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.
> +///
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: crate::sync::Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait TimerPointer: Sync + Sized {
> + /// A handle representing a scheduled timer.
> + ///
> + /// If the timer is armed or if the timer callback is running when the
> + /// handle is dropped, the drop method of `TimerHandle` should not return
> + /// until the timer is unarmed and the callback has completed.
> + ///
> + /// Note: It must be safe to leak the handle.
> + type TimerHandle: TimerHandle;
Why does this need to be an associated type? Couldn't we have a
`TimerHandle<T>` struct? The schedule function below could then return
`TimerHandle<Self>`.
> +
> + /// Schedule the timer after `expires` time units. If the timer was already
> + /// scheduled, it is rescheduled at the new expiry time.
> + fn schedule(self, expires: Ktime) -> Self::TimerHandle;
> +}
> +/// Use to implement the [`HasTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_timer {
> + (
> + impl$({$($generics:tt)*})?
> + HasTimer<$timer_type:ty>
> + for $self:ty
> + { self.$field:ident }
> + $($rest:tt)*
> + ) => {
> + // SAFETY: This implementation of `raw_get_timer` only compiles if the
> + // field has the right type.
> + unsafe impl$(<$($generics)*>)? $crate::hrtimer::HasTimer<$timer_type> for $self {
There is a double space in front of `for`.
> + const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> +
> + #[inline]
> + unsafe fn raw_get_timer(ptr: *const Self) ->
> + *const $crate::hrtimer::Timer<$timer_type>
> + {
> + // SAFETY: The caller promises that the pointer is not dangling.
> + unsafe {
> + ::core::ptr::addr_of!((*ptr).$field)
> + }
> + }
> + }
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 274bdc1b0a82..55f846c5a849 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -34,6 +34,7 @@
> pub mod error;
> #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> pub mod firmware;
> +pub mod hrtimer;
> pub mod init;
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-17 22:27 ` [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw` Andreas Hindborg
@ 2024-09-18 18:19 ` Benno Lossin
2024-09-18 20:12 ` Gary Guo
2024-09-19 5:54 ` Andreas Hindborg
0 siblings, 2 replies; 59+ messages in thread
From: Benno Lossin @ 2024-09-18 18:19 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
rust-for-linux, linux-kernel
On 18.09.24 00:27, Andreas Hindborg wrote:
> Add a method to clone an arc from a pointer to the data managed by the
> `Arc`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index a57ea3e2b44c..2c95712d12a2 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
> unsafe { Self::from_inner(ptr) }
> }
>
> + /// Clones an [`Arc`] instance from a pointer to the contained data.
> + ///
> + /// # Safety
> + ///
> + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
> + // SAFETY: The caller promises that this pointer points to data
> + // contained in an `Arc` that is still valid.
> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
> +
> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
> + // overflow to zero. SAFETY: By the function safety requirement, there
> + // is necessarily a reference to the object, so it is safe to increment
> + // the refcount.
> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
> +
> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
> + unsafe { Self::from_inner(inner.into()) }
The implementation of this function looks a bit strange to me, how about
this?:
// SAFETY: this function has the same safety requirements as `from_raw`.
let arc = unsafe { Self::from_raw(ptr) };
let clone = arc.clone();
// Prevent decrementing the refcount.
mem::forget(arc);
clone
(of course you would need to change the safety requirements of
`clone_from_raw` to point to `from_raw`)
---
Cheers,
Benno
> + }
> +
> /// Returns an [`ArcBorrow`] from the given [`Arc`].
> ///
> /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-18 18:19 ` Benno Lossin
@ 2024-09-18 20:12 ` Gary Guo
2024-09-18 21:09 ` Benno Lossin
2024-09-19 6:00 ` Andreas Hindborg
2024-09-19 5:54 ` Andreas Hindborg
1 sibling, 2 replies; 59+ messages in thread
From: Gary Guo @ 2024-09-18 20:12 UTC (permalink / raw)
To: Benno Lossin
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On Wed, 18 Sep 2024 18:19:20 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
> On 18.09.24 00:27, Andreas Hindborg wrote:
> > Add a method to clone an arc from a pointer to the data managed by the
> > `Arc`.
> >
> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > ---
> > rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index a57ea3e2b44c..2c95712d12a2 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
> > unsafe { Self::from_inner(ptr) }
> > }
> >
> > + /// Clones an [`Arc`] instance from a pointer to the contained data.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
> > + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
> > + // SAFETY: The caller promises that this pointer points to data
> > + // contained in an `Arc` that is still valid.
> > + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
> > +
> > + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
> > + // overflow to zero. SAFETY: By the function safety requirement, there
> > + // is necessarily a reference to the object, so it is safe to increment
> > + // the refcount.
> > + unsafe { bindings::refcount_inc(inner.refcount.get()) };
> > +
> > + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
> > + unsafe { Self::from_inner(inner.into()) }
>
> The implementation of this function looks a bit strange to me, how about
> this?:
>
> // SAFETY: this function has the same safety requirements as `from_raw`.
> let arc = unsafe { Self::from_raw(ptr) };
> let clone = arc.clone();
> // Prevent decrementing the refcount.
> mem::forget(arc);
> clone
>
> (of course you would need to change the safety requirements of
> `clone_from_raw` to point to `from_raw`)
Wouldn't this function simply be
// SAFETY: ...
let borrow = unsafe { ArcBorrow::from_raw(ptr) }
borrow.into()
?
Maybe this function doesn't even need to exist...
Best,
Gary
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-18 20:12 ` Gary Guo
@ 2024-09-18 21:09 ` Benno Lossin
2024-09-19 6:00 ` Andreas Hindborg
1 sibling, 0 replies; 59+ messages in thread
From: Benno Lossin @ 2024-09-18 21:09 UTC (permalink / raw)
To: Gary Guo
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On 18.09.24 22:12, Gary Guo wrote:
> On Wed, 18 Sep 2024 18:19:20 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to clone an arc from a pointer to the data managed by the
>>> `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index a57ea3e2b44c..2c95712d12a2 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
>>> unsafe { Self::from_inner(ptr) }
>>> }
>>>
>>> + /// Clones an [`Arc`] instance from a pointer to the contained data.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
>>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>>> + // SAFETY: The caller promises that this pointer points to data
>>> + // contained in an `Arc` that is still valid.
>>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>>> +
>>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>>> + // overflow to zero. SAFETY: By the function safety requirement, there
>>> + // is necessarily a reference to the object, so it is safe to increment
>>> + // the refcount.
>>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>>> +
>>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>>> + unsafe { Self::from_inner(inner.into()) }
>>
>> The implementation of this function looks a bit strange to me, how about
>> this?:
>>
>> // SAFETY: this function has the same safety requirements as `from_raw`.
>> let arc = unsafe { Self::from_raw(ptr) };
>> let clone = arc.clone();
>> // Prevent decrementing the refcount.
>> mem::forget(arc);
>> clone
>>
>> (of course you would need to change the safety requirements of
>> `clone_from_raw` to point to `from_raw`)
>
> Wouldn't this function simply be
>
> // SAFETY: ...
> let borrow = unsafe { ArcBorrow::from_raw(ptr) }
> borrow.into()
>
> ?
Oh yeah we do have ArcBorrow...
> Maybe this function doesn't even need to exist...
Depends on how often the above has to be written, but I think yeah, it
might not be necessary.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-18 18:13 ` Benno Lossin
@ 2024-09-19 5:43 ` Andreas Hindborg
2024-09-19 14:09 ` Benno Lossin
0 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-19 5:43 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
Hi Benno,
Thanks for the feedback. I will incorporate all the whitespace
suggestions you have =F0=9F=91=8D
"Benno Lossin" <benno.lossin@proton.me> writes:
> On 18.09.24 00:27, Andreas Hindborg wrote:
[...]
>> +
>> +impl<T> Timer<T> {
>> + /// Return an initializer for a new timer instance.
>> + pub fn new() -> impl PinInit<Self>
>> + where
>> + T: TimerCallback,
>> + {
>> + pin_init!( Self {
>
> I would remove the space after the `(`.
> Would be great if we had rustfmt support for custom macros.
Yes, that would be great!
>
>> + // INVARIANTS: We initialize `timer` with `hrtimer_init` be=
low.
>> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim=
er| {
>> + // SAFETY: By design of `pin_init!`, `place` is a point=
er live
>> + // allocation. hrtimer_init will initialize `place` and=
does not
>> + // require `place` to be initialized prior to the call.
>> + unsafe {
>> + bindings::hrtimer_init(
>> + place,
>> + bindings::CLOCK_MONOTONIC as i32,
>> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> + );
>> + }
>> +
>> + // SAFETY: `place` is pointing to a live allocation, so=
the deref
>> + // is safe.
>> + let function: *mut Option<_> =3D
>
> Do you really need this type hint?
Apparently not!
[...]
>> +pub trait TimerPointer: Sync + Sized {
>> + /// A handle representing a scheduled timer.
>> + ///
>> + /// If the timer is armed or if the timer callback is running when =
the
>> + /// handle is dropped, the drop method of `TimerHandle` should not =
return
>> + /// until the timer is unarmed and the callback has completed.
>> + ///
>> + /// Note: It must be safe to leak the handle.
>> + type TimerHandle: TimerHandle;
>
> Why does this need to be an associated type? Couldn't we have a
> `TimerHandle<T>` struct? The schedule function below could then return
> `TimerHandle<Self>`.
At one point, I had some cycles in trait resolution. Moving generics to
associated types solved that issue. Maybe this can be changed to a
generic. But are generics preferred over associated types for some
reason?
Best regards,
Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-18 18:19 ` Benno Lossin
2024-09-18 20:12 ` Gary Guo
@ 2024-09-19 5:54 ` Andreas Hindborg
2024-09-19 6:19 ` Andreas Hindborg
1 sibling, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-19 5:54 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
"Benno Lossin" <benno.lossin@proton.me> writes:
> On 18.09.24 00:27, Andreas Hindborg wrote:
>> Add a method to clone an arc from a pointer to the data managed by the
>> `Arc`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>> index a57ea3e2b44c..2c95712d12a2 100644
>> --- a/rust/kernel/sync/arc.rs
>> +++ b/rust/kernel/sync/arc.rs
>> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
>> unsafe { Self::from_inner(ptr) }
>> }
>>
>> + /// Clones an [`Arc`] instance from a pointer to the contained data.
>> + ///
>> + /// # Safety
>> + ///
>> + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>> + // SAFETY: The caller promises that this pointer points to data
>> + // contained in an `Arc` that is still valid.
>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>> +
>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>> + // overflow to zero. SAFETY: By the function safety requirement, there
>> + // is necessarily a reference to the object, so it is safe to increment
>> + // the refcount.
>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>> +
>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>> + unsafe { Self::from_inner(inner.into()) }
>
> The implementation of this function looks a bit strange to me, how about
> this?:
>
> // SAFETY: this function has the same safety requirements as `from_raw`.
> let arc = unsafe { Self::from_raw(ptr) };
> let clone = arc.clone();
> // Prevent decrementing the refcount.
> mem::forget(arc);
> clone
>
We do not own
a refcount on the Arc. For a short duration you will have a wrong
refcount. If you have two Arcs and the refcount is 1, the ArcInner might
be dropped after the first line of this suggestion, before you do clone,
and then this is not sound.
Best regards,
Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-18 20:12 ` Gary Guo
2024-09-18 21:09 ` Benno Lossin
@ 2024-09-19 6:00 ` Andreas Hindborg
2024-09-19 14:15 ` Benno Lossin
1 sibling, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-19 6:00 UTC (permalink / raw)
To: Gary Guo
Cc: Benno Lossin, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
"Gary Guo" <gary@garyguo.net> writes:
> On Wed, 18 Sep 2024 18:19:20 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
[...]
>> > + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>> > + // SAFETY: The caller promises that this pointer points to data
>> > + // contained in an `Arc` that is still valid.
>> > + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>> > +
>> > + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>> > + // overflow to zero. SAFETY: By the function safety requirement, there
>> > + // is necessarily a reference to the object, so it is safe to increment
>> > + // the refcount.
>> > + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>> > +
>> > + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>> > + unsafe { Self::from_inner(inner.into()) }
>>
>> The implementation of this function looks a bit strange to me, how about
>> this?:
>>
>> // SAFETY: this function has the same safety requirements as `from_raw`.
>> let arc = unsafe { Self::from_raw(ptr) };
>> let clone = arc.clone();
>> // Prevent decrementing the refcount.
>> mem::forget(arc);
>> clone
>>
>> (of course you would need to change the safety requirements of
>> `clone_from_raw` to point to `from_raw`)
>
> Wouldn't this function simply be
>
> // SAFETY: ...
> let borrow = unsafe { ArcBorrow::from_raw(ptr) }
> borrow.into()
>
> ?
>
> Maybe this function doesn't even need to exist...
Maybe that could work. But my use case does not satisfy the safety
requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
called. Perhaps we can update the requirements for that function?
Best regards,
Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-19 5:54 ` Andreas Hindborg
@ 2024-09-19 6:19 ` Andreas Hindborg
2024-09-19 6:41 ` Alice Ryhl
0 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-19 6:19 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to clone an arc from a pointer to the data managed by the
>>> `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index a57ea3e2b44c..2c95712d12a2 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
>>> unsafe { Self::from_inner(ptr) }
>>> }
>>>
>>> + /// Clones an [`Arc`] instance from a pointer to the contained data.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
>>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>>> + // SAFETY: The caller promises that this pointer points to data
>>> + // contained in an `Arc` that is still valid.
>>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>>> +
>>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>>> + // overflow to zero. SAFETY: By the function safety requirement, there
>>> + // is necessarily a reference to the object, so it is safe to increment
>>> + // the refcount.
>>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>>> +
>>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>>> + unsafe { Self::from_inner(inner.into()) }
>>
>> The implementation of this function looks a bit strange to me, how about
>> this?:
>>
>> // SAFETY: this function has the same safety requirements as `from_raw`.
>> let arc = unsafe { Self::from_raw(ptr) };
>> let clone = arc.clone();
>> // Prevent decrementing the refcount.
>> mem::forget(arc);
>> clone
>>
>
> We do not own
> a refcount on the Arc. For a short duration you will have a wrong
> refcount. If you have two Arcs and the refcount is 1, the ArcInner might
> be dropped after the first line of this suggestion, before you do clone,
> and then this is not sound.
Well, disregard that. This is why one should not reply to emails before
coffee in the morning.
Of course, a precondition for calling this function is that the arc
containing the data pointed to by `ptr` is live for the duration. So
what you wrote would work. But I still do not like having two `Arc`s in
existence with the wrong refcount.
BR Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-19 6:19 ` Andreas Hindborg
@ 2024-09-19 6:41 ` Alice Ryhl
0 siblings, 0 replies; 59+ messages in thread
From: Alice Ryhl @ 2024-09-19 6:41 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Benno Lossin, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, rust-for-linux, linux-kernel
On Thu, Sep 19, 2024 at 8:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
> > "Benno Lossin" <benno.lossin@proton.me> writes:
> >
> >> On 18.09.24 00:27, Andreas Hindborg wrote:
> >>> Add a method to clone an arc from a pointer to the data managed by the
> >>> `Arc`.
> >>>
> >>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >>> ---
> >>> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
> >>> 1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> >>> index a57ea3e2b44c..2c95712d12a2 100644
> >>> --- a/rust/kernel/sync/arc.rs
> >>> +++ b/rust/kernel/sync/arc.rs
> >>> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
> >>> unsafe { Self::from_inner(ptr) }
> >>> }
> >>>
> >>> + /// Clones an [`Arc`] instance from a pointer to the contained data.
> >>> + ///
> >>> + /// # Safety
> >>> + ///
> >>> + /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
> >>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
> >>> + // SAFETY: The caller promises that this pointer points to data
> >>> + // contained in an `Arc` that is still valid.
> >>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
> >>> +
> >>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
> >>> + // overflow to zero. SAFETY: By the function safety requirement, there
> >>> + // is necessarily a reference to the object, so it is safe to increment
> >>> + // the refcount.
> >>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
> >>> +
> >>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
> >>> + unsafe { Self::from_inner(inner.into()) }
> >>
> >> The implementation of this function looks a bit strange to me, how about
> >> this?:
> >>
> >> // SAFETY: this function has the same safety requirements as `from_raw`.
> >> let arc = unsafe { Self::from_raw(ptr) };
> >> let clone = arc.clone();
> >> // Prevent decrementing the refcount.
> >> mem::forget(arc);
> >> clone
> >>
> >
> > We do not own
> > a refcount on the Arc. For a short duration you will have a wrong
> > refcount. If you have two Arcs and the refcount is 1, the ArcInner might
> > be dropped after the first line of this suggestion, before you do clone,
> > and then this is not sound.
>
> Well, disregard that. This is why one should not reply to emails before
> coffee in the morning.
>
> Of course, a precondition for calling this function is that the arc
> containing the data pointed to by `ptr` is live for the duration. So
> what you wrote would work. But I still do not like having two `Arc`s in
> existence with the wrong refcount.
Doing it this way has been pretty standard with std for a long time,
until the Arc::increment_strong_count and similar methods were added.
I think it's fine, though I would have used ManuallyDrop instead of
mem::forget.
Of course, in this particular case, using `ArcBorrow` is even better.
Alice
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-17 22:27 ` [PATCH v2 03/14] rust: sync: add `Arc::as_ptr` Andreas Hindborg
@ 2024-09-19 14:03 ` Benno Lossin
2024-09-21 15:58 ` Gary Guo
2024-10-01 4:56 ` Dirk Behme
0 siblings, 2 replies; 59+ messages in thread
From: Benno Lossin @ 2024-09-19 14:03 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
rust-for-linux, linux-kernel
On 18.09.24 00:27, Andreas Hindborg wrote:
> Add a method to get a pointer to the data contained in an `Arc`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync/arc.rs | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 3673496c2363..a57ea3e2b44c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> unsafe { core::ptr::addr_of!((*ptr).data) }
> }
>
> + /// Return a raw pointer to the data in this arc.
> + pub fn as_ptr(&self) -> *const T {
I don't know if we have a convention for this, but shouldn't this be an
associated function? Because if `T` also has an `as_ptr` function, it
will be shadowed by this one.
---
Cheers,
Benno
> + let ptr = self.ptr.as_ptr();
> + // SAFETY: As we derive the pointer from a reference above, the pointer
> + // must be valid.
> + unsafe { core::ptr::addr_of!((*ptr).data) }
> + }
> +
> /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
> ///
> /// # Safety
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-19 5:43 ` Andreas Hindborg
@ 2024-09-19 14:09 ` Benno Lossin
2024-09-23 16:35 ` Andreas Hindborg
0 siblings, 1 reply; 59+ messages in thread
From: Benno Lossin @ 2024-09-19 14:09 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On 19.09.24 07:43, Andreas Hindborg wrote:
> Hi Benno,
>
> Thanks for the feedback. I will incorporate all the whitespace
> suggestions you have =F0=9F=91=8D
There seems to be something wrong with the encoding in this email, is
that an issue on my side or yours?
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
> [...]
>>> +
>>> +impl<T> Timer<T> {
>>> + /// Return an initializer for a new timer instance.
>>> + pub fn new() -> impl PinInit<Self>
>>> + where
>>> + T: TimerCallback,
>>> + {
>>> + pin_init!( Self {
>>
>> I would remove the space after the `(`.
>> Would be great if we had rustfmt support for custom macros.
>
> Yes, that would be great!
>
>>
>>> + // INVARIANTS: We initialize `timer` with `hrtimer_init` be=
> low.
>>> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim=
> er| {
>>> + // SAFETY: By design of `pin_init!`, `place` is a point=
> er live
>>> + // allocation. hrtimer_init will initialize `place` and=
> does not
>>> + // require `place` to be initialized prior to the call.
>>> + unsafe {
>>> + bindings::hrtimer_init(
>>> + place,
>>> + bindings::CLOCK_MONOTONIC as i32,
>>> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
>>> + );
>>> + }
>>> +
>>> + // SAFETY: `place` is pointing to a live allocation, so=
> the deref
>>> + // is safe.
>>> + let function: *mut Option<_> =3D
>>
>> Do you really need this type hint?
>
> Apparently not!
>
> [...]
>>> +pub trait TimerPointer: Sync + Sized {
>>> + /// A handle representing a scheduled timer.
>>> + ///
>>> + /// If the timer is armed or if the timer callback is running when =
> the
>>> + /// handle is dropped, the drop method of `TimerHandle` should not =
> return
>>> + /// until the timer is unarmed and the callback has completed.
>>> + ///
>>> + /// Note: It must be safe to leak the handle.
>>> + type TimerHandle: TimerHandle;
>>
>> Why does this need to be an associated type? Couldn't we have a
>> `TimerHandle<T>` struct? The schedule function below could then return
>> `TimerHandle<Self>`.
>
> At one point, I had some cycles in trait resolution. Moving generics to
> associated types solved that issue. Maybe this can be changed to a
> generic. But are generics preferred over associated types for some
> reason?
The associated type is more complicated IMO, because then every
implementer of the trait needs to create one. If we can avoid that, I
would prefer a generic type.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-19 6:00 ` Andreas Hindborg
@ 2024-09-19 14:15 ` Benno Lossin
2024-09-20 8:25 ` Andreas Hindborg
0 siblings, 1 reply; 59+ messages in thread
From: Benno Lossin @ 2024-09-19 14:15 UTC (permalink / raw)
To: Andreas Hindborg, Gary Guo
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On 19.09.24 08:00, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
>
>> On Wed, 18 Sep 2024 18:19:20 +0000
>> Benno Lossin <benno.lossin@proton.me> wrote:
>>
>>> On 18.09.24 00:27, Andreas Hindborg wrote:
> [...]
>>>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>>>> + // SAFETY: The caller promises that this pointer points to data
>>>> + // contained in an `Arc` that is still valid.
>>>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>>>> +
>>>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>>>> + // overflow to zero. SAFETY: By the function safety requirement, there
>>>> + // is necessarily a reference to the object, so it is safe to increment
>>>> + // the refcount.
>>>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>>>> +
>>>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>>>> + unsafe { Self::from_inner(inner.into()) }
>>>
>>> The implementation of this function looks a bit strange to me, how about
>>> this?:
>>>
>>> // SAFETY: this function has the same safety requirements as `from_raw`.
>>> let arc = unsafe { Self::from_raw(ptr) };
>>> let clone = arc.clone();
>>> // Prevent decrementing the refcount.
>>> mem::forget(arc);
>>> clone
>>>
>>> (of course you would need to change the safety requirements of
>>> `clone_from_raw` to point to `from_raw`)
>>
>> Wouldn't this function simply be
>>
>> // SAFETY: ...
>> let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>> borrow.into()
>>
>> ?
>>
>> Maybe this function doesn't even need to exist...
>
> Maybe that could work. But my use case does not satisfy the safety
> requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
> called. Perhaps we can update the requirements for that function?
If I understood the code correctly, you are essentially doing this:
let arc = Arc::<T>::new(..);
let ptr = Arc::as_ptr(&arc);
let ptr = T::raw_get_timer(ptr);
let ptr = Timer::raw_get(ptr);
// ptr is now used by the timer subsystem to fire the timer
let ptr = ptr.cast::<Timer>();
let ptr = T::timer_container_of(ptr);
let borrow = ArcBorrow::from_raw(ptr);
let arc = borrow.into();
The only thing that we would have to change would be adding
`Arc::as_ptr` as a source in the `ArcBorrow::from_raw` safety
requirements.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`
2024-09-19 14:15 ` Benno Lossin
@ 2024-09-20 8:25 ` Andreas Hindborg
0 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-20 8:25 UTC (permalink / raw)
To: Benno Lossin
Cc: Gary Guo, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
"Benno Lossin" <benno.lossin@proton.me> writes:
> On 19.09.24 08:00, Andreas Hindborg wrote:
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Wed, 18 Sep 2024 18:19:20 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>>> On 18.09.24 00:27, Andreas Hindborg wrote:
>> [...]
>>>>> + pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>>>>> + // SAFETY: The caller promises that this pointer points to data
>>>>> + // contained in an `Arc` that is still valid.
>>>>> + let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>>>>> +
>>>>> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>>>>> + // overflow to zero. SAFETY: By the function safety requirement, there
>>>>> + // is necessarily a reference to the object, so it is safe to increment
>>>>> + // the refcount.
>>>>> + unsafe { bindings::refcount_inc(inner.refcount.get()) };
>>>>> +
>>>>> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>>>>> + unsafe { Self::from_inner(inner.into()) }
>>>>
>>>> The implementation of this function looks a bit strange to me, how about
>>>> this?:
>>>>
>>>> // SAFETY: this function has the same safety requirements as `from_raw`.
>>>> let arc = unsafe { Self::from_raw(ptr) };
>>>> let clone = arc.clone();
>>>> // Prevent decrementing the refcount.
>>>> mem::forget(arc);
>>>> clone
>>>>
>>>> (of course you would need to change the safety requirements of
>>>> `clone_from_raw` to point to `from_raw`)
>>>
>>> Wouldn't this function simply be
>>>
>>> // SAFETY: ...
>>> let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>>> borrow.into()
>>>
>>> ?
>>>
>>> Maybe this function doesn't even need to exist...
>>
>> Maybe that could work. But my use case does not satisfy the safety
>> requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
>> called. Perhaps we can update the requirements for that function?
>
> If I understood the code correctly, you are essentially doing this:
>
> let arc = Arc::<T>::new(..);
> let ptr = Arc::as_ptr(&arc);
> let ptr = T::raw_get_timer(ptr);
> let ptr = Timer::raw_get(ptr);
>
> // ptr is now used by the timer subsystem to fire the timer
>
> let ptr = ptr.cast::<Timer>();
> let ptr = T::timer_container_of(ptr);
> let borrow = ArcBorrow::from_raw(ptr);
> let arc = borrow.into();
>
> The only thing that we would have to change would be adding
> `Arc::as_ptr` as a source in the `ArcBorrow::from_raw` safety
> requirements.
Yes, I think so. I agree that `ArcBorrow` is the right way forward here.
Patch 11 of this series extends `TimerCallback` to be able to have a
different type for the implementer of `RawCallback` and the type passed
to `run`. This is to be able to specify `Pin<Box<_>` as the pointer but
`& _` as the parameter. Those names should probably be changed to
`CallbackTarget` and `CallbackTargetBorrow`, but they could cover this
use of `ArcBorrow` as well.
This use is very similar to `ForeignOwnable`, but a slightly different
use case, since I want to implement for pinned references too. Is there
a way of aligning this and sharing some trait here?
Best regards,
Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler
2024-09-17 22:27 ` [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler Andreas Hindborg
@ 2024-09-20 14:25 ` kernel test robot
0 siblings, 0 replies; 59+ messages in thread
From: kernel test robot @ 2024-09-20 14:25 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: llvm, oe-kbuild-all, Andreas Hindborg, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
Hi Andreas,
kernel test robot noticed the following build errors:
[auto build test ERROR on 98f7e32f20d28ec452afb208f9cffc08448a2652]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Hindborg/rust-time-Add-Ktime-from_ns/20240918-063405
base: 98f7e32f20d28ec452afb208f9cffc08448a2652
patch link: https://lore.kernel.org/r/20240917222739.1298275-7-a.hindborg%40kernel.org
patch subject: [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler
config: riscv-randconfig-002-20240920 (https://download.01.org/0day-ci/archive/20240920/202409202251.Sfvd2aUn-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409202251.Sfvd2aUn-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409202251.Sfvd2aUn-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/riscv/kernel/asm-offsets.c:10:
In file included from include/linux/mm.h:2232:
include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
1 warning generated.
clang diag: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
clang diag: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
clang diag: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
>> error[E0053]: method `from` has an incompatible type for trait
--> rust/kernel/hrtimer.rs:316:20
|
316 | fn from(value: bindings::hrtimer_restart) -> Self {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| expected `u32`, found `i32`
| help: change the parameter type to match the trait: `u32`
|
= note: expected signature `fn(u32) -> TimerRestart`
found signature `fn(i32) -> TimerRestart`
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-19 14:03 ` Benno Lossin
@ 2024-09-21 15:58 ` Gary Guo
2024-09-21 18:17 ` Benno Lossin
2024-09-23 8:14 ` Alice Ryhl
2024-10-01 4:56 ` Dirk Behme
1 sibling, 2 replies; 59+ messages in thread
From: Gary Guo @ 2024-09-21 15:58 UTC (permalink / raw)
To: Benno Lossin
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On Thu, 19 Sep 2024 14:03:50 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
> On 18.09.24 00:27, Andreas Hindborg wrote:
> > Add a method to get a pointer to the data contained in an `Arc`.
> >
> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > ---
> > rust/kernel/sync/arc.rs | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index 3673496c2363..a57ea3e2b44c 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> > unsafe { core::ptr::addr_of!((*ptr).data) }
> > }
> >
> > + /// Return a raw pointer to the data in this arc.
> > + pub fn as_ptr(&self) -> *const T {
>
> I don't know if we have a convention for this, but shouldn't this be an
> associated function? Because if `T` also has an `as_ptr` function, it
> will be shadowed by this one.
The usual Rust convention is usually that if `Deref` is implemented,
then unless there's a good reason to do otherwise, associated function
should be used.
Best,
Gary
>
> ---
> Cheers,
> Benno
>
> > + let ptr = self.ptr.as_ptr();
> > + // SAFETY: As we derive the pointer from a reference above, the pointer
> > + // must be valid.
> > + unsafe { core::ptr::addr_of!((*ptr).data) }
> > + }
> > +
> > /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
> > ///
> > /// # Safety
> > --
> > 2.46.0
> >
> >
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-21 15:58 ` Gary Guo
@ 2024-09-21 18:17 ` Benno Lossin
2024-09-23 8:14 ` Alice Ryhl
1 sibling, 0 replies; 59+ messages in thread
From: Benno Lossin @ 2024-09-21 18:17 UTC (permalink / raw)
To: Gary Guo
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On 21.09.24 17:58, Gary Guo wrote:
> On Thu, 19 Sep 2024 14:03:50 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to get a pointer to the data contained in an `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index 3673496c2363..a57ea3e2b44c 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>>> unsafe { core::ptr::addr_of!((*ptr).data) }
>>> }
>>>
>>> + /// Return a raw pointer to the data in this arc.
>>> + pub fn as_ptr(&self) -> *const T {
>>
>> I don't know if we have a convention for this, but shouldn't this be an
>> associated function? Because if `T` also has an `as_ptr` function, it
>> will be shadowed by this one.
>
> The usual Rust convention is usually that if `Deref` is implemented,
> then unless there's a good reason to do otherwise, associated function
> should be used.
We don't do this for `into_raw` (ie it takes `self`), I will open a
good-first-issue to fix that then.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-21 15:58 ` Gary Guo
2024-09-21 18:17 ` Benno Lossin
@ 2024-09-23 8:14 ` Alice Ryhl
1 sibling, 0 replies; 59+ messages in thread
From: Alice Ryhl @ 2024-09-23 8:14 UTC (permalink / raw)
To: Gary Guo
Cc: Benno Lossin, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner,
Boqun Feng, Björn Roy Baron, rust-for-linux, linux-kernel
On Sat, Sep 21, 2024 at 5:58 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 19 Sep 2024 14:03:50 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
> > On 18.09.24 00:27, Andreas Hindborg wrote:
> > > Add a method to get a pointer to the data contained in an `Arc`.
> > >
> > > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > > ---
> > > rust/kernel/sync/arc.rs | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > index 3673496c2363..a57ea3e2b44c 100644
> > > --- a/rust/kernel/sync/arc.rs
> > > +++ b/rust/kernel/sync/arc.rs
> > > @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> > > unsafe { core::ptr::addr_of!((*ptr).data) }
> > > }
> > >
> > > + /// Return a raw pointer to the data in this arc.
> > > + pub fn as_ptr(&self) -> *const T {
> >
> > I don't know if we have a convention for this, but shouldn't this be an
> > associated function? Because if `T` also has an `as_ptr` function, it
> > will be shadowed by this one.
>
> The usual Rust convention is usually that if `Deref` is implemented,
> then unless there's a good reason to do otherwise, associated function
> should be used.
The reason for this convention is that adding new &self methods on a
smart pointer is a breaking change, but we don't care about breaking
changes, so it doesn't fully apply to us.
Not that we shouldn't follow the convention, but it's not completely
clear to me either.
Alice
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-19 14:09 ` Benno Lossin
@ 2024-09-23 16:35 ` Andreas Hindborg
2024-09-23 16:59 ` Benno Lossin
0 siblings, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-09-23 16:35 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
"Benno Lossin" <benno.lossin@proton.me> writes:
> On 19.09.24 07:43, Andreas Hindborg wrote:
>> Hi Benno,
>>
>> Thanks for the feedback. I will incorporate all the whitespace
>> suggestions you have =F0=9F=91=8D
>
> There seems to be something wrong with the encoding in this email, is
> that an issue on my side or yours?
It seems to be on my end. It also showed up on lore with errors. I will
check my tools.
[...]
>>>> + /// until the timer is unarmed and the callback has completed.
>>>> + ///
>>>> + /// Note: It must be safe to leak the handle.
>>>> + type TimerHandle: TimerHandle;
>>>
>>> Why does this need to be an associated type? Couldn't we have a
>>> `TimerHandle<T>` struct? The schedule function below could then return
>>> `TimerHandle<Self>`.
>>
>> At one point, I had some cycles in trait resolution. Moving generics to
>> associated types solved that issue. Maybe this can be changed to a
>> generic. But are generics preferred over associated types for some
>> reason?
>
> The associated type is more complicated IMO, because then every
> implementer of the trait needs to create one. If we can avoid that, I
> would prefer a generic type.
When you say create, you mean specify? Users would not invent their own
type to put here, they would use the types defined by the `hrtimer`
module in later patches.
BR Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-23 16:35 ` Andreas Hindborg
@ 2024-09-23 16:59 ` Benno Lossin
2024-10-10 12:24 ` Andreas Hindborg
0 siblings, 1 reply; 59+ messages in thread
From: Benno Lossin @ 2024-09-23 16:59 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
On 23.09.24 18:35, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On 19.09.24 07:43, Andreas Hindborg wrote:
>>>>> + /// until the timer is unarmed and the callback has completed.
>>>>> + ///
>>>>> + /// Note: It must be safe to leak the handle.
>>>>> + type TimerHandle: TimerHandle;
>>>>
>>>> Why does this need to be an associated type? Couldn't we have a
>>>> `TimerHandle<T>` struct? The schedule function below could then return
>>>> `TimerHandle<Self>`.
>>>
>>> At one point, I had some cycles in trait resolution. Moving generics to
>>> associated types solved that issue. Maybe this can be changed to a
>>> generic. But are generics preferred over associated types for some
>>> reason?
>>
>> The associated type is more complicated IMO, because then every
>> implementer of the trait needs to create one. If we can avoid that, I
>> would prefer a generic type.
>
> When you say create, you mean specify?
Yes.
> Users would not invent their own type to put here, they would use the
> types defined by the `hrtimer` module in later patches.
I mean the implementers of this trait, not the users of the trait. You
define an `ArcTimerHandle`, `PinTimerHandle` and a `PinMutTimerHandle`
in this series. I think we can avoid that using a single generic struct.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (13 preceding siblings ...)
2024-09-17 22:27 ` [PATCH v2 14/14] rust: hrtimer: add maintainer entry Andreas Hindborg
@ 2024-09-30 9:36 ` Anna-Maria Behnsen
2024-10-04 10:47 ` Andreas Hindborg
2024-10-01 12:37 ` Dirk Behme
15 siblings, 1 reply; 59+ messages in thread
From: Anna-Maria Behnsen @ 2024-09-30 9:36 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Frederic Weisbecker,
Thomas Gleixner
Cc: Andreas Hindborg, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, rust-for-linux, linux-kernel
Hi Andreas,
Andreas Hindborg <a.hindborg@kernel.org> writes:
> Hi!
>
> This series adds support for using the `hrtimer` subsystem from Rust code.
>
> I tried breaking up the code in some smaller patches, hopefully that will
> ease the review process a bit.
>
> The major change in this series is the use of a handle to carry ownership
> of the callback target. In v1, we used the armed timer to carry ownership
> of the callback target. This caused issues when the live timer became the
> last owner of the callback target, because the target would be dropped in
> timer callback context. That is solved by using a handle instead.
>
> A request from Thomas on v1 was to add a more complete API. While I did add
> more features, we are still missing some. In the interest of getting the
> patches on list prior to LPC 2024, I am leaving out the following planned
> features:
>
> - hrtimer_sleeper, schedule_hrtimeout, hrtimer_nanosleep and friends
> - introspection functions:
> - try_cancel
> - get_remaining
> - active
> - queued
> - callback_running
> - hrtimer_forward
> - access to timer callback target through timer handle
Regarding the API: I had a closer look at it after the discussion during
LPC. It's possible to change the API (prevent setting the mode in start
as well), but it is not as straight forward, as it originally seems to
be. So this will take some time to be changed completely.
But what we will do in short term is to create htimer_setup(). This will
do the job of hrtimer_init() but expand it by the argument of the
hrtimer function callback.
This is just an information update for you. So you can proceed right now
with the current API and we keep you in the loop for further changes.
Thanks,
Anna-Maria
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-09-19 14:03 ` Benno Lossin
2024-09-21 15:58 ` Gary Guo
@ 2024-10-01 4:56 ` Dirk Behme
2024-10-01 8:39 ` Benno Lossin
1 sibling, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-01 4:56 UTC (permalink / raw)
To: Benno Lossin, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
rust-for-linux, linux-kernel
On 19.09.2024 16:03, Benno Lossin wrote:
> On 18.09.24 00:27, Andreas Hindborg wrote:
>> Add a method to get a pointer to the data contained in an `Arc`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/sync/arc.rs | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>> index 3673496c2363..a57ea3e2b44c 100644
>> --- a/rust/kernel/sync/arc.rs
>> +++ b/rust/kernel/sync/arc.rs
>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>> unsafe { core::ptr::addr_of!((*ptr).data) }
>> }
>>
>> + /// Return a raw pointer to the data in this arc.
>> + pub fn as_ptr(&self) -> *const T {
>
> I don't know if we have a convention for this, but shouldn't this be an
> associated function? Because if `T` also has an `as_ptr` function, it
> will be shadowed by this one.
Yes. In Fabien's out of tree regmap we have an as_ptr() for Regmap [1]
which operates on &Arc<Regmap> [2]. Once this patch is applied to arc.rs
the compilation fails as then Arc.as_ptr() is used, not the
Regmap.as_ptr() any more [3]. Switching this to something like [4] makes
the compiler happy.
Thanks,
Dirk
P.S.: Just to learn something: For the unmodified, failing case: Is
there a rule when which as_ptr() will be used? Is there an order rule
for the shadowing? Any documentation link?
[1]
https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regmap.rs#L71
[2]
https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regulator/driver.rs#L418
[3]
error[E0308]: mismatched types
--> rust/kernel/regulator/driver.rs:420:33
|
420 | config.cfg.regmap = regmap.as_ptr();
| ^^^^^^^^^^^^^^^ types differ in
mutability
|
= note: expected raw pointer `*mut bindings::regmap`
found raw pointer `*const Regmap`
error: aborting due to 1 previous error
[4]
diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
index ff04b0b75bb39..7c39ab440e1c6 100644
--- a/rust/kernel/hrtimer/arc.rs
+++ b/rust/kernel/hrtimer/arc.rs
@@ -25,7 +25,7 @@ unsafe impl<U> TimerHandle for ArcTimerHandle<U>
U: HasTimer<U>,
{
fn cancel(&mut self) -> bool {
- let self_ptr = self.inner.as_ptr();
+ let self_ptr = Arc::as_ptr(&self.inner);
// SAFETY: As we obtained `self_ptr` from a valid reference
above, it
// must point to a valid `U`.
@@ -57,7 +57,7 @@ impl<U> TimerPointer for Arc<U>
fn schedule(self, expires: Ktime) -> ArcTimerHandle<U> {
// SAFETY: Since we generate the pointer passed to `schedule`
from a
// valid reference, it is a valid pointer.
- unsafe { U::schedule(self.as_ptr(), expires) };
+ unsafe { U::schedule(Arc::as_ptr(&self), expires) };
ArcTimerHandle { inner: self }
}
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 1466d9cd41652..0a314c2f4c5ea 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -259,8 +259,8 @@ pub fn into_raw(self) -> *const T {
}
/// Return a raw pointer to the data in this arc.
- pub fn as_ptr(&self) -> *const T {
- let ptr = self.ptr.as_ptr();
+ pub fn as_ptr(arc: &Self) -> *const T {
+ let ptr = arc.ptr.as_ptr();
// SAFETY: As we derive the pointer from a reference above,
the pointer
// must be valid.
unsafe { core::ptr::addr_of!((*ptr).data) }
^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH v2 03/14] rust: sync: add `Arc::as_ptr`
2024-10-01 4:56 ` Dirk Behme
@ 2024-10-01 8:39 ` Benno Lossin
0 siblings, 0 replies; 59+ messages in thread
From: Benno Lossin @ 2024-10-01 8:39 UTC (permalink / raw)
To: Dirk Behme, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
rust-for-linux, linux-kernel
On 01.10.24 06:56, Dirk Behme wrote:
> On 19.09.2024 16:03, Benno Lossin wrote:
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to get a pointer to the data contained in an `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index 3673496c2363..a57ea3e2b44c 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>>> unsafe { core::ptr::addr_of!((*ptr).data) }
>>> }
>>>
>>> + /// Return a raw pointer to the data in this arc.
>>> + pub fn as_ptr(&self) -> *const T {
>>
>> I don't know if we have a convention for this, but shouldn't this be an
>> associated function? Because if `T` also has an `as_ptr` function, it
>> will be shadowed by this one.
>
> Yes. In Fabien's out of tree regmap we have an as_ptr() for Regmap [1]
> which operates on &Arc<Regmap> [2]. Once this patch is applied to arc.rs
> the compilation fails as then Arc.as_ptr() is used, not the
> Regmap.as_ptr() any more [3]. Switching this to something like [4] makes
> the compiler happy.
Yeah then we should switch to that.
> Thanks,
>
> Dirk
>
> P.S.: Just to learn something: For the unmodified, failing case: Is
> there a rule when which as_ptr() will be used? Is there an order rule
> for the shadowing? Any documentation link?
Yes, there is a page about the method call expression in the reference:
https://doc.rust-lang.org/reference/expressions/method-call-expr.html
---
Cheers,
Benno
> [1]
> https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regmap.rs#L71
>
> [2]
> https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regulator/driver.rs#L418
>
> [3]
>
> error[E0308]: mismatched types
> --> rust/kernel/regulator/driver.rs:420:33
> |
> 420 | config.cfg.regmap = regmap.as_ptr();
> | ^^^^^^^^^^^^^^^ types differ in
> mutability
> |
> = note: expected raw pointer `*mut bindings::regmap`
> found raw pointer `*const Regmap`
>
> error: aborting due to 1 previous error
>
> [4]
>
> diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
> index ff04b0b75bb39..7c39ab440e1c6 100644
> --- a/rust/kernel/hrtimer/arc.rs
> +++ b/rust/kernel/hrtimer/arc.rs
> @@ -25,7 +25,7 @@ unsafe impl<U> TimerHandle for ArcTimerHandle<U>
> U: HasTimer<U>,
> {
> fn cancel(&mut self) -> bool {
> - let self_ptr = self.inner.as_ptr();
> + let self_ptr = Arc::as_ptr(&self.inner);
>
> // SAFETY: As we obtained `self_ptr` from a valid reference
> above, it
> // must point to a valid `U`.
> @@ -57,7 +57,7 @@ impl<U> TimerPointer for Arc<U>
> fn schedule(self, expires: Ktime) -> ArcTimerHandle<U> {
> // SAFETY: Since we generate the pointer passed to `schedule`
> from a
> // valid reference, it is a valid pointer.
> - unsafe { U::schedule(self.as_ptr(), expires) };
> + unsafe { U::schedule(Arc::as_ptr(&self), expires) };
>
> ArcTimerHandle { inner: self }
> }
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 1466d9cd41652..0a314c2f4c5ea 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -259,8 +259,8 @@ pub fn into_raw(self) -> *const T {
> }
>
> /// Return a raw pointer to the data in this arc.
> - pub fn as_ptr(&self) -> *const T {
> - let ptr = self.ptr.as_ptr();
> + pub fn as_ptr(arc: &Self) -> *const T {
> + let ptr = arc.ptr.as_ptr();
> // SAFETY: As we derive the pointer from a reference above,
> the pointer
> // must be valid.
> unsafe { core::ptr::addr_of!((*ptr).data) }
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
` (14 preceding siblings ...)
2024-09-30 9:36 ` [PATCH v2 00/14] hrtimer Rust API Anna-Maria Behnsen
@ 2024-10-01 12:37 ` Dirk Behme
2024-10-01 14:42 ` Boqun Feng
15 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-01 12:37 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On 18.09.2024 00:27, Andreas Hindborg wrote:
> Hi!
>
> This series adds support for using the `hrtimer` subsystem from Rust code.
>
> I tried breaking up the code in some smaller patches, hopefully that will
> ease the review process a bit.
Just fyi, having all 14 patches applied I get [1] on the first (doctest)
Example from hrtimer.rs.
This is from lockdep:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
Having just a quick look I'm not sure what the root cause is. Maybe
mutex in interrupt context? Or a more subtle one?
Best regards
Dirk
[1]
# rust_doctest_kernel_hrtimer_rs_0.location: rust/kernel/hrtimer.rs:10
rust_doctests_kernel: Timer called
=============================
[ BUG: Invalid wait context ]
6.11.0-rc1-arm64 #28 Tainted: G N
-----------------------------
swapper/5/0 is trying to lock:
ffff0004409ab900 (rust/doctests_kernel_generated.rs:1238){+.+.}-{3:3},
at: rust_helper_mutex_lock+0x10/0x18
other info that might help us debug this:
context-{2:2}
no locks held by swapper/5/0.
stack backtrace:
CPU: 5 UID: 0 PID: 0 Comm: swapper/5 Tainted: G N 6.11.0-rc1-arm64 #28
Tainted: [N]=TEST
Hardware name: ARM64 based board (DT)
Call trace:
$x.11+0x98/0xb4
show_stack+0x14/0x1c
$x.3+0x3c/0x94
dump_stack+0x14/0x1c
$x.205+0x538/0x594
$x.179+0xd0/0x18c
__mutex_lock+0xa0/0xa4
mutex_lock_nested+0x20/0x28
rust_helper_mutex_lock+0x10/0x18
_RNvXs_NvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_0NtB4_17ArcIntrusiveTimerNtNtCsclYTRz49wqv_6kernel7hrtimer13TimerCallback3run+0x5c/0xd0
_RNvXs1_NtNtCsclYTRz49wqv_6kernel7hrtimer3arcINtNtNtB9_4sync3arc3ArcNtNvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_017ArcIntrusiveTimerENtB7_16RawTimerCallback3runB1b_+0x20/0x2c
$x.90+0x64/0x70
hrtimer_interrupt+0x1d4/0x2ac
arch_timer_handler_phys+0x34/0x40
$x.62+0x50/0x54
generic_handle_domain_irq+0x28/0x40
$x.154+0x58/0x6c
$x.471+0x10/0x20
el1_interrupt+0x70/0x94
el1h_64_irq_handler+0x14/0x1c
el1h_64_irq+0x64/0x68
arch_local_irq_enable+0x4/0x8
cpuidle_enter+0x34/0x48
$x.37+0x58/0xe4
cpu_startup_entry+0x30/0x34
$x.2+0xf8/0x118
$x.13+0x0/0x4
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Counted to 5
ok 22 rust_doctest_kernel_hrtimer_rs_0
# rust_doctest_kernel_hrtimer_rs_1.location: rust/kernel/hrtimer.rs:137
rust_doctests_kernel: Hello from the future
rust_doctests_kernel: Flag raised
ok 23 rust_doctest_kernel_hrtimer_rs_1
# rust_doctest_kernel_hrtimer_rs_2.location: rust/kernel/hrtimer.rs:76
rust_doctests_kernel: Timer called
rust_doctests_kernel: Flag raised
ok 24 rust_doctest_kernel_hrtimer_rs_2
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-01 12:37 ` Dirk Behme
@ 2024-10-01 14:42 ` Boqun Feng
2024-10-03 8:14 ` Dirk Behme
2024-10-11 14:52 ` Andreas Hindborg
0 siblings, 2 replies; 59+ messages in thread
From: Boqun Feng @ 2024-10-01 14:42 UTC (permalink / raw)
To: Dirk Behme
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> On 18.09.2024 00:27, Andreas Hindborg wrote:
> > Hi!
> >
> > This series adds support for using the `hrtimer` subsystem from Rust code.
> >
> > I tried breaking up the code in some smaller patches, hopefully that will
> > ease the review process a bit.
>
> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> Example from hrtimer.rs.
>
> This is from lockdep:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>
> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> interrupt context? Or a more subtle one?
I think it's calling mutex inside an interrupt context as shown by the
callstack:
] __mutex_lock+0xa0/0xa4
] ...
] hrtimer_interrupt+0x1d4/0x2ac
, it is because:
+//! struct ArcIntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
irq-off is needed for the lock, because otherwise we will hit a self
deadlock due to interrupts:
spin_lock(&a);
> timer interrupt
spin_lock(&a);
Also notice that the IrqDisabled<'_> token can be simply created by
::new(), because irq contexts should guarantee interrupt disabled (i.e.
we don't support nested interrupts*).
[*]: I vaguely remember we still have some driver code for slow devices
that will enable interrupts during an irq handler, but these are going
to be gone, we shouldn't really care about this in Rust code.
Regards,
Boqun
[1]: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>
> Best regards
>
> Dirk
>
> [1]
>
> # rust_doctest_kernel_hrtimer_rs_0.location: rust/kernel/hrtimer.rs:10
> rust_doctests_kernel: Timer called
>
> =============================
> [ BUG: Invalid wait context ]
> 6.11.0-rc1-arm64 #28 Tainted: G N
> -----------------------------
> swapper/5/0 is trying to lock:
> ffff0004409ab900 (rust/doctests_kernel_generated.rs:1238){+.+.}-{3:3}, at:
> rust_helper_mutex_lock+0x10/0x18
> other info that might help us debug this:
> context-{2:2}
> no locks held by swapper/5/0.
> stack backtrace:
> CPU: 5 UID: 0 PID: 0 Comm: swapper/5 Tainted: G N 6.11.0-rc1-arm64 #28
> Tainted: [N]=TEST
> Hardware name: ARM64 based board (DT)
> Call trace:
> $x.11+0x98/0xb4
> show_stack+0x14/0x1c
> $x.3+0x3c/0x94
> dump_stack+0x14/0x1c
> $x.205+0x538/0x594
> $x.179+0xd0/0x18c
> __mutex_lock+0xa0/0xa4
> mutex_lock_nested+0x20/0x28
> rust_helper_mutex_lock+0x10/0x18
>
> _RNvXs_NvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_0NtB4_17ArcIntrusiveTimerNtNtCsclYTRz49wqv_6kernel7hrtimer13TimerCallback3run+0x5c/0xd0
>
> _RNvXs1_NtNtCsclYTRz49wqv_6kernel7hrtimer3arcINtNtNtB9_4sync3arc3ArcNtNvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_017ArcIntrusiveTimerENtB7_16RawTimerCallback3runB1b_+0x20/0x2c
> $x.90+0x64/0x70
> hrtimer_interrupt+0x1d4/0x2ac
> arch_timer_handler_phys+0x34/0x40
> $x.62+0x50/0x54
> generic_handle_domain_irq+0x28/0x40
> $x.154+0x58/0x6c
> $x.471+0x10/0x20
> el1_interrupt+0x70/0x94
> el1h_64_irq_handler+0x14/0x1c
> el1h_64_irq+0x64/0x68
> arch_local_irq_enable+0x4/0x8
> cpuidle_enter+0x34/0x48
> $x.37+0x58/0xe4
> cpu_startup_entry+0x30/0x34
> $x.2+0xf8/0x118
> $x.13+0x0/0x4
> rust_doctests_kernel: Timer called
> rust_doctests_kernel: Timer called
> rust_doctests_kernel: Timer called
> rust_doctests_kernel: Timer called
> rust_doctests_kernel: Counted to 5
> ok 22 rust_doctest_kernel_hrtimer_rs_0
> # rust_doctest_kernel_hrtimer_rs_1.location: rust/kernel/hrtimer.rs:137
> rust_doctests_kernel: Hello from the future
> rust_doctests_kernel: Flag raised
> ok 23 rust_doctest_kernel_hrtimer_rs_1
> # rust_doctest_kernel_hrtimer_rs_2.location: rust/kernel/hrtimer.rs:76
> rust_doctests_kernel: Timer called
> rust_doctests_kernel: Flag raised
> ok 24 rust_doctest_kernel_hrtimer_rs_2
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-01 14:42 ` Boqun Feng
@ 2024-10-03 8:14 ` Dirk Behme
2024-10-03 13:03 ` Boqun Feng
2024-10-11 14:52 ` Andreas Hindborg
1 sibling, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-03 8:14 UTC (permalink / raw)
To: Boqun Feng, Dirk Behme
Cc: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
Am 01.10.24 um 16:42 schrieb Boqun Feng:
> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>> Hi!
>>>
>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>
>>> I tried breaking up the code in some smaller patches, hopefully that will
>>> ease the review process a bit.
>>
>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>> Example from hrtimer.rs.
>>
>> This is from lockdep:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>
>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>> interrupt context? Or a more subtle one?
>
> I think it's calling mutex inside an interrupt context as shown by the
> callstack:
>
> ] __mutex_lock+0xa0/0xa4
> ] ...
> ] hrtimer_interrupt+0x1d4/0x2ac
>
> , it is because:
>
> +//! struct ArcIntrusiveTimer {
> +//! #[pin]
> +//! timer: Timer<Self>,
> +//! #[pin]
> +//! flag: Mutex<bool>,
> +//! #[pin]
> +//! cond: CondVar,
> +//! }
>
> has a Mutex<bool>, which actually should be a SpinLockIrq [1].
Two understanding questions:
1. In the main thread (full example for reference below [2]) where is
the lock released? After the while loop? I.e. is the lock held until
guard reaches 5?
let mut guard = has_timer.flag.lock(); // <= lock taken here?
while *guard != 5 {
has_timer.cond.wait(&mut guard);
} // <= lock
released here?
I wonder what this would mean for the interrupt TimerCallback in case
we would use an irq-off SpinLock instead here?
Or maybe:
2. The only place where the guard is modified (*guard += 1;) is in the
TimerCallback which runs in interrupt context as we learned. With that
writing the guard value can't be interrupted. Couldn't we drop the
whole lock, then?
Best regards
Dirk
[2]
//! #[pin_data]
//! struct ArcIntrusiveTimer {
//! #[pin]
//! timer: Timer<Self>,
//! #[pin]
//! flag: Mutex<u64>,
//! #[pin]
//! cond: CondVar,
//! }
//!
//! impl ArcIntrusiveTimer {
//! fn new() -> impl PinInit<Self, kernel::error::Error> {
//! try_pin_init!(Self {
//! timer <- Timer::new(),
//! flag <- new_mutex!(0),
//! cond <- new_condvar!(),
//! })
//! }
//! }
//!
//! impl TimerCallback for ArcIntrusiveTimer {
//! type CallbackTarget<'a> = Arc<Self>;
//! type CallbackPointer<'a> = Arc<Self>;
//!
//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
//! let mut guard = this.flag.lock();
//! *guard += 1;
//! this.cond.notify_all();
//! if *guard == 5 {
//! TimerRestart::NoRestart
//! }
//! else {
//! TimerRestart::Restart
//!
//! }
//! }
//! }
//!
//! impl_has_timer! {
//! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
//! }
//!
//!
//! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
//! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
//! let mut guard = has_timer.flag.lock();
//!
//! while *guard != 5 {
//! has_timer.cond.wait(&mut guard);
//! }
//!
//! pr_info!("Counted to 5\n");
//! # Ok::<(), kernel::error::Error>(())
> Note that
> irq-off is needed for the lock, because otherwise we will hit a self
> deadlock due to interrupts:
>
> spin_lock(&a);
> > timer interrupt
> spin_lock(&a);
>
> Also notice that the IrqDisabled<'_> token can be simply created by
> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> we don't support nested interrupts*).
>
> [*]: I vaguely remember we still have some driver code for slow devices
> that will enable interrupts during an irq handler, but these are going
> to be gone, we shouldn't really care about this in Rust code.
>
> Regards,
> Boqun
>
> [1]: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>
>
>>
>> Best regards
>>
>> Dirk
>>
>> [1]
>>
>> # rust_doctest_kernel_hrtimer_rs_0.location: rust/kernel/hrtimer.rs:10
>> rust_doctests_kernel: Timer called
>>
>> =============================
>> [ BUG: Invalid wait context ]
>> 6.11.0-rc1-arm64 #28 Tainted: G N
>> -----------------------------
>> swapper/5/0 is trying to lock:
>> ffff0004409ab900 (rust/doctests_kernel_generated.rs:1238){+.+.}-{3:3}, at:
>> rust_helper_mutex_lock+0x10/0x18
>> other info that might help us debug this:
>> context-{2:2}
>> no locks held by swapper/5/0.
>> stack backtrace:
>> CPU: 5 UID: 0 PID: 0 Comm: swapper/5 Tainted: G N 6.11.0-rc1-arm64 #28
>> Tainted: [N]=TEST
>> Hardware name: ARM64 based board (DT)
>> Call trace:
>> $x.11+0x98/0xb4
>> show_stack+0x14/0x1c
>> $x.3+0x3c/0x94
>> dump_stack+0x14/0x1c
>> $x.205+0x538/0x594
>> $x.179+0xd0/0x18c
>> __mutex_lock+0xa0/0xa4
>> mutex_lock_nested+0x20/0x28
>> rust_helper_mutex_lock+0x10/0x18
>>
>> _RNvXs_NvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_0NtB4_17ArcIntrusiveTimerNtNtCsclYTRz49wqv_6kernel7hrtimer13TimerCallback3run+0x5c/0xd0
>>
>> _RNvXs1_NtNtCsclYTRz49wqv_6kernel7hrtimer3arcINtNtNtB9_4sync3arc3ArcNtNvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_017ArcIntrusiveTimerENtB7_16RawTimerCallback3runB1b_+0x20/0x2c
>> $x.90+0x64/0x70
>> hrtimer_interrupt+0x1d4/0x2ac
>> arch_timer_handler_phys+0x34/0x40
>> $x.62+0x50/0x54
>> generic_handle_domain_irq+0x28/0x40
>> $x.154+0x58/0x6c
>> $x.471+0x10/0x20
>> el1_interrupt+0x70/0x94
>> el1h_64_irq_handler+0x14/0x1c
>> el1h_64_irq+0x64/0x68
>> arch_local_irq_enable+0x4/0x8
>> cpuidle_enter+0x34/0x48
>> $x.37+0x58/0xe4
>> cpu_startup_entry+0x30/0x34
>> $x.2+0xf8/0x118
>> $x.13+0x0/0x4
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Counted to 5
>> ok 22 rust_doctest_kernel_hrtimer_rs_0
>> # rust_doctest_kernel_hrtimer_rs_1.location: rust/kernel/hrtimer.rs:137
>> rust_doctests_kernel: Hello from the future
>> rust_doctests_kernel: Flag raised
>> ok 23 rust_doctest_kernel_hrtimer_rs_1
>> # rust_doctest_kernel_hrtimer_rs_2.location: rust/kernel/hrtimer.rs:76
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Flag raised
>> ok 24 rust_doctest_kernel_hrtimer_rs_2
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-03 8:14 ` Dirk Behme
@ 2024-10-03 13:03 ` Boqun Feng
2024-10-03 16:18 ` Dirk Behme
0 siblings, 1 reply; 59+ messages in thread
From: Boqun Feng @ 2024-10-03 13:03 UTC (permalink / raw)
To: Dirk Behme
Cc: Dirk Behme, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
rust-for-linux, linux-kernel
On Thu, Oct 03, 2024 at 10:14:17AM +0200, Dirk Behme wrote:
> Am 01.10.24 um 16:42 schrieb Boqun Feng:
> > On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> > > On 18.09.2024 00:27, Andreas Hindborg wrote:
> > > > Hi!
> > > >
> > > > This series adds support for using the `hrtimer` subsystem from Rust code.
> > > >
> > > > I tried breaking up the code in some smaller patches, hopefully that will
> > > > ease the review process a bit.
> > >
> > > Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> > > Example from hrtimer.rs.
> > >
> > > This is from lockdep:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> > >
> > > Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> > > interrupt context? Or a more subtle one?
> >
> > I think it's calling mutex inside an interrupt context as shown by the
> > callstack:
> >
> > ] __mutex_lock+0xa0/0xa4
> > ] ...
> > ] hrtimer_interrupt+0x1d4/0x2ac
> >
> > , it is because:
> >
> > +//! struct ArcIntrusiveTimer {
> > +//! #[pin]
> > +//! timer: Timer<Self>,
> > +//! #[pin]
> > +//! flag: Mutex<bool>,
> > +//! #[pin]
> > +//! cond: CondVar,
> > +//! }
> >
> > has a Mutex<bool>, which actually should be a SpinLockIrq [1].
>
>
> Two understanding questions:
>
Good questions. ;-)
> 1. In the main thread (full example for reference below [2]) where is the
> lock released? After the while loop? I.e. is the lock held until guard
With the current implementation, there are two places the lock will be
released: 1) inside CondVar::wait() and 2) after `guard` is eventually
drop after the loop.
> reaches 5?
>
> let mut guard = has_timer.flag.lock(); // <= lock taken here?
>
> while *guard != 5 {
> has_timer.cond.wait(&mut guard);
> } // <= lock
> released here?
>
> I wonder what this would mean for the interrupt TimerCallback in case we
> would use an irq-off SpinLock instead here?
>
> Or maybe:
>
> 2. The only place where the guard is modified (*guard += 1;) is in the
> TimerCallback which runs in interrupt context as we learned. With that
> writing the guard value can't be interrupted. Couldn't we drop the whole
> lock, then?
>
No, because the main thread can run on another CPU, so disabling
interrupts (because of the interrupt handlers) doesn't mean exclusive
access to value.
Regards,
Boqun
> Best regards
>
> Dirk
>
>
> [2]
>
> //! #[pin_data]
> //! struct ArcIntrusiveTimer {
> //! #[pin]
> //! timer: Timer<Self>,
> //! #[pin]
> //! flag: Mutex<u64>,
> //! #[pin]
> //! cond: CondVar,
> //! }
> //!
> //! impl ArcIntrusiveTimer {
> //! fn new() -> impl PinInit<Self, kernel::error::Error> {
> //! try_pin_init!(Self {
> //! timer <- Timer::new(),
> //! flag <- new_mutex!(0),
> //! cond <- new_condvar!(),
> //! })
> //! }
> //! }
> //!
> //! impl TimerCallback for ArcIntrusiveTimer {
> //! type CallbackTarget<'a> = Arc<Self>;
> //! type CallbackPointer<'a> = Arc<Self>;
> //!
> //! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
> //! pr_info!("Timer called\n");
> //! let mut guard = this.flag.lock();
> //! *guard += 1;
> //! this.cond.notify_all();
> //! if *guard == 5 {
> //! TimerRestart::NoRestart
> //! }
> //! else {
> //! TimerRestart::Restart
> //!
> //! }
> //! }
> //! }
> //!
> //! impl_has_timer! {
> //! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
> //! }
> //!
> //!
> //! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
> //! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
> //! let mut guard = has_timer.flag.lock();
> //!
> //! while *guard != 5 {
> //! has_timer.cond.wait(&mut guard);
> //! }
> //!
> //! pr_info!("Counted to 5\n");
> //! # Ok::<(), kernel::error::Error>(())
>
>
>
[...]
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-03 13:03 ` Boqun Feng
@ 2024-10-03 16:18 ` Dirk Behme
0 siblings, 0 replies; 59+ messages in thread
From: Dirk Behme @ 2024-10-03 16:18 UTC (permalink / raw)
To: Boqun Feng
Cc: Dirk Behme, Andreas Hindborg, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
rust-for-linux, linux-kernel
Am 03.10.24 um 15:03 schrieb Boqun Feng:
> On Thu, Oct 03, 2024 at 10:14:17AM +0200, Dirk Behme wrote:
>> Am 01.10.24 um 16:42 schrieb Boqun Feng:
>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>> Hi!
>>>>>
>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>
>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>> ease the review process a bit.
>>>>
>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>> Example from hrtimer.rs.
>>>>
>>>> This is from lockdep:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>
>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>> interrupt context? Or a more subtle one?
>>>
>>> I think it's calling mutex inside an interrupt context as shown by the
>>> callstack:
>>>
>>> ] __mutex_lock+0xa0/0xa4
>>> ] ...
>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>
>>> , it is because:
>>>
>>> +//! struct ArcIntrusiveTimer {
>>> +//! #[pin]
>>> +//! timer: Timer<Self>,
>>> +//! #[pin]
>>> +//! flag: Mutex<bool>,
>>> +//! #[pin]
>>> +//! cond: CondVar,
>>> +//! }
>>>
>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1].
>>
>>
>> Two understanding questions:
>>
>
> Good questions. ;-)
:-)
>> 1. In the main thread (full example for reference below [2]) where is the
>> lock released? After the while loop? I.e. is the lock held until guard
>
> With the current implementation, there are two places the lock will be
> released: 1) inside CondVar::wait() and
CondVar::wait() releases *and* reaquires, the lock then? So that
outside of CondVar::wait() but inside the while() loop the lock is
held until the while loop is exit?
Would that lock handling inside CondVar::wait() handle the irq stuff
(irq enable and disable) of SpinLockIrq correctly, then?
> 2) after `guard` is eventually
> drop after the loop.
>
>> reaches 5?
>>
>> let mut guard = has_timer.flag.lock(); // <= lock taken here?
>>
>> while *guard != 5 {
>> has_timer.cond.wait(&mut guard);
>> } // <= lock
>> released here?
>>
>> I wonder what this would mean for the interrupt TimerCallback in case we
>> would use an irq-off SpinLock instead here?
>>
>> Or maybe:
>>
>> 2. The only place where the guard is modified (*guard += 1;) is in the
>> TimerCallback which runs in interrupt context as we learned. With that
>> writing the guard value can't be interrupted. Couldn't we drop the whole
>> lock, then?
>>
>
> No, because the main thread can run on another CPU, so disabling
> interrupts (because of the interrupt handlers) doesn't mean exclusive
> access to value.
Yes. I agree if the main thread would write. But that main thread does
read-only accesses, only? So it reads either the old or the new value,
indepenent on the locking? Only the interrupt handler does
read/modify/write. But thats protected by the interrupt context, already.
Dirk
>> Best regards
>>
>> Dirk
>>
>>
>> [2]
>>
>> //! #[pin_data]
>> //! struct ArcIntrusiveTimer {
>> //! #[pin]
>> //! timer: Timer<Self>,
>> //! #[pin]
>> //! flag: Mutex<u64>,
>> //! #[pin]
>> //! cond: CondVar,
>> //! }
>> //!
>> //! impl ArcIntrusiveTimer {
>> //! fn new() -> impl PinInit<Self, kernel::error::Error> {
>> //! try_pin_init!(Self {
>> //! timer <- Timer::new(),
>> //! flag <- new_mutex!(0),
>> //! cond <- new_condvar!(),
>> //! })
>> //! }
>> //! }
>> //!
>> //! impl TimerCallback for ArcIntrusiveTimer {
>> //! type CallbackTarget<'a> = Arc<Self>;
>> //! type CallbackPointer<'a> = Arc<Self>;
>> //!
>> //! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
>> //! pr_info!("Timer called\n");
>> //! let mut guard = this.flag.lock();
>> //! *guard += 1;
>> //! this.cond.notify_all();
>> //! if *guard == 5 {
>> //! TimerRestart::NoRestart
>> //! }
>> //! else {
>> //! TimerRestart::Restart
>> //!
>> //! }
>> //! }
>> //! }
>> //!
>> //! impl_has_timer! {
>> //! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
>> //! }
>> //!
>> //!
>> //! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
>> //! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
>> //! let mut guard = has_timer.flag.lock();
>> //!
>> //! while *guard != 5 {
>> //! has_timer.cond.wait(&mut guard);
>> //! }
>> //!
>> //! pr_info!("Counted to 5\n");
>> //! # Ok::<(), kernel::error::Error>(())
>>
>>
>>
> [...]
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-09-30 9:36 ` [PATCH v2 00/14] hrtimer Rust API Anna-Maria Behnsen
@ 2024-10-04 10:47 ` Andreas Hindborg
0 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-10-04 10:47 UTC (permalink / raw)
To: Anna-Maria Behnsen
Cc: Miguel Ojeda, Alex Gaynor, Frederic Weisbecker, Thomas Gleixner,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
"Anna-Maria Behnsen" <anna-maria@linutronix.de> writes:
> Hi Andreas,
>
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> Hi!
>>
>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>
>> I tried breaking up the code in some smaller patches, hopefully that will
>> ease the review process a bit.
>>
>> The major change in this series is the use of a handle to carry ownership
>> of the callback target. In v1, we used the armed timer to carry ownership
>> of the callback target. This caused issues when the live timer became the
>> last owner of the callback target, because the target would be dropped in
>> timer callback context. That is solved by using a handle instead.
>>
>> A request from Thomas on v1 was to add a more complete API. While I did add
>> more features, we are still missing some. In the interest of getting the
>> patches on list prior to LPC 2024, I am leaving out the following planned
>> features:
>>
>> - hrtimer_sleeper, schedule_hrtimeout, hrtimer_nanosleep and friends
>> - introspection functions:
>> - try_cancel
>> - get_remaining
>> - active
>> - queued
>> - callback_running
>> - hrtimer_forward
>> - access to timer callback target through timer handle
>
> Regarding the API: I had a closer look at it after the discussion during
> LPC. It's possible to change the API (prevent setting the mode in start
> as well), but it is not as straight forward, as it originally seems to
> be. So this will take some time to be changed completely.
>
> But what we will do in short term is to create htimer_setup(). This will
> do the job of hrtimer_init() but expand it by the argument of the
> hrtimer function callback.
>
> This is just an information update for you. So you can proceed right now
> with the current API and we keep you in the loop for further changes.
Thanks! I think we talked about something similar for v1 as well.
BR Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
2024-09-23 16:59 ` Benno Lossin
@ 2024-10-10 12:24 ` Andreas Hindborg
0 siblings, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-10-10 12:24 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, rust-for-linux, linux-kernel
Benno Lossin <benno.lossin@proton.me> writes:
> On 23.09.24 18:35, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On 19.09.24 07:43, Andreas Hindborg wrote:
>>>>>> + /// until the timer is unarmed and the callback has completed.
>>>>>> + ///
>>>>>> + /// Note: It must be safe to leak the handle.
>>>>>> + type TimerHandle: TimerHandle;
>>>>>
>>>>> Why does this need to be an associated type? Couldn't we have a
>>>>> `TimerHandle<T>` struct? The schedule function below could then return
>>>>> `TimerHandle<Self>`.
>>>>
>>>> At one point, I had some cycles in trait resolution. Moving generics to
>>>> associated types solved that issue. Maybe this can be changed to a
>>>> generic. But are generics preferred over associated types for some
>>>> reason?
>>>
>>> The associated type is more complicated IMO, because then every
>>> implementer of the trait needs to create one. If we can avoid that, I
>>> would prefer a generic type.
>>
>> When you say create, you mean specify?
>
> Yes.
>
>> Users would not invent their own type to put here, they would use the
>> types defined by the `hrtimer` module in later patches.
>
> I mean the implementers of this trait, not the users of the trait. You
> define an `ArcTimerHandle`, `PinTimerHandle` and a `PinMutTimerHandle`
> in this series. I think we can avoid that using a single generic struct.
It is not immediately clear for me how to do this. For `Box` we have:
pub struct BoxTimerHandle<U>
where
U: HasTimer<U>,
{
pub(crate) inner: *mut U,
}
but for `Pin<&U>` we have
pub struct PinTimerHandle<'a, U>
where
U: HasTimer<U>,
{
pub(crate) inner: Pin<&'a U>,
}
How can these be combined to a single generic struct?
Best regards,
Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-01 14:42 ` Boqun Feng
2024-10-03 8:14 ` Dirk Behme
@ 2024-10-11 14:52 ` Andreas Hindborg
2024-10-11 15:43 ` Dirk Behme
1 sibling, 1 reply; 59+ messages in thread
From: Andreas Hindborg @ 2024-10-11 14:52 UTC (permalink / raw)
To: Boqun Feng, Lyude Paul
Cc: Dirk Behme, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
Dirk, thanks for reporting!
Boqun Feng <boqun.feng@gmail.com> writes:
> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>> > Hi!
>> >
>> > This series adds support for using the `hrtimer` subsystem from Rust code.
>> >
>> > I tried breaking up the code in some smaller patches, hopefully that will
>> > ease the review process a bit.
>>
>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>> Example from hrtimer.rs.
>>
>> This is from lockdep:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>
>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>> interrupt context? Or a more subtle one?
>
> I think it's calling mutex inside an interrupt context as shown by the
> callstack:
>
> ] __mutex_lock+0xa0/0xa4
> ] ...
> ] hrtimer_interrupt+0x1d4/0x2ac
>
> , it is because:
>
> +//! struct ArcIntrusiveTimer {
> +//! #[pin]
> +//! timer: Timer<Self>,
> +//! #[pin]
> +//! flag: Mutex<bool>,
> +//! #[pin]
> +//! cond: CondVar,
> +//! }
>
> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> irq-off is needed for the lock, because otherwise we will hit a self
> deadlock due to interrupts:
>
> spin_lock(&a);
> > timer interrupt
> spin_lock(&a);
>
> Also notice that the IrqDisabled<'_> token can be simply created by
> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> we don't support nested interrupts*).
I updated the example based on the work in [1]. I think we need to
update `CondVar::wait` to support waiting with irq disabled. Without
this, when we get back from `bindings::schedule_timeout` in
`CondVar::wait_internal`, interrupts are enabled:
```rust
use kernel::{
hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
irq::IrqDisabled,
prelude::*,
sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
time::Ktime,
};
#[pin_data]
struct ArcIntrusiveTimer {
#[pin]
timer: Timer<Self>,
#[pin]
flag: SpinLockIrq<u64>,
#[pin]
cond: CondVar,
}
impl ArcIntrusiveTimer {
fn new() -> impl PinInit<Self, kernel::error::Error> {
try_pin_init!(Self {
timer <- Timer::new(),
flag <- new_spinlock_irq!(0),
cond <- new_condvar!(),
})
}
}
impl TimerCallback for ArcIntrusiveTimer {
type CallbackTarget<'a> = Arc<Self>;
type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
pr_info!("Timer called\n");
let mut guard = this.flag.lock_with(irq);
*guard += 1;
this.cond.notify_all();
if *guard == 5 {
TimerRestart::NoRestart
}
else {
TimerRestart::Restart
}
}
}
impl_has_timer! {
impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
}
let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
kernel::irq::with_irqs_disabled(|irq| {
let mut guard = has_timer.flag.lock_with(irq);
while *guard != 5 {
pr_info!("Not 5 yet, waiting\n");
has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
}
});
```
I think an update of `CondVar::wait` should be part of the patch set [1].
Best regards,
Andreas
[1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-11 14:52 ` Andreas Hindborg
@ 2024-10-11 15:43 ` Dirk Behme
2024-10-11 23:21 ` Boqun Feng
0 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-11 15:43 UTC (permalink / raw)
To: Andreas Hindborg, Boqun Feng, Lyude Paul
Cc: Dirk Behme, Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
Hi Andreas,
Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>
> Dirk, thanks for reporting!
:)
> Boqun Feng <boqun.feng@gmail.com> writes:
>
>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>> Hi!
>>>>
>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>
>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>> ease the review process a bit.
>>>
>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>> Example from hrtimer.rs.
>>>
>>> This is from lockdep:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>
>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>> interrupt context? Or a more subtle one?
>>
>> I think it's calling mutex inside an interrupt context as shown by the
>> callstack:
>>
>> ] __mutex_lock+0xa0/0xa4
>> ] ...
>> ] hrtimer_interrupt+0x1d4/0x2ac
>>
>> , it is because:
>>
>> +//! struct ArcIntrusiveTimer {
>> +//! #[pin]
>> +//! timer: Timer<Self>,
>> +//! #[pin]
>> +//! flag: Mutex<bool>,
>> +//! #[pin]
>> +//! cond: CondVar,
>> +//! }
>>
>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>> irq-off is needed for the lock, because otherwise we will hit a self
>> deadlock due to interrupts:
>>
>> spin_lock(&a);
>> > timer interrupt
>> spin_lock(&a);
>>
>> Also notice that the IrqDisabled<'_> token can be simply created by
>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>> we don't support nested interrupts*).
>
> I updated the example based on the work in [1]. I think we need to
> update `CondVar::wait` to support waiting with irq disabled.
Yes, I agree. This answers one of the open questions I had in the
discussion with Boqun :)
What do you think regarding the other open question: In this *special*
case here, what do you think to go *without* any lock? I mean the
'while *guard != 5' loop in the main thread is read only regarding
guard. So it doesn't matter if it *reads* the old or the new value.
And the read/modify/write of guard in the callback is done with
interrupts disabled anyhow as it runs in interrupt context. And with
this can't be interrupted (excluding nested interrupts). So this
modification of guard doesn't need to be protected from being
interrupted by a lock if there is no modifcation of guard "outside"
the interupt locked context.
What do you think?
Thanks
Dirk
> Without
> this, when we get back from `bindings::schedule_timeout` in
> `CondVar::wait_internal`, interrupts are enabled:
>
> ```rust
> use kernel::{
> hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
> impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
> irq::IrqDisabled,
> prelude::*,
> sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
> time::Ktime,
> };
>
> #[pin_data]
> struct ArcIntrusiveTimer {
> #[pin]
> timer: Timer<Self>,
> #[pin]
> flag: SpinLockIrq<u64>,
> #[pin]
> cond: CondVar,
> }
>
> impl ArcIntrusiveTimer {
> fn new() -> impl PinInit<Self, kernel::error::Error> {
> try_pin_init!(Self {
> timer <- Timer::new(),
> flag <- new_spinlock_irq!(0),
> cond <- new_condvar!(),
> })
> }
> }
>
> impl TimerCallback for ArcIntrusiveTimer {
> type CallbackTarget<'a> = Arc<Self>;
> type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
>
> fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
> pr_info!("Timer called\n");
> let mut guard = this.flag.lock_with(irq);
> *guard += 1;
> this.cond.notify_all();
> if *guard == 5 {
> TimerRestart::NoRestart
> }
> else {
> TimerRestart::Restart
>
> }
> }
> }
>
> impl_has_timer! {
> impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
> }
>
>
> let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
> let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
>
> kernel::irq::with_irqs_disabled(|irq| {
> let mut guard = has_timer.flag.lock_with(irq);
>
> while *guard != 5 {
> pr_info!("Not 5 yet, waiting\n");
> has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
> }
> });
> ```
>
> I think an update of `CondVar::wait` should be part of the patch set [1].
>
>
> Best regards,
> Andreas
>
>
> [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-11 15:43 ` Dirk Behme
@ 2024-10-11 23:21 ` Boqun Feng
2024-10-12 5:19 ` Dirk Behme
0 siblings, 1 reply; 59+ messages in thread
From: Boqun Feng @ 2024-10-11 23:21 UTC (permalink / raw)
To: Dirk Behme
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> Hi Andreas,
>
> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> >
> > Dirk, thanks for reporting!
>
> :)
>
> > Boqun Feng <boqun.feng@gmail.com> writes:
> >
> > > On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> > > > On 18.09.2024 00:27, Andreas Hindborg wrote:
> > > > > Hi!
> > > > >
> > > > > This series adds support for using the `hrtimer` subsystem from Rust code.
> > > > >
> > > > > I tried breaking up the code in some smaller patches, hopefully that will
> > > > > ease the review process a bit.
> > > >
> > > > Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> > > > Example from hrtimer.rs.
> > > >
> > > > This is from lockdep:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> > > >
> > > > Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> > > > interrupt context? Or a more subtle one?
> > >
> > > I think it's calling mutex inside an interrupt context as shown by the
> > > callstack:
> > >
> > > ] __mutex_lock+0xa0/0xa4
> > > ] ...
> > > ] hrtimer_interrupt+0x1d4/0x2ac
> > >
> > > , it is because:
> > >
> > > +//! struct ArcIntrusiveTimer {
> > > +//! #[pin]
> > > +//! timer: Timer<Self>,
> > > +//! #[pin]
> > > +//! flag: Mutex<bool>,
> > > +//! #[pin]
> > > +//! cond: CondVar,
> > > +//! }
> > >
> > > has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> > > irq-off is needed for the lock, because otherwise we will hit a self
> > > deadlock due to interrupts:
> > >
> > > spin_lock(&a);
> > > > timer interrupt
> > > spin_lock(&a);
> > >
> > > Also notice that the IrqDisabled<'_> token can be simply created by
> > > ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> > > we don't support nested interrupts*).
> >
> > I updated the example based on the work in [1]. I think we need to
> > update `CondVar::wait` to support waiting with irq disabled.
>
> Yes, I agree. This answers one of the open questions I had in the discussion
> with Boqun :)
>
> What do you think regarding the other open question: In this *special* case
> here, what do you think to go *without* any lock? I mean the 'while *guard
> != 5' loop in the main thread is read only regarding guard. So it doesn't
> matter if it *reads* the old or the new value. And the read/modify/write of
> guard in the callback is done with interrupts disabled anyhow as it runs in
> interrupt context. And with this can't be interrupted (excluding nested
> interrupts). So this modification of guard doesn't need to be protected from
> being interrupted by a lock if there is no modifcation of guard "outside"
> the interupt locked context.
>
> What do you think?
>
Reading while there is another CPU is writing is data-race, which is UB.
Regards,
Boqun
> Thanks
>
> Dirk
>
>
> > Without
> > this, when we get back from `bindings::schedule_timeout` in
> > `CondVar::wait_internal`, interrupts are enabled:
> >
> > ```rust
> > use kernel::{
> > hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
> > impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
> > irq::IrqDisabled,
> > prelude::*,
> > sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
> > time::Ktime,
> > };
> >
> > #[pin_data]
> > struct ArcIntrusiveTimer {
> > #[pin]
> > timer: Timer<Self>,
> > #[pin]
> > flag: SpinLockIrq<u64>,
> > #[pin]
> > cond: CondVar,
> > }
> >
> > impl ArcIntrusiveTimer {
> > fn new() -> impl PinInit<Self, kernel::error::Error> {
> > try_pin_init!(Self {
> > timer <- Timer::new(),
> > flag <- new_spinlock_irq!(0),
> > cond <- new_condvar!(),
> > })
> > }
> > }
> >
> > impl TimerCallback for ArcIntrusiveTimer {
> > type CallbackTarget<'a> = Arc<Self>;
> > type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
> >
> > fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
> > pr_info!("Timer called\n");
> > let mut guard = this.flag.lock_with(irq);
> > *guard += 1;
> > this.cond.notify_all();
> > if *guard == 5 {
> > TimerRestart::NoRestart
> > }
> > else {
> > TimerRestart::Restart
> >
> > }
> > }
> > }
> >
> > impl_has_timer! {
> > impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
> > }
> >
> >
> > let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
> > let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
> >
> > kernel::irq::with_irqs_disabled(|irq| {
> > let mut guard = has_timer.flag.lock_with(irq);
> >
> > while *guard != 5 {
> > pr_info!("Not 5 yet, waiting\n");
> > has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
> > }
> > });
> > ```
> >
> > I think an update of `CondVar::wait` should be part of the patch set [1].
> >
> >
> > Best regards,
> > Andreas
> >
> >
> > [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
> >
> >
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-11 23:21 ` Boqun Feng
@ 2024-10-12 5:19 ` Dirk Behme
2024-10-12 7:41 ` Boqun Feng
0 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-12 5:19 UTC (permalink / raw)
To: Boqun Feng
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On 12.10.24 01:21, Boqun Feng wrote:
> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>> Hi Andreas,
>>
>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>
>>> Dirk, thanks for reporting!
>>
>> :)
>>
>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>
>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>> Hi!
>>>>>>
>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>
>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>> ease the review process a bit.
>>>>>
>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>> Example from hrtimer.rs.
>>>>>
>>>>> This is from lockdep:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>
>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>> interrupt context? Or a more subtle one?
>>>>
>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>> callstack:
>>>>
>>>> ] __mutex_lock+0xa0/0xa4
>>>> ] ...
>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>
>>>> , it is because:
>>>>
>>>> +//! struct ArcIntrusiveTimer {
>>>> +//! #[pin]
>>>> +//! timer: Timer<Self>,
>>>> +//! #[pin]
>>>> +//! flag: Mutex<bool>,
>>>> +//! #[pin]
>>>> +//! cond: CondVar,
>>>> +//! }
>>>>
>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>> deadlock due to interrupts:
>>>>
>>>> spin_lock(&a);
>>>> > timer interrupt
>>>> spin_lock(&a);
>>>>
>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>> we don't support nested interrupts*).
>>>
>>> I updated the example based on the work in [1]. I think we need to
>>> update `CondVar::wait` to support waiting with irq disabled.
>>
>> Yes, I agree. This answers one of the open questions I had in the discussion
>> with Boqun :)
>>
>> What do you think regarding the other open question: In this *special* case
>> here, what do you think to go *without* any lock? I mean the 'while *guard
>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>> matter if it *reads* the old or the new value. And the read/modify/write of
>> guard in the callback is done with interrupts disabled anyhow as it runs in
>> interrupt context. And with this can't be interrupted (excluding nested
>> interrupts). So this modification of guard doesn't need to be protected from
>> being interrupted by a lock if there is no modifcation of guard "outside"
>> the interupt locked context.
>>
>> What do you think?
>>
>
> Reading while there is another CPU is writing is data-race, which is UB.
Could you help to understand where exactly you see UB in Andreas'
'while *guard != 5' loop in case no locking is used? As mentioned I'm
under the impression that it doesn't matter if the old or new guard
value is read in this special case.
Best regards
Dirk
> Regards,
> Boqun
>
>> Thanks
>>
>> Dirk
>>
>>
>>> Without
>>> this, when we get back from `bindings::schedule_timeout` in
>>> `CondVar::wait_internal`, interrupts are enabled:
>>>
>>> ```rust
>>> use kernel::{
>>> hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
>>> impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
>>> irq::IrqDisabled,
>>> prelude::*,
>>> sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
>>> time::Ktime,
>>> };
>>>
>>> #[pin_data]
>>> struct ArcIntrusiveTimer {
>>> #[pin]
>>> timer: Timer<Self>,
>>> #[pin]
>>> flag: SpinLockIrq<u64>,
>>> #[pin]
>>> cond: CondVar,
>>> }
>>>
>>> impl ArcIntrusiveTimer {
>>> fn new() -> impl PinInit<Self, kernel::error::Error> {
>>> try_pin_init!(Self {
>>> timer <- Timer::new(),
>>> flag <- new_spinlock_irq!(0),
>>> cond <- new_condvar!(),
>>> })
>>> }
>>> }
>>>
>>> impl TimerCallback for ArcIntrusiveTimer {
>>> type CallbackTarget<'a> = Arc<Self>;
>>> type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
>>>
>>> fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
>>> pr_info!("Timer called\n");
>>> let mut guard = this.flag.lock_with(irq);
>>> *guard += 1;
>>> this.cond.notify_all();
>>> if *guard == 5 {
>>> TimerRestart::NoRestart
>>> }
>>> else {
>>> TimerRestart::Restart
>>>
>>> }
>>> }
>>> }
>>>
>>> impl_has_timer! {
>>> impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
>>> }
>>>
>>>
>>> let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
>>> let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
>>>
>>> kernel::irq::with_irqs_disabled(|irq| {
>>> let mut guard = has_timer.flag.lock_with(irq);
>>>
>>> while *guard != 5 {
>>> pr_info!("Not 5 yet, waiting\n");
>>> has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
>>> }
>>> });
>>> ```
>>>
>>> I think an update of `CondVar::wait` should be part of the patch set [1].
>>>
>>>
>>> Best regards,
>>> Andreas
>>>
>>>
>>> [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>>>
>>>
>>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-12 5:19 ` Dirk Behme
@ 2024-10-12 7:41 ` Boqun Feng
2024-10-12 7:50 ` Dirk Behme
0 siblings, 1 reply; 59+ messages in thread
From: Boqun Feng @ 2024-10-12 7:41 UTC (permalink / raw)
To: Dirk Behme
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
> On 12.10.24 01:21, Boqun Feng wrote:
> > On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> > > Hi Andreas,
> > >
> > > Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> > > >
> > > > Dirk, thanks for reporting!
> > >
> > > :)
> > >
> > > > Boqun Feng <boqun.feng@gmail.com> writes:
> > > >
> > > > > On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> > > > > > On 18.09.2024 00:27, Andreas Hindborg wrote:
> > > > > > > Hi!
> > > > > > >
> > > > > > > This series adds support for using the `hrtimer` subsystem from Rust code.
> > > > > > >
> > > > > > > I tried breaking up the code in some smaller patches, hopefully that will
> > > > > > > ease the review process a bit.
> > > > > >
> > > > > > Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> > > > > > Example from hrtimer.rs.
> > > > > >
> > > > > > This is from lockdep:
> > > > > >
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> > > > > >
> > > > > > Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> > > > > > interrupt context? Or a more subtle one?
> > > > >
> > > > > I think it's calling mutex inside an interrupt context as shown by the
> > > > > callstack:
> > > > >
> > > > > ] __mutex_lock+0xa0/0xa4
> > > > > ] ...
> > > > > ] hrtimer_interrupt+0x1d4/0x2ac
> > > > >
> > > > > , it is because:
> > > > >
> > > > > +//! struct ArcIntrusiveTimer {
> > > > > +//! #[pin]
> > > > > +//! timer: Timer<Self>,
> > > > > +//! #[pin]
> > > > > +//! flag: Mutex<bool>,
> > > > > +//! #[pin]
> > > > > +//! cond: CondVar,
> > > > > +//! }
> > > > >
> > > > > has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> > > > > irq-off is needed for the lock, because otherwise we will hit a self
> > > > > deadlock due to interrupts:
> > > > >
> > > > > spin_lock(&a);
> > > > > > timer interrupt
> > > > > spin_lock(&a);
> > > > >
> > > > > Also notice that the IrqDisabled<'_> token can be simply created by
> > > > > ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> > > > > we don't support nested interrupts*).
> > > >
> > > > I updated the example based on the work in [1]. I think we need to
> > > > update `CondVar::wait` to support waiting with irq disabled.
> > >
> > > Yes, I agree. This answers one of the open questions I had in the discussion
> > > with Boqun :)
> > >
> > > What do you think regarding the other open question: In this *special* case
> > > here, what do you think to go *without* any lock? I mean the 'while *guard
> > > != 5' loop in the main thread is read only regarding guard. So it doesn't
> > > matter if it *reads* the old or the new value. And the read/modify/write of
> > > guard in the callback is done with interrupts disabled anyhow as it runs in
> > > interrupt context. And with this can't be interrupted (excluding nested
> > > interrupts). So this modification of guard doesn't need to be protected from
> > > being interrupted by a lock if there is no modifcation of guard "outside"
> > > the interupt locked context.
> > >
> > > What do you think?
> > >
> >
> > Reading while there is another CPU is writing is data-race, which is UB.
>
> Could you help to understand where exactly you see UB in Andreas' 'while
> *guard != 5' loop in case no locking is used? As mentioned I'm under the
Sure, but could you provide the code of what you mean exactly, if you
don't use a lock here, you cannot have a guard. I need to the exact code
to point out where the compiler may "mis-compile" (a result of being
UB).
> impression that it doesn't matter if the old or new guard value is read in
> this special case.
>
For one thing, if the compiler believes no one is accessing the value
because the code uses an immutable reference, it can "optimize" the loop
away:
while *var != 5 {
do_something();
}
into
if *var != 5 {
loop { do_something(); }
}
But as I said, I need to see the exact code to suggest a relevant
mis-compile, and note that sometimes, even mis-compile seems impossible
at the moment, a UB is a UB, compilers are free to do anything they
want (or don't want). So "mis-compile" is only helping we understand the
potential result of a UB.
Regards,
Boqun
> Best regards
>
> Dirk
>
>
> > Regards,
> > Boqun
> >
> > > Thanks
> > >
> > > Dirk
> > >
> > >
> > > > Without
> > > > this, when we get back from `bindings::schedule_timeout` in
> > > > `CondVar::wait_internal`, interrupts are enabled:
> > > >
> > > > ```rust
> > > > use kernel::{
> > > > hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
> > > > impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
> > > > irq::IrqDisabled,
> > > > prelude::*,
> > > > sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
> > > > time::Ktime,
> > > > };
> > > >
> > > > #[pin_data]
> > > > struct ArcIntrusiveTimer {
> > > > #[pin]
> > > > timer: Timer<Self>,
> > > > #[pin]
> > > > flag: SpinLockIrq<u64>,
> > > > #[pin]
> > > > cond: CondVar,
> > > > }
> > > >
> > > > impl ArcIntrusiveTimer {
> > > > fn new() -> impl PinInit<Self, kernel::error::Error> {
> > > > try_pin_init!(Self {
> > > > timer <- Timer::new(),
> > > > flag <- new_spinlock_irq!(0),
> > > > cond <- new_condvar!(),
> > > > })
> > > > }
> > > > }
> > > >
> > > > impl TimerCallback for ArcIntrusiveTimer {
> > > > type CallbackTarget<'a> = Arc<Self>;
> > > > type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
> > > >
> > > > fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
> > > > pr_info!("Timer called\n");
> > > > let mut guard = this.flag.lock_with(irq);
> > > > *guard += 1;
> > > > this.cond.notify_all();
> > > > if *guard == 5 {
> > > > TimerRestart::NoRestart
> > > > }
> > > > else {
> > > > TimerRestart::Restart
> > > >
> > > > }
> > > > }
> > > > }
> > > >
> > > > impl_has_timer! {
> > > > impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
> > > > }
> > > >
> > > >
> > > > let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
> > > > let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
> > > >
> > > > kernel::irq::with_irqs_disabled(|irq| {
> > > > let mut guard = has_timer.flag.lock_with(irq);
> > > >
> > > > while *guard != 5 {
> > > > pr_info!("Not 5 yet, waiting\n");
> > > > has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
> > > > }
> > > > });
> > > > ```
> > > >
> > > > I think an update of `CondVar::wait` should be part of the patch set [1].
> > > >
> > > >
> > > > Best regards,
> > > > Andreas
> > > >
> > > >
> > > > [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-12 7:41 ` Boqun Feng
@ 2024-10-12 7:50 ` Dirk Behme
2024-10-12 22:26 ` Boqun Feng
0 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-12 7:50 UTC (permalink / raw)
To: Boqun Feng
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On 12.10.24 09:41, Boqun Feng wrote:
> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
>> On 12.10.24 01:21, Boqun Feng wrote:
>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>>>> Hi Andreas,
>>>>
>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>>>
>>>>> Dirk, thanks for reporting!
>>>>
>>>> :)
>>>>
>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>>
>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>>>
>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>>>> ease the review process a bit.
>>>>>>>
>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>>>> Example from hrtimer.rs.
>>>>>>>
>>>>>>> This is from lockdep:
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>>>
>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>>>> interrupt context? Or a more subtle one?
>>>>>>
>>>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>>>> callstack:
>>>>>>
>>>>>> ] __mutex_lock+0xa0/0xa4
>>>>>> ] ...
>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>>>
>>>>>> , it is because:
>>>>>>
>>>>>> +//! struct ArcIntrusiveTimer {
>>>>>> +//! #[pin]
>>>>>> +//! timer: Timer<Self>,
>>>>>> +//! #[pin]
>>>>>> +//! flag: Mutex<bool>,
>>>>>> +//! #[pin]
>>>>>> +//! cond: CondVar,
>>>>>> +//! }
>>>>>>
>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>>>> deadlock due to interrupts:
>>>>>>
>>>>>> spin_lock(&a);
>>>>>> > timer interrupt
>>>>>> spin_lock(&a);
>>>>>>
>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>>>> we don't support nested interrupts*).
>>>>>
>>>>> I updated the example based on the work in [1]. I think we need to
>>>>> update `CondVar::wait` to support waiting with irq disabled.
>>>>
>>>> Yes, I agree. This answers one of the open questions I had in the discussion
>>>> with Boqun :)
>>>>
>>>> What do you think regarding the other open question: In this *special* case
>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>>>> matter if it *reads* the old or the new value. And the read/modify/write of
>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
>>>> interrupt context. And with this can't be interrupted (excluding nested
>>>> interrupts). So this modification of guard doesn't need to be protected from
>>>> being interrupted by a lock if there is no modifcation of guard "outside"
>>>> the interupt locked context.
>>>>
>>>> What do you think?
>>>>
>>>
>>> Reading while there is another CPU is writing is data-race, which is UB.
>>
>> Could you help to understand where exactly you see UB in Andreas' 'while
>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
>
> Sure, but could you provide the code of what you mean exactly, if you
> don't use a lock here, you cannot have a guard. I need to the exact code
> to point out where the compiler may "mis-compile" (a result of being
> UB).
I thought we are talking about anything like
#[pin_data]
struct ArcIntrusiveTimer {
#[pin]
timer: Timer<Self>,
#[pin]
- flag: SpinLockIrq<u64>,
+ flag: u64,
#[pin]
cond: CondVar,
}
?
Best regards
Dirk
>> impression that it doesn't matter if the old or new guard value is read in
>> this special case.
>>
>
> For one thing, if the compiler believes no one is accessing the value
> because the code uses an immutable reference, it can "optimize" the loop
> away:
>
> while *var != 5 {
> do_something();
> }
>
> into
>
> if *var != 5 {
> loop { do_something(); }
> }
>
> But as I said, I need to see the exact code to suggest a relevant
> mis-compile, and note that sometimes, even mis-compile seems impossible
> at the moment, a UB is a UB, compilers are free to do anything they
> want (or don't want). So "mis-compile" is only helping we understand the
> potential result of a UB.
>
> Regards,
> Boqun
>
>> Best regards
>>
>> Dirk
>>
>>
>>> Regards,
>>> Boqun
>>>
>>>> Thanks
>>>>
>>>> Dirk
>>>>
>>>>
>>>>> Without
>>>>> this, when we get back from `bindings::schedule_timeout` in
>>>>> `CondVar::wait_internal`, interrupts are enabled:
>>>>>
>>>>> ```rust
>>>>> use kernel::{
>>>>> hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
>>>>> impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
>>>>> irq::IrqDisabled,
>>>>> prelude::*,
>>>>> sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
>>>>> time::Ktime,
>>>>> };
>>>>>
>>>>> #[pin_data]
>>>>> struct ArcIntrusiveTimer {
>>>>> #[pin]
>>>>> timer: Timer<Self>,
>>>>> #[pin]
>>>>> flag: SpinLockIrq<u64>,
>>>>> #[pin]
>>>>> cond: CondVar,
>>>>> }
>>>>>
>>>>> impl ArcIntrusiveTimer {
>>>>> fn new() -> impl PinInit<Self, kernel::error::Error> {
>>>>> try_pin_init!(Self {
>>>>> timer <- Timer::new(),
>>>>> flag <- new_spinlock_irq!(0),
>>>>> cond <- new_condvar!(),
>>>>> })
>>>>> }
>>>>> }
>>>>>
>>>>> impl TimerCallback for ArcIntrusiveTimer {
>>>>> type CallbackTarget<'a> = Arc<Self>;
>>>>> type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
>>>>>
>>>>> fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
>>>>> pr_info!("Timer called\n");
>>>>> let mut guard = this.flag.lock_with(irq);
>>>>> *guard += 1;
>>>>> this.cond.notify_all();
>>>>> if *guard == 5 {
>>>>> TimerRestart::NoRestart
>>>>> }
>>>>> else {
>>>>> TimerRestart::Restart
>>>>>
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>> impl_has_timer! {
>>>>> impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
>>>>> }
>>>>>
>>>>>
>>>>> let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
>>>>> let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
>>>>>
>>>>> kernel::irq::with_irqs_disabled(|irq| {
>>>>> let mut guard = has_timer.flag.lock_with(irq);
>>>>>
>>>>> while *guard != 5 {
>>>>> pr_info!("Not 5 yet, waiting\n");
>>>>> has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
>>>>> }
>>>>> });
>>>>> ```
>>>>>
>>>>> I think an update of `CondVar::wait` should be part of the patch set [1].
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Andreas
>>>>>
>>>>>
>>>>> [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>>>>>
>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 14/14] rust: hrtimer: add maintainer entry
2024-09-17 22:27 ` [PATCH v2 14/14] rust: hrtimer: add maintainer entry Andreas Hindborg
@ 2024-10-12 15:19 ` Boqun Feng
0 siblings, 0 replies; 59+ messages in thread
From: Boqun Feng @ 2024-10-12 15:19 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, rust-for-linux,
linux-kernel
On Wed, Sep 18, 2024 at 12:27:38AM +0200, Andreas Hindborg wrote:
> Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
> add Boqun Feng as reviewer.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Although, I would like to suggest we move hrtimer under
rust/kernel/time.
Regards,
Boqun
> ---
> MAINTAINERS | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cc40a9d9b8cd..018847269dd3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10035,6 +10035,16 @@ F: kernel/time/timer_list.c
> F: kernel/time/timer_migration.*
> F: tools/testing/selftests/timers/
>
> +HIGH-RESOLUTION TIMERS [RUST]
> +M: Andreas Hindborg <a.hindborg@kernel.org>
> +R: Boqun Feng <boqun.feng@gmail.com>
> +L: rust-for-linux@vger.kernel.org
> +S: Supported
> +W: https://rust-for-linux.com
> +B: https://github.com/Rust-for-Linux/linux/issues
> +F: rust/kernel/hrtimer.rs
> +F: rust/kernel/hrtimer/
> +
> HIGH-SPEED SCC DRIVER FOR AX.25
> L: linux-hams@vger.kernel.org
> S: Orphan
> --
> 2.46.0
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-12 7:50 ` Dirk Behme
@ 2024-10-12 22:26 ` Boqun Feng
2024-10-13 17:39 ` Dirk Behme
0 siblings, 1 reply; 59+ messages in thread
From: Boqun Feng @ 2024-10-12 22:26 UTC (permalink / raw)
To: Dirk Behme
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
> On 12.10.24 09:41, Boqun Feng wrote:
> > On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
> > > On 12.10.24 01:21, Boqun Feng wrote:
> > > > On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> > > > > Hi Andreas,
> > > > >
> > > > > Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> > > > > >
> > > > > > Dirk, thanks for reporting!
> > > > >
> > > > > :)
> > > > >
> > > > > > Boqun Feng <boqun.feng@gmail.com> writes:
> > > > > >
> > > > > > > On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> > > > > > > > On 18.09.2024 00:27, Andreas Hindborg wrote:
> > > > > > > > > Hi!
> > > > > > > > >
> > > > > > > > > This series adds support for using the `hrtimer` subsystem from Rust code.
> > > > > > > > >
> > > > > > > > > I tried breaking up the code in some smaller patches, hopefully that will
> > > > > > > > > ease the review process a bit.
> > > > > > > >
> > > > > > > > Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> > > > > > > > Example from hrtimer.rs.
> > > > > > > >
> > > > > > > > This is from lockdep:
> > > > > > > >
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> > > > > > > >
> > > > > > > > Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> > > > > > > > interrupt context? Or a more subtle one?
> > > > > > >
> > > > > > > I think it's calling mutex inside an interrupt context as shown by the
> > > > > > > callstack:
> > > > > > >
> > > > > > > ] __mutex_lock+0xa0/0xa4
> > > > > > > ] ...
> > > > > > > ] hrtimer_interrupt+0x1d4/0x2ac
> > > > > > >
> > > > > > > , it is because:
> > > > > > >
> > > > > > > +//! struct ArcIntrusiveTimer {
> > > > > > > +//! #[pin]
> > > > > > > +//! timer: Timer<Self>,
> > > > > > > +//! #[pin]
> > > > > > > +//! flag: Mutex<bool>,
> > > > > > > +//! #[pin]
> > > > > > > +//! cond: CondVar,
> > > > > > > +//! }
> > > > > > >
> > > > > > > has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> > > > > > > irq-off is needed for the lock, because otherwise we will hit a self
> > > > > > > deadlock due to interrupts:
> > > > > > >
> > > > > > > spin_lock(&a);
> > > > > > > > timer interrupt
> > > > > > > spin_lock(&a);
> > > > > > >
> > > > > > > Also notice that the IrqDisabled<'_> token can be simply created by
> > > > > > > ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> > > > > > > we don't support nested interrupts*).
> > > > > >
> > > > > > I updated the example based on the work in [1]. I think we need to
> > > > > > update `CondVar::wait` to support waiting with irq disabled.
> > > > >
> > > > > Yes, I agree. This answers one of the open questions I had in the discussion
> > > > > with Boqun :)
> > > > >
> > > > > What do you think regarding the other open question: In this *special* case
> > > > > here, what do you think to go *without* any lock? I mean the 'while *guard
> > > > > != 5' loop in the main thread is read only regarding guard. So it doesn't
> > > > > matter if it *reads* the old or the new value. And the read/modify/write of
> > > > > guard in the callback is done with interrupts disabled anyhow as it runs in
> > > > > interrupt context. And with this can't be interrupted (excluding nested
> > > > > interrupts). So this modification of guard doesn't need to be protected from
> > > > > being interrupted by a lock if there is no modifcation of guard "outside"
> > > > > the interupt locked context.
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > Reading while there is another CPU is writing is data-race, which is UB.
> > >
> > > Could you help to understand where exactly you see UB in Andreas' 'while
> > > *guard != 5' loop in case no locking is used? As mentioned I'm under the
> >
> > Sure, but could you provide the code of what you mean exactly, if you
> > don't use a lock here, you cannot have a guard. I need to the exact code
> > to point out where the compiler may "mis-compile" (a result of being
> > UB).
>
>
> I thought we are talking about anything like
>
> #[pin_data]
> struct ArcIntrusiveTimer {
> #[pin]
> timer: Timer<Self>,
> #[pin]
> - flag: SpinLockIrq<u64>,
> + flag: u64,
> #[pin]
> cond: CondVar,
> }
>
> ?
>
Yes, but have you tried to actually use that for the example from
Andreas? I think you will find that you cannot write to `flag` inside
the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
not mutable reference for `ArcIntrusiveTimer`. You can of course use
unsafe to create a mutable reference to `flag`, but it won't be sound,
since you are getting a mutable reference from an immutable reference.
Regards,
Boqun
> Best regards
>
> Dirk
>
> > > impression that it doesn't matter if the old or new guard value is read in
> > > this special case.
> > >
> >
> > For one thing, if the compiler believes no one is accessing the value
> > because the code uses an immutable reference, it can "optimize" the loop
> > away:
> >
> > while *var != 5 {
> > do_something();
> > }
> >
> > into
> >
> > if *var != 5 {
> > loop { do_something(); }
> > }
> >
> > But as I said, I need to see the exact code to suggest a relevant
> > mis-compile, and note that sometimes, even mis-compile seems impossible
> > at the moment, a UB is a UB, compilers are free to do anything they
> > want (or don't want). So "mis-compile" is only helping we understand the
> > potential result of a UB.
> >
> > Regards,
> > Boqun
> >
> > > Best regards
> > >
> > > Dirk
> > >
> > >
> > > > Regards,
> > > > Boqun
> > > >
> > > > > Thanks
> > > > >
> > > > > Dirk
> > > > >
> > > > >
> > > > > > Without
> > > > > > this, when we get back from `bindings::schedule_timeout` in
> > > > > > `CondVar::wait_internal`, interrupts are enabled:
> > > > > >
> > > > > > ```rust
> > > > > > use kernel::{
> > > > > > hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
> > > > > > impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
> > > > > > irq::IrqDisabled,
> > > > > > prelude::*,
> > > > > > sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
> > > > > > time::Ktime,
> > > > > > };
> > > > > >
> > > > > > #[pin_data]
> > > > > > struct ArcIntrusiveTimer {
> > > > > > #[pin]
> > > > > > timer: Timer<Self>,
> > > > > > #[pin]
> > > > > > flag: SpinLockIrq<u64>,
> > > > > > #[pin]
> > > > > > cond: CondVar,
> > > > > > }
> > > > > >
> > > > > > impl ArcIntrusiveTimer {
> > > > > > fn new() -> impl PinInit<Self, kernel::error::Error> {
> > > > > > try_pin_init!(Self {
> > > > > > timer <- Timer::new(),
> > > > > > flag <- new_spinlock_irq!(0),
> > > > > > cond <- new_condvar!(),
> > > > > > })
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > impl TimerCallback for ArcIntrusiveTimer {
> > > > > > type CallbackTarget<'a> = Arc<Self>;
> > > > > > type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
> > > > > >
> > > > > > fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
> > > > > > pr_info!("Timer called\n");
> > > > > > let mut guard = this.flag.lock_with(irq);
> > > > > > *guard += 1;
> > > > > > this.cond.notify_all();
> > > > > > if *guard == 5 {
> > > > > > TimerRestart::NoRestart
> > > > > > }
> > > > > > else {
> > > > > > TimerRestart::Restart
> > > > > >
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > impl_has_timer! {
> > > > > > impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
> > > > > > }
> > > > > >
> > > > > >
> > > > > > let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
> > > > > > let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
> > > > > >
> > > > > > kernel::irq::with_irqs_disabled(|irq| {
> > > > > > let mut guard = has_timer.flag.lock_with(irq);
> > > > > >
> > > > > > while *guard != 5 {
> > > > > > pr_info!("Not 5 yet, waiting\n");
> > > > > > has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
> > > > > > }
> > > > > > });
> > > > > > ```
> > > > > >
> > > > > > I think an update of `CondVar::wait` should be part of the patch set [1].
> > > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Andreas
> > > > > >
> > > > > >
> > > > > > [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
> > > > > >
> > > > > >
> > > > >
> > >
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-12 22:26 ` Boqun Feng
@ 2024-10-13 17:39 ` Dirk Behme
2024-10-13 21:06 ` Boqun Feng
0 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-13 17:39 UTC (permalink / raw)
To: Boqun Feng
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On 13.10.24 00:26, Boqun Feng wrote:
> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
>> On 12.10.24 09:41, Boqun Feng wrote:
>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
>>>> On 12.10.24 01:21, Boqun Feng wrote:
>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>>>>>> Hi Andreas,
>>>>>>
>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>>>>>
>>>>>>> Dirk, thanks for reporting!
>>>>>>
>>>>>> :)
>>>>>>
>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>>>>
>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>>>>>
>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>>>>>> ease the review process a bit.
>>>>>>>>>
>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>>>>>> Example from hrtimer.rs.
>>>>>>>>>
>>>>>>>>> This is from lockdep:
>>>>>>>>>
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>>>>>
>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>>>>>> interrupt context? Or a more subtle one?
>>>>>>>>
>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>>>>>> callstack:
>>>>>>>>
>>>>>>>> ] __mutex_lock+0xa0/0xa4
>>>>>>>> ] ...
>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>>>>>
>>>>>>>> , it is because:
>>>>>>>>
>>>>>>>> +//! struct ArcIntrusiveTimer {
>>>>>>>> +//! #[pin]
>>>>>>>> +//! timer: Timer<Self>,
>>>>>>>> +//! #[pin]
>>>>>>>> +//! flag: Mutex<bool>,
>>>>>>>> +//! #[pin]
>>>>>>>> +//! cond: CondVar,
>>>>>>>> +//! }
>>>>>>>>
>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>>>>>> deadlock due to interrupts:
>>>>>>>>
>>>>>>>> spin_lock(&a);
>>>>>>>> > timer interrupt
>>>>>>>> spin_lock(&a);
>>>>>>>>
>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>>>>>> we don't support nested interrupts*).
>>>>>>>
>>>>>>> I updated the example based on the work in [1]. I think we need to
>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
>>>>>>
>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
>>>>>> with Boqun :)
>>>>>>
>>>>>> What do you think regarding the other open question: In this *special* case
>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
>>>>>> interrupt context. And with this can't be interrupted (excluding nested
>>>>>> interrupts). So this modification of guard doesn't need to be protected from
>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
>>>>>> the interupt locked context.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>
>>>>> Reading while there is another CPU is writing is data-race, which is UB.
>>>>
>>>> Could you help to understand where exactly you see UB in Andreas' 'while
>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
>>>
>>> Sure, but could you provide the code of what you mean exactly, if you
>>> don't use a lock here, you cannot have a guard. I need to the exact code
>>> to point out where the compiler may "mis-compile" (a result of being
>>> UB).
>>
>>
>> I thought we are talking about anything like
>>
>> #[pin_data]
>> struct ArcIntrusiveTimer {
>> #[pin]
>> timer: Timer<Self>,
>> #[pin]
>> - flag: SpinLockIrq<u64>,
>> + flag: u64,
>> #[pin]
>> cond: CondVar,
>> }
>>
>> ?
>>
>
> Yes, but have you tried to actually use that for the example from
> Andreas? I think you will find that you cannot write to `flag` inside
> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
> not mutable reference for `ArcIntrusiveTimer`. You can of course use
> unsafe to create a mutable reference to `flag`, but it won't be sound,
> since you are getting a mutable reference from an immutable reference.
Yes, of course. But, hmm, wouldn't that unsoundness be independent on
the topic we discuss here? I mean we are talking about getting the
compiler to read/modify/write 'flag' in the TimerCallback. *How* we
tell him to do so should be independent on the result what we want to
look at regarding the locking requirements of 'flag'?
Anyhow, my root motivation was to simplify Andreas example to not use
a lock where not strictly required. And with this make Andreas example
independent on mutex lockdep issues, SpinLockIrq changes and possible
required CondVar updates. But maybe we find an other way to simplify
it and decrease the dependencies. In the end its just example code ;)
Best regards
Dirk
> Regards,
> Boqun
>
>> Best regards
>>
>> Dirk
>>
>>>> impression that it doesn't matter if the old or new guard value is read in
>>>> this special case.
>>>>
>>>
>>> For one thing, if the compiler believes no one is accessing the value
>>> because the code uses an immutable reference, it can "optimize" the loop
>>> away:
>>>
>>> while *var != 5 {
>>> do_something();
>>> }
>>>
>>> into
>>>
>>> if *var != 5 {
>>> loop { do_something(); }
>>> }
>>>
>>> But as I said, I need to see the exact code to suggest a relevant
>>> mis-compile, and note that sometimes, even mis-compile seems impossible
>>> at the moment, a UB is a UB, compilers are free to do anything they
>>> want (or don't want). So "mis-compile" is only helping we understand the
>>> potential result of a UB.
>>>
>>> Regards,
>>> Boqun
>>>
>>>> Best regards
>>>>
>>>> Dirk
>>>>
>>>>
>>>>> Regards,
>>>>> Boqun
>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Dirk
>>>>>>
>>>>>>
>>>>>>> Without
>>>>>>> this, when we get back from `bindings::schedule_timeout` in
>>>>>>> `CondVar::wait_internal`, interrupts are enabled:
>>>>>>>
>>>>>>> ```rust
>>>>>>> use kernel::{
>>>>>>> hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart},
>>>>>>> impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq,
>>>>>>> irq::IrqDisabled,
>>>>>>> prelude::*,
>>>>>>> sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq},
>>>>>>> time::Ktime,
>>>>>>> };
>>>>>>>
>>>>>>> #[pin_data]
>>>>>>> struct ArcIntrusiveTimer {
>>>>>>> #[pin]
>>>>>>> timer: Timer<Self>,
>>>>>>> #[pin]
>>>>>>> flag: SpinLockIrq<u64>,
>>>>>>> #[pin]
>>>>>>> cond: CondVar,
>>>>>>> }
>>>>>>>
>>>>>>> impl ArcIntrusiveTimer {
>>>>>>> fn new() -> impl PinInit<Self, kernel::error::Error> {
>>>>>>> try_pin_init!(Self {
>>>>>>> timer <- Timer::new(),
>>>>>>> flag <- new_spinlock_irq!(0),
>>>>>>> cond <- new_condvar!(),
>>>>>>> })
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> impl TimerCallback for ArcIntrusiveTimer {
>>>>>>> type CallbackTarget<'a> = Arc<Self>;
>>>>>>> type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>;
>>>>>>>
>>>>>>> fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart {
>>>>>>> pr_info!("Timer called\n");
>>>>>>> let mut guard = this.flag.lock_with(irq);
>>>>>>> *guard += 1;
>>>>>>> this.cond.notify_all();
>>>>>>> if *guard == 5 {
>>>>>>> TimerRestart::NoRestart
>>>>>>> }
>>>>>>> else {
>>>>>>> TimerRestart::Restart
>>>>>>>
>>>>>>> }
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> impl_has_timer! {
>>>>>>> impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
>>>>>>> let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
>>>>>>>
>>>>>>> kernel::irq::with_irqs_disabled(|irq| {
>>>>>>> let mut guard = has_timer.flag.lock_with(irq);
>>>>>>>
>>>>>>> while *guard != 5 {
>>>>>>> pr_info!("Not 5 yet, waiting\n");
>>>>>>> has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled!
>>>>>>> }
>>>>>>> });
>>>>>>> ```
>>>>>>>
>>>>>>> I think an update of `CondVar::wait` should be part of the patch set [1].
>>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Andreas
>>>>>>>
>>>>>>>
>>>>>>> [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-13 17:39 ` Dirk Behme
@ 2024-10-13 21:06 ` Boqun Feng
2024-10-14 6:58 ` Dirk Behme
0 siblings, 1 reply; 59+ messages in thread
From: Boqun Feng @ 2024-10-13 21:06 UTC (permalink / raw)
To: Dirk Behme
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
> On 13.10.24 00:26, Boqun Feng wrote:
> > On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
> > > On 12.10.24 09:41, Boqun Feng wrote:
> > > > On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
> > > > > On 12.10.24 01:21, Boqun Feng wrote:
> > > > > > On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> > > > > > > Hi Andreas,
> > > > > > >
> > > > > > > Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> > > > > > > >
> > > > > > > > Dirk, thanks for reporting!
> > > > > > >
> > > > > > > :)
> > > > > > >
> > > > > > > > Boqun Feng <boqun.feng@gmail.com> writes:
> > > > > > > >
> > > > > > > > > On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> > > > > > > > > > On 18.09.2024 00:27, Andreas Hindborg wrote:
> > > > > > > > > > > Hi!
> > > > > > > > > > >
> > > > > > > > > > > This series adds support for using the `hrtimer` subsystem from Rust code.
> > > > > > > > > > >
> > > > > > > > > > > I tried breaking up the code in some smaller patches, hopefully that will
> > > > > > > > > > > ease the review process a bit.
> > > > > > > > > >
> > > > > > > > > > Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> > > > > > > > > > Example from hrtimer.rs.
> > > > > > > > > >
> > > > > > > > > > This is from lockdep:
> > > > > > > > > >
> > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> > > > > > > > > >
> > > > > > > > > > Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> > > > > > > > > > interrupt context? Or a more subtle one?
> > > > > > > > >
> > > > > > > > > I think it's calling mutex inside an interrupt context as shown by the
> > > > > > > > > callstack:
> > > > > > > > >
> > > > > > > > > ] __mutex_lock+0xa0/0xa4
> > > > > > > > > ] ...
> > > > > > > > > ] hrtimer_interrupt+0x1d4/0x2ac
> > > > > > > > >
> > > > > > > > > , it is because:
> > > > > > > > >
> > > > > > > > > +//! struct ArcIntrusiveTimer {
> > > > > > > > > +//! #[pin]
> > > > > > > > > +//! timer: Timer<Self>,
> > > > > > > > > +//! #[pin]
> > > > > > > > > +//! flag: Mutex<bool>,
> > > > > > > > > +//! #[pin]
> > > > > > > > > +//! cond: CondVar,
> > > > > > > > > +//! }
> > > > > > > > >
> > > > > > > > > has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> > > > > > > > > irq-off is needed for the lock, because otherwise we will hit a self
> > > > > > > > > deadlock due to interrupts:
> > > > > > > > >
> > > > > > > > > spin_lock(&a);
> > > > > > > > > > timer interrupt
> > > > > > > > > spin_lock(&a);
> > > > > > > > >
> > > > > > > > > Also notice that the IrqDisabled<'_> token can be simply created by
> > > > > > > > > ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> > > > > > > > > we don't support nested interrupts*).
> > > > > > > >
> > > > > > > > I updated the example based on the work in [1]. I think we need to
> > > > > > > > update `CondVar::wait` to support waiting with irq disabled.
> > > > > > >
> > > > > > > Yes, I agree. This answers one of the open questions I had in the discussion
> > > > > > > with Boqun :)
> > > > > > >
> > > > > > > What do you think regarding the other open question: In this *special* case
> > > > > > > here, what do you think to go *without* any lock? I mean the 'while *guard
> > > > > > > != 5' loop in the main thread is read only regarding guard. So it doesn't
> > > > > > > matter if it *reads* the old or the new value. And the read/modify/write of
> > > > > > > guard in the callback is done with interrupts disabled anyhow as it runs in
> > > > > > > interrupt context. And with this can't be interrupted (excluding nested
> > > > > > > interrupts). So this modification of guard doesn't need to be protected from
> > > > > > > being interrupted by a lock if there is no modifcation of guard "outside"
> > > > > > > the interupt locked context.
> > > > > > >
> > > > > > > What do you think?
> > > > > > >
> > > > > >
> > > > > > Reading while there is another CPU is writing is data-race, which is UB.
> > > > >
> > > > > Could you help to understand where exactly you see UB in Andreas' 'while
> > > > > *guard != 5' loop in case no locking is used? As mentioned I'm under the
> > > >
> > > > Sure, but could you provide the code of what you mean exactly, if you
> > > > don't use a lock here, you cannot have a guard. I need to the exact code
> > > > to point out where the compiler may "mis-compile" (a result of being
[...]
> > > I thought we are talking about anything like
> > >
> > > #[pin_data]
> > > struct ArcIntrusiveTimer {
> > > #[pin]
> > > timer: Timer<Self>,
> > > #[pin]
> > > - flag: SpinLockIrq<u64>,
> > > + flag: u64,
> > > #[pin]
> > > cond: CondVar,
> > > }
> > >
> > > ?
> > >
> >
> > Yes, but have you tried to actually use that for the example from
> > Andreas? I think you will find that you cannot write to `flag` inside
> > the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
> > not mutable reference for `ArcIntrusiveTimer`. You can of course use
> > unsafe to create a mutable reference to `flag`, but it won't be sound,
> > since you are getting a mutable reference from an immutable reference.
>
> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
> topic we discuss here? I mean we are talking about getting the compiler to
What do you mean? If the code is unsound, you won't want to use it in an
example, right?
> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
> should be independent on the result what we want to look at regarding the
> locking requirements of 'flag'?
>
> Anyhow, my root motivation was to simplify Andreas example to not use a lock
> where not strictly required. And with this make Andreas example independent
Well, if you don't want to use a lock then you need to use atomics,
otherwise it's likely a UB, but atomics are still WIP, so that why I
suggested Andreas to use a lock first. But I guess I didn't realise the
lock needs to be irq-safe when I suggested that.
Regards,
Boqun
> on mutex lockdep issues, SpinLockIrq changes and possible required CondVar
> updates. But maybe we find an other way to simplify it and decrease the
> dependencies. In the end its just example code ;)
>
> Best regards
>
> Dirk
>
>
> > Regards,
> > Boqun
> >
[...]
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-13 21:06 ` Boqun Feng
@ 2024-10-14 6:58 ` Dirk Behme
2024-10-14 9:17 ` Andreas Hindborg
2024-10-14 9:38 ` Alice Ryhl
0 siblings, 2 replies; 59+ messages in thread
From: Dirk Behme @ 2024-10-14 6:58 UTC (permalink / raw)
To: Boqun Feng
Cc: Andreas Hindborg, Lyude Paul, Dirk Behme, Miguel Ojeda,
Alex Gaynor, Anna-Maria Behnsen, Frederic Weisbecker,
Thomas Gleixner, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, rust-for-linux, linux-kernel
On 13.10.24 23:06, Boqun Feng wrote:
> On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
>> On 13.10.24 00:26, Boqun Feng wrote:
>>> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
>>>> On 12.10.24 09:41, Boqun Feng wrote:
>>>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
>>>>>> On 12.10.24 01:21, Boqun Feng wrote:
>>>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>>>>>>>> Hi Andreas,
>>>>>>>>
>>>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>>>>>>>
>>>>>>>>> Dirk, thanks for reporting!
>>>>>>>>
>>>>>>>> :)
>>>>>>>>
>>>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>>>>>>
>>>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>>>>>>>
>>>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>>>>>>>> ease the review process a bit.
>>>>>>>>>>>
>>>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>>>>>>>> Example from hrtimer.rs.
>>>>>>>>>>>
>>>>>>>>>>> This is from lockdep:
>>>>>>>>>>>
>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>>>>>>>
>>>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>>>>>>>> interrupt context? Or a more subtle one?
>>>>>>>>>>
>>>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>>>>>>>> callstack:
>>>>>>>>>>
>>>>>>>>>> ] __mutex_lock+0xa0/0xa4
>>>>>>>>>> ] ...
>>>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>>>>>>>
>>>>>>>>>> , it is because:
>>>>>>>>>>
>>>>>>>>>> +//! struct ArcIntrusiveTimer {
>>>>>>>>>> +//! #[pin]
>>>>>>>>>> +//! timer: Timer<Self>,
>>>>>>>>>> +//! #[pin]
>>>>>>>>>> +//! flag: Mutex<bool>,
>>>>>>>>>> +//! #[pin]
>>>>>>>>>> +//! cond: CondVar,
>>>>>>>>>> +//! }
>>>>>>>>>>
>>>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>>>>>>>> deadlock due to interrupts:
>>>>>>>>>>
>>>>>>>>>> spin_lock(&a);
>>>>>>>>>> > timer interrupt
>>>>>>>>>> spin_lock(&a);
>>>>>>>>>>
>>>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>>>>>>>> we don't support nested interrupts*).
>>>>>>>>>
>>>>>>>>> I updated the example based on the work in [1]. I think we need to
>>>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
>>>>>>>>
>>>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
>>>>>>>> with Boqun :)
>>>>>>>>
>>>>>>>> What do you think regarding the other open question: In this *special* case
>>>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
>>>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>>>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
>>>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
>>>>>>>> interrupt context. And with this can't be interrupted (excluding nested
>>>>>>>> interrupts). So this modification of guard doesn't need to be protected from
>>>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
>>>>>>>> the interupt locked context.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>
>>>>>>> Reading while there is another CPU is writing is data-race, which is UB.
>>>>>>
>>>>>> Could you help to understand where exactly you see UB in Andreas' 'while
>>>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
>>>>>
>>>>> Sure, but could you provide the code of what you mean exactly, if you
>>>>> don't use a lock here, you cannot have a guard. I need to the exact code
>>>>> to point out where the compiler may "mis-compile" (a result of being
> [...]
>>>> I thought we are talking about anything like
>>>>
>>>> #[pin_data]
>>>> struct ArcIntrusiveTimer {
>>>> #[pin]
>>>> timer: Timer<Self>,
>>>> #[pin]
>>>> - flag: SpinLockIrq<u64>,
>>>> + flag: u64,
>>>> #[pin]
>>>> cond: CondVar,
>>>> }
>>>>
>>>> ?
>>>>
>>>
>>> Yes, but have you tried to actually use that for the example from
>>> Andreas? I think you will find that you cannot write to `flag` inside
>>> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
>>> not mutable reference for `ArcIntrusiveTimer`. You can of course use
>>> unsafe to create a mutable reference to `flag`, but it won't be sound,
>>> since you are getting a mutable reference from an immutable reference.
>>
>> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
>> topic we discuss here? I mean we are talking about getting the compiler to
>
> What do you mean? If the code is unsound, you won't want to use it in an
> example, right?
Yes, sure. But ;)
In a first step I just wanted to answer the question if we do need a
lock at all in this special example. And that we could do even with
unsound read/modify/write I would guess. And then, in a second step,
if the answer would be "we don't need the lock", then we could think
about how to make the flag handling sound. So I'm talking just about
answering that question, not about the final example code. Step by step :)
>> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
>> should be independent on the result what we want to look at regarding the
>> locking requirements of 'flag'?
>>
>> Anyhow, my root motivation was to simplify Andreas example to not use a lock
>> where not strictly required. And with this make Andreas example independent
>
> Well, if you don't want to use a lock then you need to use atomics,
> otherwise it's likely a UB,
And here we are back to the initial question :) Why would it be UB
without lock (and atomics)?
Some (pseudo) assembly:
Lets start with the main thread:
ldr x1, [x0]
<work with x1>
x0 and x1 are registers. x0 contains the address of flag in the main
memory. I.e. that instruction reads (ldr == load) the content of that
memory location (flag) into x1. x1 then contains flag which can be
used then. This is what I mean with "the main thread is read only". If
flag, i.e. x1, does contain the old or new flag value doesn't matter.
I.e. for the read only operation it doesn't matter if it is protected
by a lock as the load (ldr) can't be interrupted.
Now to the TimerCallback:
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
This is what I mean with read/modify/write. And this needs to be
ensured that it is not interruptable. I.e. that we are scheduled
between ldr and add or between add and str. Yes, I *totally* agree
that for this a lock is needed:
<lock>
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
<unlock>
But:
In this this special example we know that we are executing this code
in interrupt context. I.e.:
<interrupts are disabled>
ldr x1, [x0]
add x1, x1, #1
str x1, [x0]
<interrupts are still disabled>
So this read/modify/write can't be interrupted because the interrupts
are off. I.e. the interrupt off prevents the scheduling here. And in
this sense replaces the lock. And as mentioned, which value is read by
the main thread doesn't matter.
To summarize: I totally agree that usually a lock would be needed. But
in this special case with (a) read/modify/write in interrupt context
*and* (b) read only in main thread I'm unclear.
So with this back to the main question: What is my misunderstanding
here? I.e. what is UB in this special case? :)
Best regards
Dirk
> but atomics are still WIP, so that why I
> suggested Andreas to use a lock first. But I guess I didn't realise the
> lock needs to be irq-safe when I suggested that.
>
> Regards,
> Boqun
>
>> on mutex lockdep issues, SpinLockIrq changes and possible required CondVar
>> updates. But maybe we find an other way to simplify it and decrease the
>> dependencies. In the end its just example code ;)
>>
>> Best regards
>>
>> Dirk
>>
>>
>>> Regards,
>>> Boqun
>>>
> [...]
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-14 6:58 ` Dirk Behme
@ 2024-10-14 9:17 ` Andreas Hindborg
2024-10-14 9:38 ` Alice Ryhl
1 sibling, 0 replies; 59+ messages in thread
From: Andreas Hindborg @ 2024-10-14 9:17 UTC (permalink / raw)
To: Dirk Behme
Cc: Boqun Feng, Lyude Paul, Dirk Behme, Miguel Ojeda, Alex Gaynor,
Anna-Maria Behnsen, Frederic Weisbecker, Thomas Gleixner,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
rust-for-linux, linux-kernel
"Dirk Behme" <dirk.behme@gmail.com> writes:
> On 13.10.24 23:06, Boqun Feng wrote:
>> On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
>>> On 13.10.24 00:26, Boqun Feng wrote:
>>>> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
>>>>> On 12.10.24 09:41, Boqun Feng wrote:
>>>>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
>>>>>>> On 12.10.24 01:21, Boqun Feng wrote:
>>>>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>>>>>>>>> Hi Andreas,
>>>>>>>>>
>>>>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>>>>>>>>
>>>>>>>>>> Dirk, thanks for reporting!
>>>>>>>>>
>>>>>>>>> :)
>>>>>>>>>
>>>>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>>>>>>>
>>>>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>
>>>>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>>>>>>>>> ease the review process a bit.
>>>>>>>>>>>>
>>>>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>>>>>>>>> Example from hrtimer.rs.
>>>>>>>>>>>>
>>>>>>>>>>>> This is from lockdep:
>>>>>>>>>>>>
>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>>>>>>>>
>>>>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>>>>>>>>> interrupt context? Or a more subtle one?
>>>>>>>>>>>
>>>>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>>>>>>>>> callstack:
>>>>>>>>>>>
>>>>>>>>>>> ] __mutex_lock+0xa0/0xa4
>>>>>>>>>>> ] ...
>>>>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>>>>>>>>
>>>>>>>>>>> , it is because:
>>>>>>>>>>>
>>>>>>>>>>> +//! struct ArcIntrusiveTimer {
>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>> +//! timer: Timer<Self>,
>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>> +//! flag: Mutex<bool>,
>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>> +//! cond: CondVar,
>>>>>>>>>>> +//! }
>>>>>>>>>>>
>>>>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>>>>>>>>> deadlock due to interrupts:
>>>>>>>>>>>
>>>>>>>>>>> spin_lock(&a);
>>>>>>>>>>> > timer interrupt
>>>>>>>>>>> spin_lock(&a);
>>>>>>>>>>>
>>>>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>>>>>>>>> we don't support nested interrupts*).
>>>>>>>>>>
>>>>>>>>>> I updated the example based on the work in [1]. I think we need to
>>>>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
>>>>>>>>>
>>>>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
>>>>>>>>> with Boqun :)
>>>>>>>>>
>>>>>>>>> What do you think regarding the other open question: In this *special* case
>>>>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
>>>>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>>>>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
>>>>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
>>>>>>>>> interrupt context. And with this can't be interrupted (excluding nested
>>>>>>>>> interrupts). So this modification of guard doesn't need to be protected from
>>>>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
>>>>>>>>> the interupt locked context.
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Reading while there is another CPU is writing is data-race, which is UB.
>>>>>>>
>>>>>>> Could you help to understand where exactly you see UB in Andreas' 'while
>>>>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
>>>>>>
>>>>>> Sure, but could you provide the code of what you mean exactly, if you
>>>>>> don't use a lock here, you cannot have a guard. I need to the exact code
>>>>>> to point out where the compiler may "mis-compile" (a result of being
>> [...]
>>>>> I thought we are talking about anything like
>>>>>
>>>>> #[pin_data]
>>>>> struct ArcIntrusiveTimer {
>>>>> #[pin]
>>>>> timer: Timer<Self>,
>>>>> #[pin]
>>>>> - flag: SpinLockIrq<u64>,
>>>>> + flag: u64,
>>>>> #[pin]
>>>>> cond: CondVar,
>>>>> }
>>>>>
>>>>> ?
>>>>>
>>>>
>>>> Yes, but have you tried to actually use that for the example from
>>>> Andreas? I think you will find that you cannot write to `flag` inside
>>>> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
>>>> not mutable reference for `ArcIntrusiveTimer`. You can of course use
>>>> unsafe to create a mutable reference to `flag`, but it won't be sound,
>>>> since you are getting a mutable reference from an immutable reference.
>>>
>>> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
>>> topic we discuss here? I mean we are talking about getting the compiler to
>>
>> What do you mean? If the code is unsound, you won't want to use it in an
>> example, right?
>
> Yes, sure. But ;)
>
> In a first step I just wanted to answer the question if we do need a
> lock at all in this special example. And that we could do even with
> unsound read/modify/write I would guess. And then, in a second step,
> if the answer would be "we don't need the lock", then we could think
> about how to make the flag handling sound. So I'm talking just about
> answering that question, not about the final example code. Step by step :)
>
>
>>> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
>>> should be independent on the result what we want to look at regarding the
>>> locking requirements of 'flag'?
>>>
>>> Anyhow, my root motivation was to simplify Andreas example to not use a lock
>>> where not strictly required. And with this make Andreas example independent
>>
>> Well, if you don't want to use a lock then you need to use atomics,
>> otherwise it's likely a UB,
>
> And here we are back to the initial question :) Why would it be UB
> without lock (and atomics)?
It is UB at the language level. Miri will yell at you. If you do this,
the compiler will give you zero guarantees.
> Some (pseudo) assembly:
>
> Lets start with the main thread:
>
> ldr x1, [x0]
> <work with x1>
>
> x0 and x1 are registers. x0 contains the address of flag in the main
> memory. I.e. that instruction reads (ldr == load) the content of that
> memory location (flag) into x1. x1 then contains flag which can be
> used then. This is what I mean with "the main thread is read only". If
> flag, i.e. x1, does contain the old or new flag value doesn't matter.
> I.e. for the read only operation it doesn't matter if it is protected
> by a lock as the load (ldr) can't be interrupted.
>
> Now to the TimerCallback:
>
> ldr x1, [x0]
> add x1, x1, #1
> str x1, [x0]
>
> This is what I mean with read/modify/write. And this needs to be
> ensured that it is not interruptable. I.e. that we are scheduled
> between ldr and add or between add and str. Yes, I *totally* agree
> that for this a lock is needed:
>
> <lock>
> ldr x1, [x0]
> add x1, x1, #1
> str x1, [x0]
> <unlock>
>
> But:
>
> In this this special example we know that we are executing this code
> in interrupt context. I.e.:
>
> <interrupts are disabled>
> ldr x1, [x0]
> add x1, x1, #1
> str x1, [x0]
> <interrupts are still disabled>
>
> So this read/modify/write can't be interrupted because the interrupts
> are off. I.e. the interrupt off prevents the scheduling here. And in
> this sense replaces the lock. And as mentioned, which value is read by
> the main thread doesn't matter.
You can have the interrupt handler running on one core and the process
on another core. For uni-processor systems you are right. I actually
think spinlock operations collapse to no-ops on non-SMP configurations.
Bur for SMP configurations, this would be broken.
I don't think the rust language cares that though. Doing this kind of
modification from multiple execution contexts without synchronization is
always UB in rust.
BR Andreas
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-14 6:58 ` Dirk Behme
2024-10-14 9:17 ` Andreas Hindborg
@ 2024-10-14 9:38 ` Alice Ryhl
2024-10-14 11:53 ` Dirk Behme
1 sibling, 1 reply; 59+ messages in thread
From: Alice Ryhl @ 2024-10-14 9:38 UTC (permalink / raw)
To: Dirk Behme
Cc: Boqun Feng, Andreas Hindborg, Lyude Paul, Dirk Behme,
Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, rust-for-linux, linux-kernel
On Mon, Oct 14, 2024 at 8:58 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>
> On 13.10.24 23:06, Boqun Feng wrote:
> > On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
> >> On 13.10.24 00:26, Boqun Feng wrote:
> >>> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
> >>>> On 12.10.24 09:41, Boqun Feng wrote:
> >>>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
> >>>>>> On 12.10.24 01:21, Boqun Feng wrote:
> >>>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> >>>>>>>> Hi Andreas,
> >>>>>>>>
> >>>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> >>>>>>>>>
> >>>>>>>>> Dirk, thanks for reporting!
> >>>>>>>>
> >>>>>>>> :)
> >>>>>>>>
> >>>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
> >>>>>>>>>
> >>>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> >>>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
> >>>>>>>>>>>> Hi!
> >>>>>>>>>>>>
> >>>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
> >>>>>>>>>>>> ease the review process a bit.
> >>>>>>>>>>>
> >>>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> >>>>>>>>>>> Example from hrtimer.rs.
> >>>>>>>>>>>
> >>>>>>>>>>> This is from lockdep:
> >>>>>>>>>>>
> >>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> >>>>>>>>>>>
> >>>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> >>>>>>>>>>> interrupt context? Or a more subtle one?
> >>>>>>>>>>
> >>>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
> >>>>>>>>>> callstack:
> >>>>>>>>>>
> >>>>>>>>>> ] __mutex_lock+0xa0/0xa4
> >>>>>>>>>> ] ...
> >>>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
> >>>>>>>>>>
> >>>>>>>>>> , it is because:
> >>>>>>>>>>
> >>>>>>>>>> +//! struct ArcIntrusiveTimer {
> >>>>>>>>>> +//! #[pin]
> >>>>>>>>>> +//! timer: Timer<Self>,
> >>>>>>>>>> +//! #[pin]
> >>>>>>>>>> +//! flag: Mutex<bool>,
> >>>>>>>>>> +//! #[pin]
> >>>>>>>>>> +//! cond: CondVar,
> >>>>>>>>>> +//! }
> >>>>>>>>>>
> >>>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> >>>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
> >>>>>>>>>> deadlock due to interrupts:
> >>>>>>>>>>
> >>>>>>>>>> spin_lock(&a);
> >>>>>>>>>> > timer interrupt
> >>>>>>>>>> spin_lock(&a);
> >>>>>>>>>>
> >>>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
> >>>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> >>>>>>>>>> we don't support nested interrupts*).
> >>>>>>>>>
> >>>>>>>>> I updated the example based on the work in [1]. I think we need to
> >>>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
> >>>>>>>>
> >>>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
> >>>>>>>> with Boqun :)
> >>>>>>>>
> >>>>>>>> What do you think regarding the other open question: In this *special* case
> >>>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
> >>>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
> >>>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
> >>>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
> >>>>>>>> interrupt context. And with this can't be interrupted (excluding nested
> >>>>>>>> interrupts). So this modification of guard doesn't need to be protected from
> >>>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
> >>>>>>>> the interupt locked context.
> >>>>>>>>
> >>>>>>>> What do you think?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Reading while there is another CPU is writing is data-race, which is UB.
> >>>>>>
> >>>>>> Could you help to understand where exactly you see UB in Andreas' 'while
> >>>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
> >>>>>
> >>>>> Sure, but could you provide the code of what you mean exactly, if you
> >>>>> don't use a lock here, you cannot have a guard. I need to the exact code
> >>>>> to point out where the compiler may "mis-compile" (a result of being
> > [...]
> >>>> I thought we are talking about anything like
> >>>>
> >>>> #[pin_data]
> >>>> struct ArcIntrusiveTimer {
> >>>> #[pin]
> >>>> timer: Timer<Self>,
> >>>> #[pin]
> >>>> - flag: SpinLockIrq<u64>,
> >>>> + flag: u64,
> >>>> #[pin]
> >>>> cond: CondVar,
> >>>> }
> >>>>
> >>>> ?
> >>>>
> >>>
> >>> Yes, but have you tried to actually use that for the example from
> >>> Andreas? I think you will find that you cannot write to `flag` inside
> >>> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
> >>> not mutable reference for `ArcIntrusiveTimer`. You can of course use
> >>> unsafe to create a mutable reference to `flag`, but it won't be sound,
> >>> since you are getting a mutable reference from an immutable reference.
> >>
> >> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
> >> topic we discuss here? I mean we are talking about getting the compiler to
> >
> > What do you mean? If the code is unsound, you won't want to use it in an
> > example, right?
>
> Yes, sure. But ;)
>
> In a first step I just wanted to answer the question if we do need a
> lock at all in this special example. And that we could do even with
> unsound read/modify/write I would guess. And then, in a second step,
> if the answer would be "we don't need the lock", then we could think
> about how to make the flag handling sound. So I'm talking just about
> answering that question, not about the final example code. Step by step :)
>
>
> >> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
> >> should be independent on the result what we want to look at regarding the
> >> locking requirements of 'flag'?
> >>
> >> Anyhow, my root motivation was to simplify Andreas example to not use a lock
> >> where not strictly required. And with this make Andreas example independent
> >
> > Well, if you don't want to use a lock then you need to use atomics,
> > otherwise it's likely a UB,
>
> And here we are back to the initial question :) Why would it be UB
> without lock (and atomics)?
>
> Some (pseudo) assembly:
>
> Lets start with the main thread:
>
> ldr x1, [x0]
> <work with x1>
>
> x0 and x1 are registers. x0 contains the address of flag in the main
> memory. I.e. that instruction reads (ldr == load) the content of that
> memory location (flag) into x1. x1 then contains flag which can be
> used then. This is what I mean with "the main thread is read only". If
> flag, i.e. x1, does contain the old or new flag value doesn't matter.
> I.e. for the read only operation it doesn't matter if it is protected
> by a lock as the load (ldr) can't be interrupted.
If the compiler generates a single load, then sure. But for an
unsynchronized load, the compiler may generate two separate load
instructions and assume that both loads read the same value.
Alice
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-14 9:38 ` Alice Ryhl
@ 2024-10-14 11:53 ` Dirk Behme
2024-10-14 11:58 ` Alice Ryhl
0 siblings, 1 reply; 59+ messages in thread
From: Dirk Behme @ 2024-10-14 11:53 UTC (permalink / raw)
To: Alice Ryhl
Cc: Boqun Feng, Andreas Hindborg, Lyude Paul, Dirk Behme,
Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, rust-for-linux, linux-kernel
Hi Alice,
On 14.10.24 11:38, Alice Ryhl wrote:
> On Mon, Oct 14, 2024 at 8:58 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>>
>> On 13.10.24 23:06, Boqun Feng wrote:
>>> On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
>>>> On 13.10.24 00:26, Boqun Feng wrote:
>>>>> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
>>>>>> On 12.10.24 09:41, Boqun Feng wrote:
>>>>>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
>>>>>>>> On 12.10.24 01:21, Boqun Feng wrote:
>>>>>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
>>>>>>>>>> Hi Andreas,
>>>>>>>>>>
>>>>>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
>>>>>>>>>>>
>>>>>>>>>>> Dirk, thanks for reporting!
>>>>>>>>>>
>>>>>>>>>> :)
>>>>>>>>>>
>>>>>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>>>>>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
>>>>>>>>>>>>>> ease the review process a bit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>>>>>>>>>>>>> Example from hrtimer.rs.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is from lockdep:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>>>>>>>>>>>>
>>>>>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>>>>>>>>>>>>> interrupt context? Or a more subtle one?
>>>>>>>>>>>>
>>>>>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
>>>>>>>>>>>> callstack:
>>>>>>>>>>>>
>>>>>>>>>>>> ] __mutex_lock+0xa0/0xa4
>>>>>>>>>>>> ] ...
>>>>>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
>>>>>>>>>>>>
>>>>>>>>>>>> , it is because:
>>>>>>>>>>>>
>>>>>>>>>>>> +//! struct ArcIntrusiveTimer {
>>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>>> +//! timer: Timer<Self>,
>>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>>> +//! flag: Mutex<bool>,
>>>>>>>>>>>> +//! #[pin]
>>>>>>>>>>>> +//! cond: CondVar,
>>>>>>>>>>>> +//! }
>>>>>>>>>>>>
>>>>>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
>>>>>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
>>>>>>>>>>>> deadlock due to interrupts:
>>>>>>>>>>>>
>>>>>>>>>>>> spin_lock(&a);
>>>>>>>>>>>> > timer interrupt
>>>>>>>>>>>> spin_lock(&a);
>>>>>>>>>>>>
>>>>>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
>>>>>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
>>>>>>>>>>>> we don't support nested interrupts*).
>>>>>>>>>>>
>>>>>>>>>>> I updated the example based on the work in [1]. I think we need to
>>>>>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
>>>>>>>>>>
>>>>>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
>>>>>>>>>> with Boqun :)
>>>>>>>>>>
>>>>>>>>>> What do you think regarding the other open question: In this *special* case
>>>>>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
>>>>>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
>>>>>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
>>>>>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
>>>>>>>>>> interrupt context. And with this can't be interrupted (excluding nested
>>>>>>>>>> interrupts). So this modification of guard doesn't need to be protected from
>>>>>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
>>>>>>>>>> the interupt locked context.
>>>>>>>>>>
>>>>>>>>>> What do you think?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Reading while there is another CPU is writing is data-race, which is UB.
>>>>>>>>
>>>>>>>> Could you help to understand where exactly you see UB in Andreas' 'while
>>>>>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
>>>>>>>
>>>>>>> Sure, but could you provide the code of what you mean exactly, if you
>>>>>>> don't use a lock here, you cannot have a guard. I need to the exact code
>>>>>>> to point out where the compiler may "mis-compile" (a result of being
>>> [...]
>>>>>> I thought we are talking about anything like
>>>>>>
>>>>>> #[pin_data]
>>>>>> struct ArcIntrusiveTimer {
>>>>>> #[pin]
>>>>>> timer: Timer<Self>,
>>>>>> #[pin]
>>>>>> - flag: SpinLockIrq<u64>,
>>>>>> + flag: u64,
>>>>>> #[pin]
>>>>>> cond: CondVar,
>>>>>> }
>>>>>>
>>>>>> ?
>>>>>>
>>>>>
>>>>> Yes, but have you tried to actually use that for the example from
>>>>> Andreas? I think you will find that you cannot write to `flag` inside
>>>>> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
>>>>> not mutable reference for `ArcIntrusiveTimer`. You can of course use
>>>>> unsafe to create a mutable reference to `flag`, but it won't be sound,
>>>>> since you are getting a mutable reference from an immutable reference.
>>>>
>>>> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
>>>> topic we discuss here? I mean we are talking about getting the compiler to
>>>
>>> What do you mean? If the code is unsound, you won't want to use it in an
>>> example, right?
>>
>> Yes, sure. But ;)
>>
>> In a first step I just wanted to answer the question if we do need a
>> lock at all in this special example. And that we could do even with
>> unsound read/modify/write I would guess. And then, in a second step,
>> if the answer would be "we don't need the lock", then we could think
>> about how to make the flag handling sound. So I'm talking just about
>> answering that question, not about the final example code. Step by step :)
>>
>>
>>>> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
>>>> should be independent on the result what we want to look at regarding the
>>>> locking requirements of 'flag'?
>>>>
>>>> Anyhow, my root motivation was to simplify Andreas example to not use a lock
>>>> where not strictly required. And with this make Andreas example independent
>>>
>>> Well, if you don't want to use a lock then you need to use atomics,
>>> otherwise it's likely a UB,
>>
>> And here we are back to the initial question :) Why would it be UB
>> without lock (and atomics)?
>>
>> Some (pseudo) assembly:
>>
>> Lets start with the main thread:
>>
>> ldr x1, [x0]
>> <work with x1>
>>
>> x0 and x1 are registers. x0 contains the address of flag in the main
>> memory. I.e. that instruction reads (ldr == load) the content of that
>> memory location (flag) into x1. x1 then contains flag which can be
>> used then. This is what I mean with "the main thread is read only". If
>> flag, i.e. x1, does contain the old or new flag value doesn't matter.
>> I.e. for the read only operation it doesn't matter if it is protected
>> by a lock as the load (ldr) can't be interrupted.
>
> If the compiler generates a single load, then sure.
Yes :)
> But for an
> unsynchronized load, the compiler may generate two separate load
> instructions and assume that both loads read the same value.
Ok, yes, if we get this from the compiler I agree that we need the
lock, even if its just for the read. If I get the chance the next time
I will try to have a look to the compiler's result to get a better
idea of this.
Many thanks
Dirk
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 00/14] hrtimer Rust API
2024-10-14 11:53 ` Dirk Behme
@ 2024-10-14 11:58 ` Alice Ryhl
0 siblings, 0 replies; 59+ messages in thread
From: Alice Ryhl @ 2024-10-14 11:58 UTC (permalink / raw)
To: Dirk Behme
Cc: Boqun Feng, Andreas Hindborg, Lyude Paul, Dirk Behme,
Miguel Ojeda, Alex Gaynor, Anna-Maria Behnsen,
Frederic Weisbecker, Thomas Gleixner, Gary Guo,
Björn Roy Baron, Benno Lossin, rust-for-linux, linux-kernel
On Mon, Oct 14, 2024 at 1:53 PM Dirk Behme <dirk.behme@gmail.com> wrote:
>
> Hi Alice,
>
> On 14.10.24 11:38, Alice Ryhl wrote:
> > On Mon, Oct 14, 2024 at 8:58 AM Dirk Behme <dirk.behme@gmail.com> wrote:
> >>
> >> On 13.10.24 23:06, Boqun Feng wrote:
> >>> On Sun, Oct 13, 2024 at 07:39:29PM +0200, Dirk Behme wrote:
> >>>> On 13.10.24 00:26, Boqun Feng wrote:
> >>>>> On Sat, Oct 12, 2024 at 09:50:00AM +0200, Dirk Behme wrote:
> >>>>>> On 12.10.24 09:41, Boqun Feng wrote:
> >>>>>>> On Sat, Oct 12, 2024 at 07:19:41AM +0200, Dirk Behme wrote:
> >>>>>>>> On 12.10.24 01:21, Boqun Feng wrote:
> >>>>>>>>> On Fri, Oct 11, 2024 at 05:43:57PM +0200, Dirk Behme wrote:
> >>>>>>>>>> Hi Andreas,
> >>>>>>>>>>
> >>>>>>>>>> Am 11.10.24 um 16:52 schrieb Andreas Hindborg:
> >>>>>>>>>>>
> >>>>>>>>>>> Dirk, thanks for reporting!
> >>>>>>>>>>
> >>>>>>>>>> :)
> >>>>>>>>>>
> >>>>>>>>>>> Boqun Feng <boqun.feng@gmail.com> writes:
> >>>>>>>>>>>
> >>>>>>>>>>>> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
> >>>>>>>>>>>>> On 18.09.2024 00:27, Andreas Hindborg wrote:
> >>>>>>>>>>>>>> Hi!
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This series adds support for using the `hrtimer` subsystem from Rust code.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I tried breaking up the code in some smaller patches, hopefully that will
> >>>>>>>>>>>>>> ease the review process a bit.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
> >>>>>>>>>>>>> Example from hrtimer.rs.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This is from lockdep:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
> >>>>>>>>>>>>> interrupt context? Or a more subtle one?
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think it's calling mutex inside an interrupt context as shown by the
> >>>>>>>>>>>> callstack:
> >>>>>>>>>>>>
> >>>>>>>>>>>> ] __mutex_lock+0xa0/0xa4
> >>>>>>>>>>>> ] ...
> >>>>>>>>>>>> ] hrtimer_interrupt+0x1d4/0x2ac
> >>>>>>>>>>>>
> >>>>>>>>>>>> , it is because:
> >>>>>>>>>>>>
> >>>>>>>>>>>> +//! struct ArcIntrusiveTimer {
> >>>>>>>>>>>> +//! #[pin]
> >>>>>>>>>>>> +//! timer: Timer<Self>,
> >>>>>>>>>>>> +//! #[pin]
> >>>>>>>>>>>> +//! flag: Mutex<bool>,
> >>>>>>>>>>>> +//! #[pin]
> >>>>>>>>>>>> +//! cond: CondVar,
> >>>>>>>>>>>> +//! }
> >>>>>>>>>>>>
> >>>>>>>>>>>> has a Mutex<bool>, which actually should be a SpinLockIrq [1]. Note that
> >>>>>>>>>>>> irq-off is needed for the lock, because otherwise we will hit a self
> >>>>>>>>>>>> deadlock due to interrupts:
> >>>>>>>>>>>>
> >>>>>>>>>>>> spin_lock(&a);
> >>>>>>>>>>>> > timer interrupt
> >>>>>>>>>>>> spin_lock(&a);
> >>>>>>>>>>>>
> >>>>>>>>>>>> Also notice that the IrqDisabled<'_> token can be simply created by
> >>>>>>>>>>>> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> >>>>>>>>>>>> we don't support nested interrupts*).
> >>>>>>>>>>>
> >>>>>>>>>>> I updated the example based on the work in [1]. I think we need to
> >>>>>>>>>>> update `CondVar::wait` to support waiting with irq disabled.
> >>>>>>>>>>
> >>>>>>>>>> Yes, I agree. This answers one of the open questions I had in the discussion
> >>>>>>>>>> with Boqun :)
> >>>>>>>>>>
> >>>>>>>>>> What do you think regarding the other open question: In this *special* case
> >>>>>>>>>> here, what do you think to go *without* any lock? I mean the 'while *guard
> >>>>>>>>>> != 5' loop in the main thread is read only regarding guard. So it doesn't
> >>>>>>>>>> matter if it *reads* the old or the new value. And the read/modify/write of
> >>>>>>>>>> guard in the callback is done with interrupts disabled anyhow as it runs in
> >>>>>>>>>> interrupt context. And with this can't be interrupted (excluding nested
> >>>>>>>>>> interrupts). So this modification of guard doesn't need to be protected from
> >>>>>>>>>> being interrupted by a lock if there is no modifcation of guard "outside"
> >>>>>>>>>> the interupt locked context.
> >>>>>>>>>>
> >>>>>>>>>> What do you think?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Reading while there is another CPU is writing is data-race, which is UB.
> >>>>>>>>
> >>>>>>>> Could you help to understand where exactly you see UB in Andreas' 'while
> >>>>>>>> *guard != 5' loop in case no locking is used? As mentioned I'm under the
> >>>>>>>
> >>>>>>> Sure, but could you provide the code of what you mean exactly, if you
> >>>>>>> don't use a lock here, you cannot have a guard. I need to the exact code
> >>>>>>> to point out where the compiler may "mis-compile" (a result of being
> >>> [...]
> >>>>>> I thought we are talking about anything like
> >>>>>>
> >>>>>> #[pin_data]
> >>>>>> struct ArcIntrusiveTimer {
> >>>>>> #[pin]
> >>>>>> timer: Timer<Self>,
> >>>>>> #[pin]
> >>>>>> - flag: SpinLockIrq<u64>,
> >>>>>> + flag: u64,
> >>>>>> #[pin]
> >>>>>> cond: CondVar,
> >>>>>> }
> >>>>>>
> >>>>>> ?
> >>>>>>
> >>>>>
> >>>>> Yes, but have you tried to actually use that for the example from
> >>>>> Andreas? I think you will find that you cannot write to `flag` inside
> >>>>> the timer callback, because you only has a `Arc<ArcIntrusiveTimer>`, so
> >>>>> not mutable reference for `ArcIntrusiveTimer`. You can of course use
> >>>>> unsafe to create a mutable reference to `flag`, but it won't be sound,
> >>>>> since you are getting a mutable reference from an immutable reference.
> >>>>
> >>>> Yes, of course. But, hmm, wouldn't that unsoundness be independent on the
> >>>> topic we discuss here? I mean we are talking about getting the compiler to
> >>>
> >>> What do you mean? If the code is unsound, you won't want to use it in an
> >>> example, right?
> >>
> >> Yes, sure. But ;)
> >>
> >> In a first step I just wanted to answer the question if we do need a
> >> lock at all in this special example. And that we could do even with
> >> unsound read/modify/write I would guess. And then, in a second step,
> >> if the answer would be "we don't need the lock", then we could think
> >> about how to make the flag handling sound. So I'm talking just about
> >> answering that question, not about the final example code. Step by step :)
> >>
> >>
> >>>> read/modify/write 'flag' in the TimerCallback. *How* we tell him to do so
> >>>> should be independent on the result what we want to look at regarding the
> >>>> locking requirements of 'flag'?
> >>>>
> >>>> Anyhow, my root motivation was to simplify Andreas example to not use a lock
> >>>> where not strictly required. And with this make Andreas example independent
> >>>
> >>> Well, if you don't want to use a lock then you need to use atomics,
> >>> otherwise it's likely a UB,
> >>
> >> And here we are back to the initial question :) Why would it be UB
> >> without lock (and atomics)?
> >>
> >> Some (pseudo) assembly:
> >>
> >> Lets start with the main thread:
> >>
> >> ldr x1, [x0]
> >> <work with x1>
> >>
> >> x0 and x1 are registers. x0 contains the address of flag in the main
> >> memory. I.e. that instruction reads (ldr == load) the content of that
> >> memory location (flag) into x1. x1 then contains flag which can be
> >> used then. This is what I mean with "the main thread is read only". If
> >> flag, i.e. x1, does contain the old or new flag value doesn't matter.
> >> I.e. for the read only operation it doesn't matter if it is protected
> >> by a lock as the load (ldr) can't be interrupted.
> >
> > If the compiler generates a single load, then sure.
>
> Yes :)
>
> > But for an
> > unsynchronized load, the compiler may generate two separate load
> > instructions and assume that both loads read the same value.
>
> Ok, yes, if we get this from the compiler I agree that we need the
> lock, even if its just for the read. If I get the chance the next time
> I will try to have a look to the compiler's result to get a better
> idea of this.
Usually I would say that for cases like this, the correct approach is
to use relaxed atomic loads and stores. That compiles down to ordinary
load/store instructions as desired without letting the compiler split
the load.
Alice
^ permalink raw reply [flat|nested] 59+ messages in thread
end of thread, other threads:[~2024-10-14 11:59 UTC | newest]
Thread overview: 59+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-17 22:27 [PATCH v2 00/14] hrtimer Rust API Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 01/14] rust: time: Add Ktime::from_ns() Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support Andreas Hindborg
2024-09-18 18:13 ` Benno Lossin
2024-09-19 5:43 ` Andreas Hindborg
2024-09-19 14:09 ` Benno Lossin
2024-09-23 16:35 ` Andreas Hindborg
2024-09-23 16:59 ` Benno Lossin
2024-10-10 12:24 ` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 03/14] rust: sync: add `Arc::as_ptr` Andreas Hindborg
2024-09-19 14:03 ` Benno Lossin
2024-09-21 15:58 ` Gary Guo
2024-09-21 18:17 ` Benno Lossin
2024-09-23 8:14 ` Alice Ryhl
2024-10-01 4:56 ` Dirk Behme
2024-10-01 8:39 ` Benno Lossin
2024-09-17 22:27 ` [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw` Andreas Hindborg
2024-09-18 18:19 ` Benno Lossin
2024-09-18 20:12 ` Gary Guo
2024-09-18 21:09 ` Benno Lossin
2024-09-19 6:00 ` Andreas Hindborg
2024-09-19 14:15 ` Benno Lossin
2024-09-20 8:25 ` Andreas Hindborg
2024-09-19 5:54 ` Andreas Hindborg
2024-09-19 6:19 ` Andreas Hindborg
2024-09-19 6:41 ` Alice Ryhl
2024-09-17 22:27 ` [PATCH v2 05/14] rust: hrtimer: implement `TimerPointer` for `Arc` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 06/14] rust: hrtimer: allow timer restart from timer handler Andreas Hindborg
2024-09-20 14:25 ` kernel test robot
2024-09-17 22:27 ` [PATCH v2 07/14] rust: hrtimer: add `UnsafeTimerPointer` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 08/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 09/14] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&mut T>` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 10/14] rust: hrtimer: add `hrtimer::ScopedTimerPointer` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 11/14] rust: hrtimer: allow specifying a distinct callback parameter Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 12/14] rust: hrtimer: implement `TimerPointer` for `Pin<Box<T>>` Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 13/14] rust: hrtimer: add `schedule_function` to schedule closures Andreas Hindborg
2024-09-17 22:27 ` [PATCH v2 14/14] rust: hrtimer: add maintainer entry Andreas Hindborg
2024-10-12 15:19 ` Boqun Feng
2024-09-30 9:36 ` [PATCH v2 00/14] hrtimer Rust API Anna-Maria Behnsen
2024-10-04 10:47 ` Andreas Hindborg
2024-10-01 12:37 ` Dirk Behme
2024-10-01 14:42 ` Boqun Feng
2024-10-03 8:14 ` Dirk Behme
2024-10-03 13:03 ` Boqun Feng
2024-10-03 16:18 ` Dirk Behme
2024-10-11 14:52 ` Andreas Hindborg
2024-10-11 15:43 ` Dirk Behme
2024-10-11 23:21 ` Boqun Feng
2024-10-12 5:19 ` Dirk Behme
2024-10-12 7:41 ` Boqun Feng
2024-10-12 7:50 ` Dirk Behme
2024-10-12 22:26 ` Boqun Feng
2024-10-13 17:39 ` Dirk Behme
2024-10-13 21:06 ` Boqun Feng
2024-10-14 6:58 ` Dirk Behme
2024-10-14 9:17 ` Andreas Hindborg
2024-10-14 9:38 ` Alice Ryhl
2024-10-14 11:53 ` Dirk Behme
2024-10-14 11:58 ` Alice Ryhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).