Message-ID: <CAJ-ks9m9B1ibFtJNSTo0-e9y01-7+Rk=d8i2_L6Zq+qOeBvY5g@mail.gmail.com>
Date: Tue, 14 Jan 2025 12:23:42 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Lyude Paul <lyude@...hat.com>, Guangbo Cui <2407018371@...com>, 
	Dirk Behme <dirk.behme@...il.com>, Daniel Almeida <daniel.almeida@...labora.com>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

On Fri, Jan 10, 2025 at 3:18 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
> ---
>  rust/kernel/time.rs         |   2 +
>  rust/kernel/time/hrtimer.rs | 296 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 298 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index f59e0fea79d3acfddd922f601f569353609aeec1..51c3532eee0184495ed5b7d717860c9980ff2a43 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -10,6 +10,8 @@
>
>  use core::convert::Into;
>
> +pub mod hrtimer;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..31c461ddd2c377101ed6c1c7e009f7dccf458ebc
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Intrusive high resolution timers.
> +//!
> +//! Allows running timer callbacks without doing allocations at the time of
> +//! starting the timer. For now, only one timer per type is allowed.
> +//!
> +//! # Vocabulary
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** with an **expiry** time. After the timer is started, it is
> +//! **running**. When the timer **expires**, the timer handler is executed.
> +//! After the handler has executed, the timer may be **restarted** or
> +//! **stopped**. A running timer can be **cancelled** before it's handler is
> +//! executed. A timer that is cancelled enters the **stopped** state.

s/it's/its/

I find this pretty hard to read because it contains a mix of
descriptions of states and descriptions of state transitions. Can we
state all the states first, and then all the transitions? Diagrams are
always helpful.

> +//!
> +//! States:
> +//!
> +//! * Stopped
> +//! * Running
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Stop
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +
> +use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// A timer backed by a C `struct hrtimer`.
> +///
> +/// # Invariants
> +///
> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
> +#[pin_data]
> +#[repr(C)]
> +pub struct HrTimer<U> {
> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<U>,
> +}
> +
> +// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.

Is that because it's on the heap on the C side, and the Rust type is
roughly a pointer to it?

