From: Andreas Hindborg <[email protected]>
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 <[email protected]>
---
This patch is a dependency for the Rust null block driver [1].
Link: https://lore.kernel.org/rust-for-linux/[email protected]/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
2 files changed, 284 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..1e282608e70c
--- /dev/null
+++ b/rust/kernel/hrtimer.rs
@@ -0,0 +1,283 @@
+// 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.
+//!
+//! # Example
+//!
+//! ```rust
+//! use kernel::{
+//! sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
+//! impl_has_timer, prelude::*, stack_pin_init
+//! };
+//! use core::sync::atomic::AtomicBool;
+//! use core::sync::atomic::Ordering;
+//!
+//! #[pin_data]
+//! struct IntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! flag: AtomicBool,
+//! }
+//!
+//! impl IntrusiveTimer {
+//! fn new() -> impl PinInit<Self> {
+//! pin_init!(Self {
+//! timer <- Timer::new(),
+//! flag: AtomicBool::new(false),
+//! })
+//! }
+//! }
+//!
+//! impl TimerCallback for IntrusiveTimer {
+//! type Receiver = Arc<IntrusiveTimer>;
+//!
+//! fn run(this: Self::Receiver) {
+//! pr_info!("Timer called\n");
+//! this.flag.store(true, Ordering::Relaxed);
+//! }
+//! }
+//!
+//! impl_has_timer! {
+//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
+//! }
+//!
+//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
+//! has_timer.clone().schedule(200_000_000);
+//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
+//!
+//! pr_info!("Flag raised\n");
+//!
+//! # Ok::<(), kernel::error::Error>(())
+//! ```
+//!
+//! C header: [`include/linux/hrtimer.h`](srctree/include/linux/hrtimer.h)
+
+use core::{marker::PhantomData, pin::Pin};
+
+use crate::{init::PinInit, prelude::*, sync::Arc, types::Opaque};
+
+/// A timer backed by a C `struct hrtimer`
+///
+/// # Invariants
+///
+/// * `self.timer` is initialized by `bindings::hrtimer_init`.
+#[repr(transparent)]
+#[pin_data(PinnedDrop)]
+pub struct Timer<T> {
+ #[pin]
+ timer: Opaque<bindings::hrtimer>,
+ _t: PhantomData<T>,
+}
+
+// SAFETY: A `Timer` can be moved to other threads and used from there.
+unsafe impl<T> Send for Timer<T> {}
+
+// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
+// timer from multiple threads
+unsafe impl<T> Sync for Timer<T> {}
+
+impl<T: TimerCallback> Timer<T> {
+ /// Return an initializer for a new timer instance.
+ pub fn new() -> impl PinInit<Self> {
+ crate::pin_init!( Self {
+ 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. The `function` field might not be initialized, but
+ // `addr_of_mut` does not create a reference to the field.
+ let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
+
+ // SAFETY: `function` points to a valid allocation.
+ unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
+ }),
+ _t: PhantomData,
+ })
+ }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Timer<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: By struct invariant `self.timer` was initialized by
+ // `hrtimer_init` so by C API contract it is safe to call
+ // `hrtimer_cancel`.
+ unsafe {
+ bindings::hrtimer_cancel(self.timer.get());
+ }
+ }
+}
+
+/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
+/// facilitates queueing the timer through the pointer that implements the
+/// trait.
+///
+/// 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.
+///
+/// [`Box<T>`]: Box
+/// [`Arc<T>`]: Arc
+/// [`ARef<T>`]: crate::types::ARef
+pub trait RawTimer: Sync {
+ /// Schedule the timer after `expires` time units
+ fn schedule(self, expires: u64);
+}
+
+/// 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<T> {
+ /// 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<T> {
+ // 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<T>>() }
+ }
+
+ /// Return a pointer to the struct that is embedding the [`Timer`] pointed
+ /// to by `ptr`.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a [`Timer<T>`] field in a struct of type `Self`.
+ unsafe fn timer_container_of(ptr: *mut Timer<T>) -> *mut Self
+ where
+ Self: Sized,
+ {
+ // SAFETY: By the safety requirement of this trait, the trait
+ // implementor will have a `Timer` field at the specified offset.
+ unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
+ }
+}
+
+/// Implemented by pointer types that can be the target of a C timer callback.
+pub trait RawTimerCallback: RawTimer {
+ /// Callback to be called from C.
+ ///
+ /// # Safety
+ ///
+ /// Only to be called by C code in `hrtimer`subsystem.
+ unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+}
+
+/// Implemented by pointers to structs that can the target of a timer callback
+pub trait TimerCallback {
+ /// Type of `this` argument for `run()`.
+ type Receiver: RawTimerCallback;
+
+ /// Called by the timer logic when the timer fires
+ fn run(this: Self::Receiver);
+}
+
+impl<T> RawTimer for Arc<T>
+where
+ T: Send + Sync,
+ T: HasTimer<T>,
+{
+ fn schedule(self, expires: u64) {
+ let self_ptr = Arc::into_raw(self);
+
+ // SAFETY: `self_ptr` is a valid pointer to a `T`
+ let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
+
+ // `Timer` is `repr(transparent)`
+ let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
+
+ // Schedule the timer - if it is already scheduled it is removed and
+ // inserted
+
+ // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
+ // initialized by `hrtimer_init`
+ unsafe {
+ bindings::hrtimer_start_range_ns(
+ c_timer_ptr.cast_mut(),
+ expires as i64,
+ 0,
+ bindings::hrtimer_mode_HRTIMER_MODE_REL,
+ );
+ }
+ }
+}
+
+impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
+where
+ T: Send + Sync,
+ T: HasTimer<T>,
+ T: TimerCallback<Receiver = 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<T>>();
+
+ // SAFETY: By C API contract `ptr` is the pointer we passed when
+ // enqueing the timer, so it is a `Timer<T>` embedded in a `T`
+ let data_ptr = unsafe { T::timer_container_of(timer_ptr) };
+
+ // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
+ let receiver = unsafe { Arc::from_raw(data_ptr) };
+
+ T::run(receiver);
+
+ bindings::hrtimer_restart_HRTIMER_NORESTART
+ }
+}
+
+/// Use to implement the [`HasTimer<T>`] trait.
+///
+/// See [`module`] documentation for an example.
+///
+/// [`module`]: crate::hrtimer
+#[macro_export]
+macro_rules! impl_has_timer {
+ ($(impl$(<$($implarg:ident),*>)?
+ HasTimer<$timer_type:ty $(, $id:tt)?>
+ for $self:ident $(<$($selfarg:ident),*>)?
+ { self.$field:ident }
+ )*) => {$(
+ // SAFETY: This implementation of `raw_get_timer` only compiles if the
+ // field has the right type.
+ unsafe impl$(<$($implarg),*>)? $crate::hrtimer::HasTimer<$timer_type> for $self $(<$($selfarg),*>)? {
+ 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 $(, $id)?> {
+ // 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 be68d5e567b1..7af3ca601167 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -33,6 +33,7 @@
mod allocator;
mod build_assert;
pub mod error;
+pub mod hrtimer;
pub mod init;
pub mod ioctl;
#[cfg(CONFIG_KUNIT)]
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
--
2.44.0
On 25.04.24 11:46, Andreas Hindborg wrote:
> From: Andreas Hindborg <[email protected]>
>
> 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 <[email protected]>
>
> ---
>
> This patch is a dependency for the Rust null block driver [1].
>
> Link: https://lore.kernel.org/rust-for-linux/[email protected]/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
>
> rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 2 files changed, 284 insertions(+)
> create mode 100644 rust/kernel/hrtimer.rs
>
> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
Hmm is this the right place? I imagine there are other timers, does this
fit better into the `time` module (ie make `hrtimer` a submodule of
`time`) or should we later introduce a `timer` parent module?
> new file mode 100644
> index 000000000000..1e282608e70c
> --- /dev/null
> +++ b/rust/kernel/hrtimer.rs
> @@ -0,0 +1,283 @@
> +// 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.
> +//!
> +//! # Example
> +//!
> +//! ```rust
> +//! use kernel::{
> +//! sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
> +//! impl_has_timer, prelude::*, stack_pin_init
> +//! };
> +//! use core::sync::atomic::AtomicBool;
> +//! use core::sync::atomic::Ordering;
> +//!
> +//! #[pin_data]
> +//! struct IntrusiveTimer {
> +//! #[pin]
> +//! timer: Timer<Self>,
> +//! flag: AtomicBool,
> +//! }
> +//!
> +//! impl IntrusiveTimer {
> +//! fn new() -> impl PinInit<Self> {
> +//! pin_init!(Self {
> +//! timer <- Timer::new(),
> +//! flag: AtomicBool::new(false),
> +//! })
> +//! }
> +//! }
> +//!
> +//! impl TimerCallback for IntrusiveTimer {
> +//! type Receiver = Arc<IntrusiveTimer>;
> +//!
> +//! fn run(this: Self::Receiver) {
> +//! pr_info!("Timer called\n");
> +//! this.flag.store(true, Ordering::Relaxed);
> +//! }
> +//! }
> +//!
> +//! impl_has_timer! {
> +//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
> +//! }
> +//!
> +//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
I would not name this variable `has_timer`. Maybe `my_timer` is better?
> +//! has_timer.clone().schedule(200_000_000);
> +//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
Weird formatting, we should also use `rustfmt` in examples.
> +//!
> +//! pr_info!("Flag raised\n");
> +//!
> +//! # Ok::<(), kernel::error::Error>(())
> +//! ```
> +//!
> +//! C header: [`include/linux/hrtimer.h`](srctree/include/linux/hrtimer.h)
> +
> +use core::{marker::PhantomData, pin::Pin};
> +
> +use crate::{init::PinInit, prelude::*, sync::Arc, types::Opaque};
> +
> +/// A timer backed by a C `struct hrtimer`
Missing `.` at the end, this also occurs below.
> +///
> +/// # Invariants
> +///
> +/// * `self.timer` is initialized by `bindings::hrtimer_init`.
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +pub struct Timer<T> {
> + #[pin]
> + timer: Opaque<bindings::hrtimer>,
> + _t: PhantomData<T>,
> +}
> +
> +// SAFETY: A `Timer` can be moved to other threads and used from there.
> +unsafe impl<T> Send for Timer<T> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
> +unsafe impl<T> Sync for Timer<T> {}
> +
> +impl<T: TimerCallback> Timer<T> {
> + /// Return an initializer for a new timer instance.
> + pub fn new() -> impl PinInit<Self> {
> + crate::pin_init!( Self {
`pin_init!` is in the prelude, no need to prefix with `crate`.
> + 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. The `function` field might not be initialized, but
> + // `addr_of_mut` does not create a reference to the field.
> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
> +
> + // SAFETY: `function` points to a valid allocation.
> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
> + }),
> + _t: PhantomData,
> + })
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T> PinnedDrop for Timer<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: By struct invariant `self.timer` was initialized by
> + // `hrtimer_init` so by C API contract it is safe to call
> + // `hrtimer_cancel`.
> + unsafe {
> + bindings::hrtimer_cancel(self.timer.get());
> + }
> + }
> +}
Why is this needed? The only way to schedule a timer using this API is
by having an `Arc` with a timer-containing struct inside. But to
schedule the `Arc`, you consume one refcount which is then sent to the
timer subsystem. So it is impossible for the refcount to drop below zero
while the timer is scheduled, but not yet running.
Do you need to call `hrtimer_cancel` after/while a timer is running?
Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
that can happen when the timer callback owns the last refcount.
> +
> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
> +/// facilitates queueing the timer through the pointer that implements the
> +/// trait.
> +///
> +/// 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.
> +///
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait RawTimer: Sync {
> + /// Schedule the timer after `expires` time units
> + fn schedule(self, expires: u64);
> +}
> +
> +/// 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<T> {
> + /// 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<T> {
> + // 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<T>>() }
> + }
> +
> + /// Return a pointer to the struct that is embedding the [`Timer`] pointed
> + /// to by `ptr`.
> + ///
> + /// # Safety
> + ///
> + /// `ptr` must point to a [`Timer<T>`] field in a struct of type `Self`.
> + unsafe fn timer_container_of(ptr: *mut Timer<T>) -> *mut Self
> + where
> + Self: Sized,
> + {
> + // SAFETY: By the safety requirement of this trait, the trait
> + // implementor will have a `Timer` field at the specified offset.
> + unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
> + }
> +}
> +
> +/// Implemented by pointer types that can be the target of a C timer callback.
> +pub trait RawTimerCallback: RawTimer {
Do you really need two different traits? Can't we have a single
`TimerPointer` trait that does both `RawTimerCallback` and `RawTimer`?
(I am also wondering why we did this for workqueue)
> + /// Callback to be called from C.
> + ///
> + /// # Safety
> + ///
> + /// Only to be called by C code in `hrtimer`subsystem.
> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by pointers to structs that can the target of a timer callback
> +pub trait TimerCallback {
> + /// Type of `this` argument for `run()`.
> + type Receiver: RawTimerCallback;
> +
> + /// Called by the timer logic when the timer fires
> + fn run(this: Self::Receiver);
> +}
> +
> +impl<T> RawTimer for Arc<T>
> +where
> + T: Send + Sync,
> + T: HasTimer<T>,
> +{
> + fn schedule(self, expires: u64) {
> + let self_ptr = Arc::into_raw(self);
> +
> + // SAFETY: `self_ptr` is a valid pointer to a `T`
> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
> +
> + // `Timer` is `repr(transparent)`
> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
> +
> + // Schedule the timer - if it is already scheduled it is removed and
> + // inserted
> +
> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
> + // initialized by `hrtimer_init`
> + unsafe {
> + bindings::hrtimer_start_range_ns(
> + c_timer_ptr.cast_mut(),
> + expires as i64,
> + 0,
> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
> + );
> + }
> + }
> +}
> +
> +impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
Why are you spelling out the whole path?
> +where
> + T: Send + Sync,
> + T: HasTimer<T>,
> + T: TimerCallback<Receiver = 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<T>>();
> +
> + // SAFETY: By C API contract `ptr` is the pointer we passed when
> + // enqueing the timer, so it is a `Timer<T>` embedded in a `T`
> + let data_ptr = unsafe { T::timer_container_of(timer_ptr) };
> +
> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
> + let receiver = unsafe { Arc::from_raw(data_ptr) };
> +
> + T::run(receiver);
> +
> + bindings::hrtimer_restart_HRTIMER_NORESTART
> + }
> +}
> +
> +/// Use to implement the [`HasTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_timer {
> + ($(impl$(<$($implarg:ident),*>)?
Doing it this way makes it impossible to use more complex types with eg
lifetimes or const generic arguments. There is a workaround, see Alice's
linked list patch [1]:
macro_rules! impl_list_item {
(
impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
using ListLinks;
} $($rest:tt)*
) => {
[1]: https://lore.kernel.org/rust-for-linux/[email protected]/
> + HasTimer<$timer_type:ty $(, $id:tt)?>
`HasTimer` currently doesn't have an `id`, so the macro also shouldn't
have it.
--
Cheers,
Benno
> + for $self:ident $(<$($selfarg:ident),*>)?
> + { self.$field:ident }
> + )*) => {$(
> + // SAFETY: This implementation of `raw_get_timer` only compiles if the
> + // field has the right type.
> + unsafe impl$(<$($implarg),*>)? $crate::hrtimer::HasTimer<$timer_type> for $self $(<$($selfarg),*>)? {
> + 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 $(, $id)?> {
> + // 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 be68d5e567b1..7af3ca601167 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -33,6 +33,7 @@
> mod allocator;
> mod build_assert;
> pub mod error;
> +pub mod hrtimer;
> pub mod init;
> pub mod ioctl;
> #[cfg(CONFIG_KUNIT)]
>
> base-commit: ed30a4a51bb196781c8058073ea720133a65596f
> --
> 2.44.0
>
Benno Lossin <[email protected]> writes:
> On 25.04.24 11:46, Andreas Hindborg wrote:
>> From: Andreas Hindborg <[email protected]>
>>
>> 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 <[email protected]>
>>
>> ---
>>
>> This patch is a dependency for the Rust null block driver [1].
>>
>> Link: https://lore.kernel.org/rust-for-linux/[email protected]/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
>>
>> rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs | 1 +
>> 2 files changed, 284 insertions(+)
>> create mode 100644 rust/kernel/hrtimer.rs
>>
>> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
>
> Hmm is this the right place? I imagine there are other timers, does this
> fit better into the `time` module (ie make `hrtimer` a submodule of
> `time`) or should we later introduce a `timer` parent module?
We can always move it. We will move stuff anyway when the kernel crate
is split.
We can also take it to `kernel::time::hrtimer` now, either way is fine.
>
>> new file mode 100644
>> index 000000000000..1e282608e70c
>> --- /dev/null
>> +++ b/rust/kernel/hrtimer.rs
>> @@ -0,0 +1,283 @@
>> +// 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.
>> +//!
>> +//! # Example
>> +//!
>> +//! ```rust
>> +//! use kernel::{
>> +//! sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
>> +//! impl_has_timer, prelude::*, stack_pin_init
>> +//! };
>> +//! use core::sync::atomic::AtomicBool;
>> +//! use core::sync::atomic::Ordering;
>> +//!
>> +//! #[pin_data]
>> +//! struct IntrusiveTimer {
>> +//! #[pin]
>> +//! timer: Timer<Self>,
>> +//! flag: AtomicBool,
>> +//! }
>> +//!
>> +//! impl IntrusiveTimer {
>> +//! fn new() -> impl PinInit<Self> {
>> +//! pin_init!(Self {
>> +//! timer <- Timer::new(),
>> +//! flag: AtomicBool::new(false),
>> +//! })
>> +//! }
>> +//! }
>> +//!
>> +//! impl TimerCallback for IntrusiveTimer {
>> +//! type Receiver = Arc<IntrusiveTimer>;
>> +//!
>> +//! fn run(this: Self::Receiver) {
>> +//! pr_info!("Timer called\n");
>> +//! this.flag.store(true, Ordering::Relaxed);
>> +//! }
>> +//! }
>> +//!
>> +//! impl_has_timer! {
>> +//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
>> +//! }
>> +//!
>> +//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
>
> I would not name this variable `has_timer`. Maybe `my_timer` is better?
Right, thanks.
>
>> +//! has_timer.clone().schedule(200_000_000);
>> +//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
>
> Weird formatting, we should also use `rustfmt` in examples.
`format_code_in_doc_comments` is a nightly `rustfmt` feature. I tried
enabling it in `.rustfmt.toml` and running `rustfmt +nightly
hrtimer.rs`. It did not have any effect. There is some discussion here:
https://github.com/rust-lang/rustfmt/issues/3348
>
>> +//!
>> +//! pr_info!("Flag raised\n");
>> +//!
>> +//! # Ok::<(), kernel::error::Error>(())
>> +//! ```
>> +//!
>> +//! C header: [`include/linux/hrtimer.h`](srctree/include/linux/hrtimerh)
>> +
>> +use core::{marker::PhantomData, pin::Pin};
>> +
>> +use crate::{init::PinInit, prelude::*, sync::Arc, types::Opaque};
>> +
>> +/// A timer backed by a C `struct hrtimer`
>
> Missing `.` at the end, this also occurs below.
????
>
>> +///
>> +/// # Invariants
>> +///
>> +/// * `self.timer` is initialized by `bindings::hrtimer_init`.
>> +#[repr(transparent)]
>> +#[pin_data(PinnedDrop)]
>> +pub struct Timer<T> {
>> + #[pin]
>> + timer: Opaque<bindings::hrtimer>,
>> + _t: PhantomData<T>,
>> +}
>> +
>> +// SAFETY: A `Timer` can be moved to other threads and used from there.
>> +unsafe impl<T> Send for Timer<T> {}
>> +
>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>> +// timer from multiple threads
>> +unsafe impl<T> Sync for Timer<T> {}
>> +
>> +impl<T: TimerCallback> Timer<T> {
>> + /// Return an initializer for a new timer instance.
>> + pub fn new() -> impl PinInit<Self> {
>> + crate::pin_init!( Self {
>
> `pin_init!` is in the prelude, no need to prefix with `crate`.
????
>
>> + 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. The `function` field might not be initialized, but
>> + // `addr_of_mut` does not create a reference to the field.
>> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
>> +
>> + // SAFETY: `function` points to a valid allocation.
>> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
>> + }),
>> + _t: PhantomData,
>> + })
>> + }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T> PinnedDrop for Timer<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY: By struct invariant `self.timer` was initialized by
>> + // `hrtimer_init` so by C API contract it is safe to call
>> + // `hrtimer_cancel`.
>> + unsafe {
>> + bindings::hrtimer_cancel(self.timer.get());
>> + }
>> + }
>> +}
>
> Why is this needed? The only way to schedule a timer using this API is
> by having an `Arc` with a timer-containing struct inside. But to
> schedule the `Arc`, you consume one refcount which is then sent to the
> timer subsystem. So it is impossible for the refcount to drop below zero
> while the timer is scheduled, but not yet running.
> Do you need to call `hrtimer_cancel` after/while a timer is running?
This is not required any longer. It is a leftover from an earlier
revision where timers could be stack allocated. I will remove it.
> Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
> that can happen when the timer callback owns the last refcount.
That should be fine, `self` is still valid when the drop method is run?
>
>> +
>> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
>> +/// facilitates queueing the timer through the pointer that implements the
>> +/// trait.
>> +///
>> +/// 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.
>> +///
>> +/// [`Box<T>`]: Box
>> +/// [`Arc<T>`]: Arc
>> +/// [`ARef<T>`]: crate::types::ARef
>> +pub trait RawTimer: Sync {
>> + /// Schedule the timer after `expires` time units
>> + fn schedule(self, expires: u64);
>> +}
>> +
>> +/// 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<T> {
>> + /// 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<T> {
>> + // 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<T>>() }
>> + }
>> +
>> + /// Return a pointer to the struct that is embedding the [`Timer`] pointed
>> + /// to by `ptr`.
>> + ///
>> + /// # Safety
>> + ///
>> + /// `ptr` must point to a [`Timer<T>`] field in a struct of type `Self`.
>> + unsafe fn timer_container_of(ptr: *mut Timer<T>) -> *mut Self
>> + where
>> + Self: Sized,
>> + {
>> + // SAFETY: By the safety requirement of this trait, the trait
>> + // implementor will have a `Timer` field at the specified offset.
>> + unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
>> + }
>> +}
>> +
>> +/// Implemented by pointer types that can be the target of a C timer callback.
>> +pub trait RawTimerCallback: RawTimer {
>
> Do you really need two different traits? Can't we have a single
> `TimerPointer` trait that does both `RawTimerCallback` and `RawTimer`?
> (I am also wondering why we did this for workqueue)
Let me try to merge them and see what happens.
>
>> + /// Callback to be called from C.
>> + ///
>> + /// # Safety
>> + ///
>> + /// Only to be called by C code in `hrtimer`subsystem.
>> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>> +
>> +/// Implemented by pointers to structs that can the target of a timer callback
>> +pub trait TimerCallback {
>> + /// Type of `this` argument for `run()`.
>> + type Receiver: RawTimerCallback;
>> +
>> + /// Called by the timer logic when the timer fires
>> + fn run(this: Self::Receiver);
>> +}
>> +
>> +impl<T> RawTimer for Arc<T>
>> +where
>> + T: Send + Sync,
>> + T: HasTimer<T>,
>> +{
>> + fn schedule(self, expires: u64) {
>> + let self_ptr = Arc::into_raw(self);
>> +
>> + // SAFETY: `self_ptr` is a valid pointer to a `T`
>> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
>> +
>> + // `Timer` is `repr(transparent)`
>> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
>> +
>> + // Schedule the timer - if it is already scheduled it is removed and
>> + // inserted
>> +
>> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
>> + // initialized by `hrtimer_init`
>> + unsafe {
>> + bindings::hrtimer_start_range_ns(
>> + c_timer_ptr.cast_mut(),
>> + expires as i64,
>> + 0,
>> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> + );
>> + }
>> + }
>> +}
>> +
>> +impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
>
> Why are you spelling out the whole path?
The real question to ask is why does rustfmt or the compiler not suggest
that I remove the explicit path?
>
>> +where
>> + T: Send + Sync,
>> + T: HasTimer<T>,
>> + T: TimerCallback<Receiver = 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<T>>();
>> +
>> + // SAFETY: By C API contract `ptr` is the pointer we passed when
>> + // enqueing the timer, so it is a `Timer<T>` embedded in a `T`
>> + let data_ptr = unsafe { T::timer_container_of(timer_ptr) };
>> +
>> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
>> + let receiver = unsafe { Arc::from_raw(data_ptr) };
>> +
>> + T::run(receiver);
>> +
>> + bindings::hrtimer_restart_HRTIMER_NORESTART
>> + }
>> +}
>> +
>> +/// Use to implement the [`HasTimer<T>`] trait.
>> +///
>> +/// See [`module`] documentation for an example.
>> +///
>> +/// [`module`]: crate::hrtimer
>> +#[macro_export]
>> +macro_rules! impl_has_timer {
>> + ($(impl$(<$($implarg:ident),*>)?
>
> Doing it this way makes it impossible to use more complex types with eg
> lifetimes or const generic arguments. There is a workaround, see Alice's
> linked list patch [1]:
>
> macro_rules! impl_list_item {
> (
> impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
> using ListLinks;
> } $($rest:tt)*
> ) => {
>
> [1]: https://lore.kernel.org/rust-for-linux/[email protected]/
Thanks, I will take a look.
>
>> + HasTimer<$timer_type:ty $(, $id:tt)?>
>
> `HasTimer` currently doesn't have an `id`, so the macro also shouldn't
> have it.
Thanks, I guess inheritance of this macro is very evident :)
Thanks for the comments!
BR Andreas
Andreas Hindborg <[email protected]> writes:
> From: Andreas Hindborg <[email protected]>
>
> 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 <[email protected]>
This patch is very similar to the workqueue I implemented. It seems like
we have the following correspondence between them:
* Your HasTimer is my HasWork.
* Your RawTimerCallback is my WorkItemPointer.
* Your TimerCallback is my WorkItem.
* Your RawTimer is my RawWorkItem. (but the match isn't great here)
I think it would make sense to have the names be more consistent. I
propose renaming RawTimerCallback to TimerCallbackPointer.
Or we can name them TimerEntry and RawTimerEntry?
I also note that the method on your RawTimer trait seems to be the
public API of how you're supposed to schedule a timer, whereas the
workqueue RawWorkItem only provides a raw low-level method, and instead
has the "public API" be a function on the Workqueue struct.
I'm not such a big fan of having the primary method everying is supposed
to use be a method on a trait whose name starts with "Raw". It's worth
considering whether it makes more sense to have a free-standing function
called `schedule_timer` and have that be how you're supposed to schedule
timers, instead of the RawTimer trait.
> +#[pinned_drop]
> +impl<T> PinnedDrop for Timer<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: By struct invariant `self.timer` was initialized by
> + // `hrtimer_init` so by C API contract it is safe to call
> + // `hrtimer_cancel`.
> + unsafe {
> + bindings::hrtimer_cancel(self.timer.get());
> + }
> + }
> +}
Assuming that this is mirroring the workqueue, then this is not
necessary. The timer owns a refcount to the element, so the destructor
cannot run while the timer is scheduled.
Also, as a generaly note, putting semicolons outside of unsafe blocks
formats better.
> +/// Implemented by pointer types that can be the target of a C timer callback.
> +pub trait RawTimerCallback: RawTimer {
> + /// Callback to be called from C.
> + ///
> + /// # Safety
> + ///
> + /// Only to be called by C code in `hrtimer`subsystem.
> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
Safety comment is missing a space.
> +/// Implemented by pointers to structs that can the target of a timer callback
> +pub trait TimerCallback {
> + /// Type of `this` argument for `run()`.
> + type Receiver: RawTimerCallback;
> +
> + /// Called by the timer logic when the timer fires
> + fn run(this: Self::Receiver);
> +}
The documentation says that this is implemented by pointers to structs,
but that is not the case.
> +impl<T> RawTimer for Arc<T>
> +where
> + T: Send + Sync,
> + T: HasTimer<T>,
> +{
> + fn schedule(self, expires: u64) {
> + let self_ptr = Arc::into_raw(self);
> +
> + // SAFETY: `self_ptr` is a valid pointer to a `T`
> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
> +
> + // `Timer` is `repr(transparent)`
> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
I would add an `raw_get` method to `Timer` instead of this cast,
analogous to `Work::raw_get`.
> + // Schedule the timer - if it is already scheduled it is removed and
> + // inserted
> +
> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
> + // initialized by `hrtimer_init`
> + unsafe {
> + bindings::hrtimer_start_range_ns(
> + c_timer_ptr.cast_mut(),
> + expires as i64,
> + 0,
> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
> + );
> + }
> + }
> +}
Alice
Alice Ryhl <[email protected]> writes:
> Andreas Hindborg <[email protected]> writes:
>> From: Andreas Hindborg <[email protected]>
>>
>> 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 <[email protected]>
>
> This patch is very similar to the workqueue I implemented. It seems like
> we have the following correspondence between them:
>
> * Your HasTimer is my HasWork.
> * Your RawTimerCallback is my WorkItemPointer.
> * Your TimerCallback is my WorkItem.
> * Your RawTimer is my RawWorkItem. (but the match isn't great here)
>
> I think it would make sense to have the names be more consistent. I
> propose renaming RawTimerCallback to TimerCallbackPointer.
>
> Or we can name them TimerEntry and RawTimerEntry?
I took some advice from Benno and merged `RawTimerCallback` with
`RawTimer` and renamed the resulting trait `TimerPointer`. There is not
really any reason they should be split for the `hrtimer` as far as I can
tell.
> I also note that the method on your RawTimer trait seems to be the
> public API of how you're supposed to schedule a timer, whereas the
> workqueue RawWorkItem only provides a raw low-level method, and instead
> has the "public API" be a function on the Workqueue struct.
>
> I'm not such a big fan of having the primary method everying is supposed
> to use be a method on a trait whose name starts with "Raw".
I would remove the `Raw`.
> It's worth
> considering whether it makes more sense to have a free-standing function
> called `schedule_timer` and have that be how you're supposed to schedule
> timers, instead of the RawTimer trait.
I think being able to call `my_timer_containing_struct.schedule()` is
nice.
>
>> +#[pinned_drop]
>> +impl<T> PinnedDrop for Timer<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY: By struct invariant `self.timer` was initialized by
>> + // `hrtimer_init` so by C API contract it is safe to call
>> + // `hrtimer_cancel`.
>> + unsafe {
>> + bindings::hrtimer_cancel(self.timer.get());
>> + }
>> + }
>> +}
>
> Assuming that this is mirroring the workqueue, then this is not
> necessary. The timer owns a refcount to the element, so the destructor
> cannot run while the timer is scheduled.
Yes, it is very much a mirror. Yes, it is a leftover from trying to
support stack allocated timers. I will remove it.
> Also, as a generaly note, putting semicolons outside of unsafe blocks
> formats better.
????
>
>> +/// Implemented by pointer types that can be the target of a C timer callback.
>> +pub trait RawTimerCallback: RawTimer {
>> + /// Callback to be called from C.
>> + ///
>> + /// # Safety
>> + ///
>> + /// Only to be called by C code in `hrtimer`subsystem.
>> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>
> Safety comment is missing a space.
Thanks.
>
>> +/// Implemented by pointers to structs that can the target of a timer callback
>> +pub trait TimerCallback {
>> + /// Type of `this` argument for `run()`.
>> + type Receiver: RawTimerCallback;
>> +
>> + /// Called by the timer logic when the timer fires
>> + fn run(this: Self::Receiver);
>> +}
>
> The documentation says that this is implemented by pointers to structs,
> but that is not the case.
I will update the doc comment, it should say "implemented by structs that
can be the target...". Thanks.
>
>> +impl<T> RawTimer for Arc<T>
>> +where
>> + T: Send + Sync,
>> + T: HasTimer<T>,
>> +{
>> + fn schedule(self, expires: u64) {
>> + let self_ptr = Arc::into_raw(self);
>> +
>> + // SAFETY: `self_ptr` is a valid pointer to a `T`
>> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
>> +
>> + // `Timer` is `repr(transparent)`
>> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
>
> I would add an `raw_get` method to `Timer` instead of this cast,
> analogous to `Work::raw_get`.
>
Why is that? It is a lot of extra code, extra safety comments, etc.
In any case, would you prefer to implement said method with a cast
(which we can because `Timer` is transparent), or by `Opaque::raw_get`:
`Opaque::raw_get(core::ptr::addr_of!((*ptr).timer))`
Best regards,
Andreas
On Fri, Apr 26, 2024 at 11:27:49AM +0200, Andreas Hindborg wrote:
> Benno Lossin <[email protected]> writes:
>
> > On 25.04.24 11:46, Andreas Hindborg wrote:
> >> From: Andreas Hindborg <[email protected]>
> >>
> >> 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 <[email protected]>
> >>
> >> ---
> >>
> >> This patch is a dependency for the Rust null block driver [1].
> >>
> >> Link: https://lore.kernel.org/rust-for-linux/[email protected]/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
> >>
> >> rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
> >> rust/kernel/lib.rs | 1 +
> >> 2 files changed, 284 insertions(+)
> >> create mode 100644 rust/kernel/hrtimer.rs
> >>
> >> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
> >
> > Hmm is this the right place? I imagine there are other timers, does this
> > fit better into the `time` module (ie make `hrtimer` a submodule of
> > `time`) or should we later introduce a `timer` parent module?
>
> We can always move it. We will move stuff anyway when the kernel crate
> is split.
>
> We can also take it to `kernel::time::hrtimer` now, either way is fine.
>
I think `kernel::time::hrtimer` makes more sense, since ideally
schedule() function should take a time delta type as the input instead
of `u64`. So hrtimer has some logical connection to timekeeping module.
> >
> >> new file mode 100644
> >> index 000000000000..1e282608e70c
> >> --- /dev/null
> >> +++ b/rust/kernel/hrtimer.rs
> >> @@ -0,0 +1,283 @@
> >> +// 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.
> >> +//!
> >> +//! # Example
> >> +//!
> >> +//! ```rust
> >> +//! use kernel::{
> >> +//! sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
> >> +//! impl_has_timer, prelude::*, stack_pin_init
> >> +//! };
> >> +//! use core::sync::atomic::AtomicBool;
> >> +//! use core::sync::atomic::Ordering;
> >> +//!
> >> +//! #[pin_data]
> >> +//! struct IntrusiveTimer {
> >> +//! #[pin]
> >> +//! timer: Timer<Self>,
> >> +//! flag: AtomicBool,
Could you see if you can replace this with a `SpinLock<bool>` +
`CondVar`? We shouldn't use Rust atomic in kernel now. I know it's
unfortunate that LKMM atomics are still work in process, but in real
world, you won't do busy waiting for a timer to fire, so a
`CondVar::wait` is better for example purpose.
> >> +//! }
> >> +//!
> >> +//! impl IntrusiveTimer {
> >> +//! fn new() -> impl PinInit<Self> {
> >> +//! pin_init!(Self {
> >> +//! timer <- Timer::new(),
> >> +//! flag: AtomicBool::new(false),
> >> +//! })
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! impl TimerCallback for IntrusiveTimer {
> >> +//! type Receiver = Arc<IntrusiveTimer>;
> >> +//!
> >> +//! fn run(this: Self::Receiver) {
> >> +//! pr_info!("Timer called\n");
> >> +//! this.flag.store(true, Ordering::Relaxed);
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! impl_has_timer! {
> >> +//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
> >> +//! }
> >> +//!
> >> +//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
> >
> > I would not name this variable `has_timer`. Maybe `my_timer` is better?
>
> Right, thanks.
>
> >
> >> +//! has_timer.clone().schedule(200_000_000);
> >> +//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
> >
> > Weird formatting, we should also use `rustfmt` in examples.
>
> `format_code_in_doc_comments` is a nightly `rustfmt` feature. I tried
> enabling it in `.rustfmt.toml` and running `rustfmt +nightly
> hrtimer.rs`. It did not have any effect. There is some discussion here:
> https://github.com/rust-lang/rustfmt/issues/3348
>
> >
[...]
> >> +#[pinned_drop]
> >> +impl<T> PinnedDrop for Timer<T> {
> >> + fn drop(self: Pin<&mut Self>) {
> >> + // SAFETY: By struct invariant `self.timer` was initialized by
> >> + // `hrtimer_init` so by C API contract it is safe to call
> >> + // `hrtimer_cancel`.
> >> + unsafe {
> >> + bindings::hrtimer_cancel(self.timer.get());
> >> + }
> >> + }
> >> +}
> >
> > Why is this needed? The only way to schedule a timer using this API is
> > by having an `Arc` with a timer-containing struct inside. But to
> > schedule the `Arc`, you consume one refcount which is then sent to the
> > timer subsystem. So it is impossible for the refcount to drop below zero
> > while the timer is scheduled, but not yet running.
> > Do you need to call `hrtimer_cancel` after/while a timer is running?
>
> This is not required any longer. It is a leftover from an earlier
> revision where timers could be stack allocated. I will remove it.
>
So the plan is to add Arc<HasTimer> support first and stack allocated
timer later? If so, please do add a paragraph in the module level doc
describing the limition (e.g. stack allocated timers are not supported).
> > Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
> > that can happen when the timer callback owns the last refcount.
>
> That should be fine, `self` is still valid when the drop method is run?
>
> >
> >> +
> >> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
> >> +/// facilitates queueing the timer through the pointer that implements the
> >> +/// trait.
> >> +///
> >> +/// 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.
> >> +///
> >> +/// [`Box<T>`]: Box
> >> +/// [`Arc<T>`]: Arc
> >> +/// [`ARef<T>`]: crate::types::ARef
> >> +pub trait RawTimer: Sync {
> >> + /// Schedule the timer after `expires` time units
> >> + fn schedule(self, expires: u64);
This function should have a return value, see below:
> >> +}
[...]
> >> +impl<T> RawTimer for Arc<T>
> >> +where
> >> + T: Send + Sync,
> >> + T: HasTimer<T>,
> >> +{
> >> + fn schedule(self, expires: u64) {
> >> + let self_ptr = Arc::into_raw(self);
> >> +
so if the timer is already scheduled, re-scheduling will leak it, e.g.
let timer: Arc<SomeTimer> = ...;
let reschedule_handle = timer.clone(); // refcount == 2
timer.schedule(...);
...
// later on, a reschedule is needed
reschedule_handle.schedule(...); // refcount == 2
// <timer callback invoked>
Arc::drop();
// refcount == 1, the Arc is leaked.
Looks to me `schedule()` should return the `Arc` back if it's already
in the queue.
TBH, if you don't need the re-schedule and cancel functionality, maybe
start with `impl<T> RawTimer for Pin<Box<T>>` first.
Regards,
Boqun
> >> + // SAFETY: `self_ptr` is a valid pointer to a `T`
> >> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
> >> +
> >> + // `Timer` is `repr(transparent)`
> >> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
> >> +
> >> + // Schedule the timer - if it is already scheduled it is removed and
> >> + // inserted
> >> +
> >> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
> >> + // initialized by `hrtimer_init`
> >> + unsafe {
> >> + bindings::hrtimer_start_range_ns(
> >> + c_timer_ptr.cast_mut(),
> >> + expires as i64,
> >> + 0,
> >> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
> >> + );
> >> + }
> >> + }
> >> +}
> >> +
[...]
Boqun Feng <[email protected]> writes:
> On Fri, Apr 26, 2024 at 11:27:49AM +0200, Andreas Hindborg wrote:
>> Benno Lossin <[email protected]> writes:
>>
>> > On 25.04.24 11:46, Andreas Hindborg wrote:
>> >> From: Andreas Hindborg <[email protected]>
>> >>
>> >> 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 <[email protected]>
>> >>
>> >> ---
>> >>
>> >> This patch is a dependency for the Rust null block driver [1].
>> >>
>> >> Link: https://lore.kernel.org/rust-for-linux/[email protected]/T/#me0990150b9ba9f5b3d00293ec9a473c7bc3cc506 [1]
>> >>
>> >> rust/kernel/hrtimer.rs | 283 +++++++++++++++++++++++++++++++++++++++++
>> >> rust/kernel/lib.rs | 1 +
>> >> 2 files changed, 284 insertions(+)
>> >> create mode 100644 rust/kernel/hrtimer.rs
>> >>
>> >> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
>> >
>> > Hmm is this the right place? I imagine there are other timers, does this
>> > fit better into the `time` module (ie make `hrtimer` a submodule of
>> > `time`) or should we later introduce a `timer` parent module?
>>
>> We can always move it. We will move stuff anyway when the kernel crate
>> is split.
>>
>> We can also take it to `kernel::time::hrtimer` now, either way is fine.
>>
>
> I think `kernel::time::hrtimer` makes more sense, since ideally
> schedule() function should take a time delta type as the input instead
> of `u64`. So hrtimer has some logical connection to timekeeping module.
Yes, there is a bit of race condition with the ktime series. I guess I
will update this when the ktime patch is in. I am not sure if that was
picked yet or what tree it is going to go through.
>
>> >
>> >> new file mode 100644
>> >> index 000000000000..1e282608e70c
>> >> --- /dev/null
>> >> +++ b/rust/kernel/hrtimer.rs
>> >> @@ -0,0 +1,283 @@
>> >> +// 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.
>> >> +//!
>> >> +//! # Example
>> >> +//!
>> >> +//! ```rust
>> >> +//! use kernel::{
>> >> +//! sync::Arc, hrtimer::{RawTimer, Timer, TimerCallback},
>> >> +//! impl_has_timer, prelude::*, stack_pin_init
>> >> +//! };
>> >> +//! use core::sync::atomic::AtomicBool;
>> >> +//! use core::sync::atomic::Ordering;
>> >> +//!
>> >> +//! #[pin_data]
>> >> +//! struct IntrusiveTimer {
>> >> +//! #[pin]
>> >> +//! timer: Timer<Self>,
>> >> +//! flag: AtomicBool,
>
> Could you see if you can replace this with a `SpinLock<bool>` +
> `CondVar`? We shouldn't use Rust atomic in kernel now. I know it's
> unfortunate that LKMM atomics are still work in process, but in real
> world, you won't do busy waiting for a timer to fire, so a
> `CondVar::wait` is better for example purpose.
Since this is only using the atomic from Rust code, it should be fine
right? There is no mixing of memory models on this memory location.
>
>> >> +//! }
>> >> +//!
>> >> +//! impl IntrusiveTimer {
>> >> +//! fn new() -> impl PinInit<Self> {
>> >> +//! pin_init!(Self {
>> >> +//! timer <- Timer::new(),
>> >> +//! flag: AtomicBool::new(false),
>> >> +//! })
>> >> +//! }
>> >> +//! }
>> >> +//!
>> >> +//! impl TimerCallback for IntrusiveTimer {
>> >> +//! type Receiver = Arc<IntrusiveTimer>;
>> >> +//!
>> >> +//! fn run(this: Self::Receiver) {
>> >> +//! pr_info!("Timer called\n");
>> >> +//! this.flag.store(true, Ordering::Relaxed);
>> >> +//! }
>> >> +//! }
>> >> +//!
>> >> +//! impl_has_timer! {
>> >> +//! impl HasTimer<Self> for IntrusiveTimer { self.timer }
>> >> +//! }
>> >> +//!
>> >> +//! let has_timer = Arc::pin_init(IntrusiveTimer::new())?;
>> >
>> > I would not name this variable `has_timer`. Maybe `my_timer` is better?
>>
>> Right, thanks.
>>
>> >
>> >> +//! has_timer.clone().schedule(200_000_000);
>> >> +//! while !has_timer.flag.load(Ordering::Relaxed) { core::hint::spin_loop() }
>> >
>> > Weird formatting, we should also use `rustfmt` in examples.
>>
>> `format_code_in_doc_comments` is a nightly `rustfmt` feature. I tried
>> enabling it in `.rustfmt.toml` and running `rustfmt +nightly
>> hrtimer.rs`. It did not have any effect. There is some discussion here:
>> https://github.com/rust-lang/rustfmt/issues/3348
>>
>> >
> [...]
>> >> +#[pinned_drop]
>> >> +impl<T> PinnedDrop for Timer<T> {
>> >> + fn drop(self: Pin<&mut Self>) {
>> >> + // SAFETY: By struct invariant `self.timer` was initialized by
>> >> + // `hrtimer_init` so by C API contract it is safe to call
>> >> + // `hrtimer_cancel`.
>> >> + unsafe {
>> >> + bindings::hrtimer_cancel(self.timer.get());
>> >> + }
>> >> + }
>> >> +}
>> >
>> > Why is this needed? The only way to schedule a timer using this API is
>> > by having an `Arc` with a timer-containing struct inside. But to
>> > schedule the `Arc`, you consume one refcount which is then sent to the
>> > timer subsystem. So it is impossible for the refcount to drop below zero
>> > while the timer is scheduled, but not yet running.
>> > Do you need to call `hrtimer_cancel` after/while a timer is running?
>>
>> This is not required any longer. It is a leftover from an earlier
>> revision where timers could be stack allocated. I will remove it.
>>
>
> So the plan is to add Arc<HasTimer> support first and stack allocated
> timer later? If so, please do add a paragraph in the module level doc
> describing the limition (e.g. stack allocated timers are not supported).
I do not currently have any plans to add support for stack allocated
timers. I can give it another try if someone needs it. I ran into
problems with drop order when I tried it.
I will update the docs to mention this only supports heap allocated timers.
>
>> > Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
>> > that can happen when the timer callback owns the last refcount.
>>
>> That should be fine, `self` is still valid when the drop method is run?
>>
>> >
>> >> +
>> >> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
>> >> +/// facilitates queueing the timer through the pointer that implements the
>> >> +/// trait.
>> >> +///
>> >> +/// 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.
>> >> +///
>> >> +/// [`Box<T>`]: Box
>> >> +/// [`Arc<T>`]: Arc
>> >> +/// [`ARef<T>`]: crate::types::ARef
>> >> +pub trait RawTimer: Sync {
>> >> + /// Schedule the timer after `expires` time units
>> >> + fn schedule(self, expires: u64);
>
> This function should have a return value, see below:
>
>> >> +}
> [...]
>> >> +impl<T> RawTimer for Arc<T>
>> >> +where
>> >> + T: Send + Sync,
>> >> + T: HasTimer<T>,
>> >> +{
>> >> + fn schedule(self, expires: u64) {
>> >> + let self_ptr = Arc::into_raw(self);
>> >> +
>
> so if the timer is already scheduled, re-scheduling will leak it, e.g.
>
> let timer: Arc<SomeTimer> = ...;
>
> let reschedule_handle = timer.clone(); // refcount == 2
> timer.schedule(...);
>
> ...
>
> // later on, a reschedule is needed
> reschedule_handle.schedule(...); // refcount == 2
>
> // <timer callback invoked>
> Arc::drop();
> // refcount == 1, the Arc is leaked.
>
> Looks to me `schedule()` should return the `Arc` back if it's already
> in the queue.
Nice catch. We can use `bindings::hrtimer_cancel` to drop the `Arc` used
to enqueue if the timer was already enqueued. I think that should be OK
as far as usability of the API goes?
> TBH, if you don't need the re-schedule and cancel functionality, maybe
> start with `impl<T> RawTimer for Pin<Box<T>>` first.
I do not need to reschedule, but I need to support reference counted
types, and cancel would be nice to have eventually.
Best regards,
Andreas
On Tue, Apr 30, 2024 at 02:33:50PM +0200, Andreas Hindborg wrote:
[...]
> >
> > Could you see if you can replace this with a `SpinLock<bool>` +
> > `CondVar`? We shouldn't use Rust atomic in kernel now. I know it's
> > unfortunate that LKMM atomics are still work in process, but in real
> > world, you won't do busy waiting for a timer to fire, so a
> > `CondVar::wait` is better for example purpose.
>
> Since this is only using the atomic from Rust code, it should be fine
> right? There is no mixing of memory models on this memory location.
>
It's better compared to mixing accesses on a same location, but it's
still not allowed (for now, at least) to avoid mixing memory models on
ordering guarantees, for example:
(assume all memory location is initialized as 0)
CPU 0 CPU 1
----- -----
x.store(1, RELAXED); // Rust native atomic
smp_store_release(&y, 1); // LKMM atomic
let r0 = smp_load_acquire(&y);
let r1 = x.load(RELAXED);
The smp_store_release() and smp_load_acquire() pairs per LKMM, and
provide certain rel-acq ordering. But to make it (r0 == 1 && r1 == 0),
C11 memory model needs to understand this sort of orderings, but
currently there is no such thing as an "external ordering" to C11 memory
model.
I admit this is much of a theorical concern for code reasoning, in real
world, it must "just work", but "if you want to have fun, start with
one" ;-)
Regards,
Boqun
Andreas!
On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
I'm looking at this purely from a hrtimer perspective and please excuse
my minimal rust knowledge.
> +// SAFETY: A `Timer` can be moved to other threads and used from there.
> +unsafe impl<T> Send for Timer<T> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
Kinda. Using an hrtimer from different threads needs some thought in the
implementation as obviously ordering matters:
T1 T2
hrtimer_start() hrtimer_cancel()
So depending on whether T1 gets the internal lock first or T2 the
outcome is different. If T1 gets it first the timer is canceled by
T2. If T2 gets it first the timer ends up armed.
> +unsafe impl<T> Sync for Timer<T> {}
> +
> +impl<T: TimerCallback> Timer<T> {
> + /// Return an initializer for a new timer instance.
> + pub fn new() -> impl PinInit<Self> {
> + crate::pin_init!( Self {
> + 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,
This is odd. The initializer really should take a clock ID and a mode
argument. Otherwise you end up implementing a gazillion of different
timers.
> + );
> + }
> +
> + // SAFETY: `place` is pointing to a live allocation, so the deref
> + // is safe. The `function` field might not be initialized, but
> + // `addr_of_mut` does not create a reference to the field.
> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
> +
> + // SAFETY: `function` points to a valid allocation.
> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
We probably should introduce hrtimer_setup(timer, clockid, mode, function)
to avoid this construct. That would allow to cleanup existing C code too.
> + }),
> + _t: PhantomData,
> + })
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T> PinnedDrop for Timer<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: By struct invariant `self.timer` was initialized by
> + // `hrtimer_init` so by C API contract it is safe to call
> + // `hrtimer_cancel`.
> + unsafe {
> + bindings::hrtimer_cancel(self.timer.get());
> + }
> + }
> +}
> +
> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
> +/// facilitates queueing the timer through the pointer that implements the
> +/// trait.
> +///
> +/// 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.
Timer callbacks happen in hard or soft interrupt context.
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait RawTimer: Sync {
> + /// Schedule the timer after `expires` time units
> + fn schedule(self, expires: u64);
Don't we have some time related rust types in the kernel by now?
> +}
> +/// Implemented by pointer types that can be the target of a C timer callback.
> +pub trait RawTimerCallback: RawTimer {
> + /// Callback to be called from C.
> + ///
> + /// # Safety
> + ///
> + /// Only to be called by C code in `hrtimer`subsystem.
> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by pointers to structs that can the target of a timer callback
> +pub trait TimerCallback {
> + /// Type of `this` argument for `run()`.
> + type Receiver: RawTimerCallback;
> +
> + /// Called by the timer logic when the timer fires
> + fn run(this: Self::Receiver);
> +}
> +
> +impl<T> RawTimer for Arc<T>
> +where
> + T: Send + Sync,
> + T: HasTimer<T>,
> +{
> + fn schedule(self, expires: u64) {
> + let self_ptr = Arc::into_raw(self);
> +
> + // SAFETY: `self_ptr` is a valid pointer to a `T`
> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
> +
> + // `Timer` is `repr(transparent)`
> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
> +
> + // Schedule the timer - if it is already scheduled it is removed and
> + // inserted
> +
> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
> + // initialized by `hrtimer_init`
> + unsafe {
> + bindings::hrtimer_start_range_ns(
> + c_timer_ptr.cast_mut(),
> + expires as i64,
same comment vs. time
> + 0,
> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
and mode.
> + );
> + }
> + }
> +}
> +
> +impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
> +where
> + T: Send + Sync,
> + T: HasTimer<T>,
> + T: TimerCallback<Receiver = 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<T>>();
> +
> + // SAFETY: By C API contract `ptr` is the pointer we passed when
> + // enqueing the timer, so it is a `Timer<T>` embedded in a `T`
> + let data_ptr = unsafe { T::timer_container_of(timer_ptr) };
> +
> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
> + let receiver = unsafe { Arc::from_raw(data_ptr) };
> +
> + T::run(receiver);
> +
> + bindings::hrtimer_restart_HRTIMER_NORESTART
One of the common use cases of hrtimers is to create periodic schedules
where the timer callback advances the expiry value and returns
HRTIMER_RESTART. It might be not required for your initial use case at
hand, but you'll need that in the long run IMO.
Thanks,
tglx
On Fri, Apr 26 2024 at 07:52, Benno Lossin wrote:
> On 25.04.24 11:46, Andreas Hindborg wrote:
>> +#[pinned_drop]
>> +impl<T> PinnedDrop for Timer<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY: By struct invariant `self.timer` was initialized by
>> + // `hrtimer_init` so by C API contract it is safe to call
>> + // `hrtimer_cancel`.
>> + unsafe {
>> + bindings::hrtimer_cancel(self.timer.get());
>> + }
>> + }
>> +}
>
> Why is this needed? The only way to schedule a timer using this API is
> by having an `Arc` with a timer-containing struct inside. But to
> schedule the `Arc`, you consume one refcount which is then sent to the
> timer subsystem. So it is impossible for the refcount to drop below zero
> while the timer is scheduled, but not yet running.
> Do you need to call `hrtimer_cancel` after/while a timer is running?
>
> Also is it ok to call `hrtimer_cancel` inside the timer callback? Since
> that can happen when the timer callback owns the last refcount.
You cannot invoke hrtimer_cancel() from within the callback. That
deadlocks because hrtimer_cancel() waits for the callback to complete.
Thanks,
tglx
Boqun Feng <[email protected]> writes:
> On Tue, Apr 30, 2024 at 02:33:50PM +0200, Andreas Hindborg wrote:
> [...]
>> >
>> > Could you see if you can replace this with a `SpinLock<bool>` +
>> > `CondVar`? We shouldn't use Rust atomic in kernel now. I know it's
>> > unfortunate that LKMM atomics are still work in process, but in real
>> > world, you won't do busy waiting for a timer to fire, so a
>> > `CondVar::wait` is better for example purpose.
>>
>> Since this is only using the atomic from Rust code, it should be fine
>> right? There is no mixing of memory models on this memory location.
>>
>
> It's better compared to mixing accesses on a same location, but it's
> still not allowed (for now, at least) to avoid mixing memory models on
> ordering guarantees, for example:
>
> (assume all memory location is initialized as 0)
>
> CPU 0 CPU 1
> ----- -----
> x.store(1, RELAXED); // Rust native atomic
> smp_store_release(&y, 1); // LKMM atomic
> let r0 = smp_load_acquire(&y);
> let r1 = x.load(RELAXED);
>
> The smp_store_release() and smp_load_acquire() pairs per LKMM, and
> provide certain rel-acq ordering. But to make it (r0 == 1 && r1 == 0),
> C11 memory model needs to understand this sort of orderings, but
> currently there is no such thing as an "external ordering" to C11 memory
> model.
>
> I admit this is much of a theorical concern for code reasoning, in real
> world, it must "just work", but "if you want to have fun, start with
> one" ;-)
Alright, I will change it to a `CondVar` or a `SpinLock` ????
BR Andreas
Andreas!
On Tue, Apr 30 2024 at 20:18, Andreas Hindborg wrote:
> Thomas Gleixner <[email protected]> writes:
>> On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
>>> +// SAFETY: A `Timer` can be moved to other threads and used from there.
>>> +unsafe impl<T> Send for Timer<T> {}
>>> +
>>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>>> +// timer from multiple threads
>>
>> Kinda. Using an hrtimer from different threads needs some thought in the
>> implementation as obviously ordering matters:
>>
>> T1 T2
>> hrtimer_start() hrtimer_cancel()
>>
>> So depending on whether T1 gets the internal lock first or T2 the
>> outcome is different. If T1 gets it first the timer is canceled by
>> T2. If T2 gets it first the timer ends up armed.
>
> That is all fine. What is meant here is that we will not get UB in the
> `hrtimer` subsystem when racing these operations. As far as I can tell
> from the C source, the operations are atomic, even though their
> interleaving will not be deterministic.
That's correct. All operations happen with the associated base lock held.
>>> +unsafe impl<T> Sync for Timer<T> {}
>>> +
>>> +impl<T: TimerCallback> Timer<T> {
>>> + /// Return an initializer for a new timer instance.
>>> + pub fn new() -> impl PinInit<Self> {
>>> + crate::pin_init!( Self {
>>> + 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,
>>
>> This is odd. The initializer really should take a clock ID and a mode
>> argument. Otherwise you end up implementing a gazillion of different
>> timers.
>
> I implemented the minimum set of features to satisfy the requirements
> for the Rust null block driver. It is my understanding that most
> maintainers of existing infrastructure prefers to have a user for the
> implemented features, before wanting to merge them.
>
> I can try to extend the abstractions to cover a more complete `hrtimer`
> API. Or we can work on this subset and try to get that ready to merge,
> and then expand scope later.
Wouldn't expanding scope later require to change already existing call sites?
>>> + );
>>> + }
>>> +
>>> + // SAFETY: `place` is pointing to a live allocation, so the deref
>>> + // is safe. The `function` field might not be initialized, but
>>> + // `addr_of_mut` does not create a reference to the field.
>>> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
>>> +
>>> + // SAFETY: `function` points to a valid allocation.
>>> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
>>
>> We probably should introduce hrtimer_setup(timer, clockid, mode, function)
>> to avoid this construct. That would allow to cleanup existing C code too.
>
> Do you want me to cook up a C patch for that, or would you prefer to do
> that yourself?
Please create that patch yourself and convert at least one C location to
this new interface in a separate patch. THe remaining C cleanup can go
from there and mostly be scripted with coccinelle.
>>> +/// [`Box<T>`]: Box
>>> +/// [`Arc<T>`]: Arc
>>> +/// [`ARef<T>`]: crate::types::ARef
>>> +pub trait RawTimer: Sync {
>>> + /// Schedule the timer after `expires` time units
>>> + fn schedule(self, expires: u64);
>>
>> Don't we have some time related rust types in the kernel by now?
>
> There are patches on the list, but I think they are not applied to any
> tree yet? I did not want to depend on those patches before they are
> staged somewhere. Would you prefer this patch on top of the Rust `ktime`
> patches?
The initial set is queued in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
for 6.10. Boqun has some updates on top IIRC. Your stuff should go
through that branch too.
>>> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
>>> + let receiver = unsafe { Arc::from_raw(data_ptr) };
>>> +
>>> + T::run(receiver);
>>> +
>>> + bindings::hrtimer_restart_HRTIMER_NORESTART
>>
>> One of the common use cases of hrtimers is to create periodic schedules
>> where the timer callback advances the expiry value and returns
>> HRTIMER_RESTART. It might be not required for your initial use case at
>> hand, but you'll need that in the long run IMO.
>
> If you are OK with taking that feature without a user, I will gladly add
> it.
I'm fine with taking a more complete API which does not require to
change usage sites later on.
Thanks,
tglx
Hi Thomas,
Thomas Gleixner <[email protected]> writes:
> Andreas!
>
> On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
>
> I'm looking at this purely from a hrtimer perspective and please excuse
> my minimal rust knowledge.
Thanks for taking a look!
>
>> +// SAFETY: A `Timer` can be moved to other threads and used from there.
>> +unsafe impl<T> Send for Timer<T> {}
>> +
>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>> +// timer from multiple threads
>
> Kinda. Using an hrtimer from different threads needs some thought in the
> implementation as obviously ordering matters:
>
> T1 T2
> hrtimer_start() hrtimer_cancel()
>
> So depending on whether T1 gets the internal lock first or T2 the
> outcome is different. If T1 gets it first the timer is canceled by
> T2. If T2 gets it first the timer ends up armed.
That is all fine. What is meant here is that we will not get UB in the
`hrtimer` subsystem when racing these operations. As far as I can tell
from the C source, the operations are atomic, even though their
interleaving will not be deterministic.
>
>> +unsafe impl<T> Sync for Timer<T> {}
>> +
>> +impl<T: TimerCallback> Timer<T> {
>> + /// Return an initializer for a new timer instance.
>> + pub fn new() -> impl PinInit<Self> {
>> + crate::pin_init!( Self {
>> + 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,
>
> This is odd. The initializer really should take a clock ID and a mode
> argument. Otherwise you end up implementing a gazillion of different
> timers.
I implemented the minimum set of features to satisfy the requirements
for the Rust null block driver. It is my understanding that most
maintainers of existing infrastructure prefers to have a user for the
implemented features, before wanting to merge them.
I can try to extend the abstractions to cover a more complete `hrtimer`
API. Or we can work on this subset and try to get that ready to merge,
and then expand scope later.
What would you prefer?
>
>> + );
>> + }
>> +
>> + // SAFETY: `place` is pointing to a live allocation, so the deref
>> + // is safe. The `function` field might not be initialized, but
>> + // `addr_of_mut` does not create a reference to the field.
>> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
>> +
>> + // SAFETY: `function` points to a valid allocation.
>> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
>
> We probably should introduce hrtimer_setup(timer, clockid, mode, function)
> to avoid this construct. That would allow to cleanup existing C code too.
Do you want me to cook up a C patch for that, or would you prefer to do
that yourself?
>
>> + }),
>> + _t: PhantomData,
>> + })
>> + }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T> PinnedDrop for Timer<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY: By struct invariant `self.timer` was initialized by
>> + // `hrtimer_init` so by C API contract it is safe to call
>> + // `hrtimer_cancel`.
>> + unsafe {
>> + bindings::hrtimer_cancel(self.timer.get());
>> + }
>> + }
>> +}
>> +
>> +/// Implemented by pointer types to structs that embed a [`Timer`]. This trait
>> +/// facilitates queueing the timer through the pointer that implements the
>> +/// trait.
>> +///
>> +/// 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.
>
> Timer callbacks happen in hard or soft interrupt context.
Thanks, I'll be sure to add that to the documentation.
>
>> +/// [`Box<T>`]: Box
>> +/// [`Arc<T>`]: Arc
>> +/// [`ARef<T>`]: crate::types::ARef
>> +pub trait RawTimer: Sync {
>> + /// Schedule the timer after `expires` time units
>> + fn schedule(self, expires: u64);
>
> Don't we have some time related rust types in the kernel by now?
There are patches on the list, but I think they are not applied to any
tree yet? I did not want to depend on those patches before they are
staged somewhere. Would you prefer this patch on top of the Rust `ktime`
patches?
>
>> +}
>
>> +/// Implemented by pointer types that can be the target of a C timer callback.
>> +pub trait RawTimerCallback: RawTimer {
>> + /// Callback to be called from C.
>> + ///
>> + /// # Safety
>> + ///
>> + /// Only to be called by C code in `hrtimer`subsystem.
>> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>> +
>> +/// Implemented by pointers to structs that can the target of a timer callback
>> +pub trait TimerCallback {
>> + /// Type of `this` argument for `run()`.
>> + type Receiver: RawTimerCallback;
>> +
>> + /// Called by the timer logic when the timer fires
>> + fn run(this: Self::Receiver);
>> +}
>> +
>> +impl<T> RawTimer for Arc<T>
>> +where
>> + T: Send + Sync,
>> + T: HasTimer<T>,
>> +{
>> + fn schedule(self, expires: u64) {
>> + let self_ptr = Arc::into_raw(self);
>> +
>> + // SAFETY: `self_ptr` is a valid pointer to a `T`
>> + let timer_ptr = unsafe { T::raw_get_timer(self_ptr) };
>> +
>> + // `Timer` is `repr(transparent)`
>> + let c_timer_ptr = timer_ptr.cast::<bindings::hrtimer>();
>> +
>> + // Schedule the timer - if it is already scheduled it is removed and
>> + // inserted
>> +
>> + // SAFETY: c_timer_ptr points to a valid hrtimer instance that was
>> + // initialized by `hrtimer_init`
>> + unsafe {
>> + bindings::hrtimer_start_range_ns(
>> + c_timer_ptr.cast_mut(),
>> + expires as i64,
>
> same comment vs. time
>
>> + 0,
>> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
>
> and mode.
>
>> + );
>> + }
>> + }
>> +}
>> +
>> +impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
>> +where
>> + T: Send + Sync,
>> + T: HasTimer<T>,
>> + T: TimerCallback<Receiver = 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<T>>();
>> +
>> + // SAFETY: By C API contract `ptr` is the pointer we passed when
>> + // enqueing the timer, so it is a `Timer<T>` embedded in a `T`
>> + let data_ptr = unsafe { T::timer_container_of(timer_ptr) };
>> +
>> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
>> + let receiver = unsafe { Arc::from_raw(data_ptr) };
>> +
>> + T::run(receiver);
>> +
>> + bindings::hrtimer_restart_HRTIMER_NORESTART
>
> One of the common use cases of hrtimers is to create periodic schedules
> where the timer callback advances the expiry value and returns
> HRTIMER_RESTART. It might be not required for your initial use case at
> hand, but you'll need that in the long run IMO.
If you are OK with taking that feature without a user, I will gladly add
it.
Best regards,
Andreas
Thomas Gleixner <[email protected]> writes:
> Andreas!
>
> On Tue, Apr 30 2024 at 20:18, Andreas Hindborg wrote:
>> Thomas Gleixner <[email protected]> writes:
>>> On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
>>>> +// SAFETY: A `Timer` can be moved to other threads and used from there.
>>>> +unsafe impl<T> Send for Timer<T> {}
>>>> +
>>>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>>>> +// timer from multiple threads
>>>
>>> Kinda. Using an hrtimer from different threads needs some thought in the
>>> implementation as obviously ordering matters:
>>>
>>> T1 T2
>>> hrtimer_start() hrtimer_cancel()
>>>
>>> So depending on whether T1 gets the internal lock first or T2 the
>>> outcome is different. If T1 gets it first the timer is canceled by
>>> T2. If T2 gets it first the timer ends up armed.
>>
>> That is all fine. What is meant here is that we will not get UB in the
>> `hrtimer` subsystem when racing these operations. As far as I can tell
>> from the C source, the operations are atomic, even though their
>> interleaving will not be deterministic.
>
> That's correct. All operations happen with the associated base lock held.
>
>>>> +unsafe impl<T> Sync for Timer<T> {}
>>>> +
>>>> +impl<T: TimerCallback> Timer<T> {
>>>> + /// Return an initializer for a new timer instance.
>>>> + pub fn new() -> impl PinInit<Self> {
>>>> + crate::pin_init!( Self {
>>>> + 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,
>>>
>>> This is odd. The initializer really should take a clock ID and a mode
>>> argument. Otherwise you end up implementing a gazillion of different
>>> timers.
>>
>> I implemented the minimum set of features to satisfy the requirements
>> for the Rust null block driver. It is my understanding that most
>> maintainers of existing infrastructure prefers to have a user for the
>> implemented features, before wanting to merge them.
>>
>> I can try to extend the abstractions to cover a more complete `hrtimer`
>> API. Or we can work on this subset and try to get that ready to merge,
>> and then expand scope later.
>
> Wouldn't expanding scope later require to change already existing call sites?
Yes, potentially. But I hear that Coccinelle is gaining Rust support ????
>
>>>> + );
>>>> + }
>>>> +
>>>> + // SAFETY: `place` is pointing to a live allocation, so the deref
>>>> + // is safe. The `function` field might not be initialized, but
>>>> + // `addr_of_mut` does not create a reference to the field.
>>>> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
>>>> +
>>>> + // SAFETY: `function` points to a valid allocation.
>>>> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
>>>
>>> We probably should introduce hrtimer_setup(timer, clockid, mode, function)
>>> to avoid this construct. That would allow to cleanup existing C code too.
>>
>> Do you want me to cook up a C patch for that, or would you prefer to do
>> that yourself?
>
> Please create that patch yourself and convert at least one C location to
> this new interface in a separate patch. THe remaining C cleanup can go
> from there and mostly be scripted with coccinelle.
Ok.
>
>>>> +/// [`Box<T>`]: Box
>>>> +/// [`Arc<T>`]: Arc
>>>> +/// [`ARef<T>`]: crate::types::ARef
>>>> +pub trait RawTimer: Sync {
>>>> + /// Schedule the timer after `expires` time units
>>>> + fn schedule(self, expires: u64);
>>>
>>> Don't we have some time related rust types in the kernel by now?
>>
>> There are patches on the list, but I think they are not applied to any
>> tree yet? I did not want to depend on those patches before they are
>> staged somewhere. Would you prefer this patch on top of the Rust `ktime`
>> patches?
>
> The initial set is queued in
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
>
> for 6.10. Boqun has some updates on top IIRC. Your stuff should go
> through that branch too.
Ok.
>
>>>> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
>>>> + let receiver = unsafe { Arc::from_raw(data_ptr) };
>>>> +
>>>> + T::run(receiver);
>>>> +
>>>> + bindings::hrtimer_restart_HRTIMER_NORESTART
>>>
>>> One of the common use cases of hrtimers is to create periodic schedules
>>> where the timer callback advances the expiry value and returns
>>> HRTIMER_RESTART. It might be not required for your initial use case at
>>> hand, but you'll need that in the long run IMO.
>>
>> If you are OK with taking that feature without a user, I will gladly add
>> it.
>
> I'm fine with taking a more complete API which does not require to
> change usage sites later on.
I will expand the API and send an updated patch when that is done ????
BR Andreas