Message-ID: <87v844lbhm.fsf@metaspace.dk>
Date: Fri, 26 Apr 2024 11:27:49 +0200
From: Andreas Hindborg <nmi@...aspace.dk>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Miguel Ojeda <ojeda@...nel.org>,  Alex Gaynor <alex.gaynor@...il.com>,
  Wedson Almeida Filho <wedsonaf@...il.com>,  Anna-Maria Behnsen
 <anna-maria@...utronix.de>,  Frederic Weisbecker <frederic@...nel.org>,
  Thomas Gleixner <tglx@...utronix.de>,  Andreas Hindborg
 <a.hindborg@...sung.com>,  Boqun Feng <boqun.feng@...il.com>,  Gary Guo
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  Alice
 Ryhl <aliceryhl@...gle.com>,  rust-for-linux@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support

Benno Lossin <benno.lossin@...ton.me> writes:

> On 25.04.24 11:46, Andreas Hindborg wrote:
>> From: Andreas Hindborg <a.hindborg@...sung.com>
>> 
>> 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@...sung.com>
>> 
>> ---
>> 
>> This patch is a dependency for the Rust null block driver [1].
>> 
>> Link: https://lore.kernel.org/rust-for-linux/20240313110515.70088-1-nmi@metaspace.dk/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@google.com/

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

Powered by blists - more mailing lists