Re: [PATCH] rust: hrtimer: introduce hrtimer support
From: Andreas Hindborg
Date: Fri Apr 26 2024 - 05:28:07 EST
Benno Lossin <benno.lossin@xxxxxxxxx> writes:
> On 25.04.24 11:46, Andreas Hindborg wrote:
>> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>>
>> ---
>>
>> This patch is a dependency for the Rust null block driver [1].
>>
>> Link: https://lore.kernel.org/rust-for-linux/20240313110515.70088-1-nmi@xxxxxxxxxxxx/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/20240402-linked-list-v1-4-b1c59ba7ae3b@xxxxxxxxxx/
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