> +unsafe impl<U> Send for HrTimer<U> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
> +unsafe impl<U> Sync for HrTimer<U> {}
> +
> +impl<T> HrTimer<T> {
> +    /// Return an initializer for a new timer instance.

What's an initializer?

> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: HrTimerCallback,
> +    {
> +        pin_init!(Self {
> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> +                // SAFETY: By design of `pin_init!`, `place` is a pointer live

pointer *to a* live allocation?

> +                // allocation. hrtimer_setup will initialize `place` and does
> +                // not require `place` to be initialized prior to the call.
> +                unsafe {
> +                    bindings::hrtimer_setup(
> +                        place,
> +                        Some(T::CallbackTarget::run),
> +                        bindings::CLOCK_MONOTONIC as i32,

Is it possible to avoid this as cast?

> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a live allocation of at least the size of `Self`.

This function is private, this doc comment won't ever be rendered by rustdoc.

> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> +        // because the caller of this function promises that `ptr` points to an
> +        // allocation of at least the size of `Self`.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> +    }
> +
> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this will block until the handler is
> +    /// finished.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    #[allow(dead_code)]

Why does this need to be raw? This can be explained in the commit
message or the cover letter IMO.

> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
> +
> +        // If handler is running, this will wait for handler to finish before
> +        // returning.

Missing article in both halves; s/handler/the handler/.

> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.
> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
> +///
> +/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
> +/// has a field of type [`HrTimer`].

nit: this paragraph is over specifying things.

> +///
> +/// Target must be [`Sync`] because timer callbacks happen in another thread of
> +/// execution (hard or soft interrupt context).

What is "Target"?

> +///
> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the start function repeatedly, and
> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.

"Cancel" is a transition, not a state, so I think this should say "a
stopped timer".

> +///
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: crate::sync::Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait HrTimerPointer: Sync + Sized {
> +    /// A handle representing a running timer.

The comment just above says that a handle can represent a stopped
timer. Which is it?

> +    ///
> +    /// If the timer is running or if the timer callback is executing when the
> +    /// handle is dropped, the drop method of [`HrTimerHandle`] should not return
> +    /// until the timer is stopped and the callback has completed.
> +    ///
> +    /// Note: It must be safe to leak the handle.

What does "safe" mean exactly? Also, this comment seems to be
describing the trait `HrTimerHandle` rather than the associated type
`TimerHandle`.

> +    type TimerHandle: HrTimerHandle;
> +
> +    /// Start the timer with expiry after `expires` time units. If the timer was
> +    /// already running, it is restarted with the new expiry time.
> +    fn start(self, expires: Ktime) -> Self::TimerHandle;
> +}
> +
> +/// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
> +/// function to call.
> +// This is split from `HrTimerPointer` to make it easier to specify trait bounds.
> +pub trait RawHrTimerCallback {
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
> +    /// the `bindings::hrtimer` structure that was used to start the timer.
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by structs that can the target of a timer callback.

Missing verb: s/can the/can be the/

> +pub trait HrTimerCallback {
> +    /// The type that was used for starting the timer.

This comment isn't very helpful, is it? I think it should say "The
type whose `run` method will be invoked when the timer expires.".

> +    type CallbackTarget<'a>: RawHrTimerCallback;
> +
> +    /// This type is passed to the timer callback function. It may be a borrow
> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
> +    /// implementation can guarantee exclusive access to the target during timer
> +    /// handler execution.
> +    type CallbackTargetParameter<'a>;
> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: Self::CallbackTargetParameter<'_>)
> +    where
> +        Self: Sized;
> +}
> +
> +/// A handle representing a potentially running timer.
> +///
> +/// More than one handle representing the same timer might exist.
> +///
> +/// # Safety
> +///
> +/// When dropped, the timer represented by this handle must be cancelled, if it
> +/// is running. If the timer handler is running when the handle is dropped, the
> +/// drop method must wait for the handler to finish before returning.

See my note above anchored on `type TimerHandle: HrTimerHandle`; it
repeats this comment.

> +pub unsafe trait HrTimerHandle {
> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> +    /// till the handler has finished.
> +    fn cancel(&mut self) -> bool;
> +}
> +
> +/// Implemented by structs that contain timer nodes.
> +///
> +/// Clients of the timer API would usually safely implement this trait by using
> +/// the [`crate::impl_has_hr_timer`] macro.
> +///
> +/// # Safety
> +///
> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> +/// 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 HasHrTimer<U> {
> +    /// Offset of the [`HrTimer`] field within `Self`
> +    const OFFSET: usize;
> +
> +    /// Return a pointer to the [`HrTimer`] within `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid struct of type `Self`.
> +    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<U> {
> +        // SAFETY: By the safety requirement of this trait, the trait
> +        // implementor will have a `HrTimer` field at the specified offset.
> +        unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<HrTimer<U>>() }
> +    }
> +
> +    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
> +    /// to by `ptr`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a [`HrTimer<U>`] field in a struct of type `Self`.
> +    unsafe fn timer_container_of(ptr: *mut HrTimer<U>) -> *mut Self
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: By the safety requirement of this function and the `HasHrTimer`
> +        // trait, the following expression will yield a pointer to the `Self`
> +        // containing the timer addressed by `ptr`.
> +        unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
> +    }
> +
> +    /// Get pointer to embedded `bindings::hrtimer` struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
> +        // SAFETY: `self_ptr` is a valid pointer to a `Self`.
> +        let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
> +
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        unsafe { HrTimer::raw_get(timer_ptr) }
> +    }
> +
> +    /// Start the timer contained in the `Self` pointed to by `self_ptr`. If
> +    /// it is already running it is removed and inserted.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn start(self_ptr: *const Self, expires: Ktime) {
> +        // SAFETY: By function safety requirement, `self_ptr`is a valid `Self`.
> +        unsafe {
> +            bindings::hrtimer_start_range_ns(
> +                Self::c_timer_ptr(self_ptr).cast_mut(),
> +                expires.to_ns(),
> +                0,
> +                bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +            );
> +        }
> +    }
> +}
> +
> +/// Use to implement the [`HasHrTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::time::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_hr_timer {
> +    (
> +        impl$({$($generics:tt)*})?
> +            HasHrTimer<$timer_type:ty>
> +            for $self:ty
> +        { self.$field:ident }
> +        $($rest:tt)*
> +    ) => {
> +        // SAFETY: This implementation of `raw_get_timer` only compiles if the
> +        // field has the right type.
> +        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
> +            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> +
> +            #[inline]
> +            unsafe fn raw_get_timer(ptr: *const Self) ->
> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
> +            {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of!((*ptr).$field)
> +                }
> +            }
> +        }
> +    }
> +}

It'd be helpful to understand these design choices; why is so much of
this written in terms of raw pointers?

Tamir

Powered by blists - more mailing lists