Message-ID: <CAJ-ks9nj8+fXM_oo0LJo4O6Q=skFRcHwz8TLxw-yB3QTcDF9GA@mail.gmail.com>
Date: Mon, 24 Feb 2025 15:04:57 -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 v9 01/13] rust: hrtimer: introduce hrtimer support

Hi Andreas, mostly grammar and prose clarity comments below.

I still think HasHrTimer::OFFSET is less clear and more fragile than
just generating compiler-checked implementations in the macro (you're
already generating OFFSET and one method implementation rather than
generating 2 method implementations).

On Mon, Feb 24, 2025 at 7:06 AM 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>
> ---
>  kernel/time/Kconfig         |  13 ++
>  rust/kernel/time.rs         |   3 +
>  rust/kernel/time/hrtimer.rs | 347 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>
> diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
> index b0b97a60aaa6..7726e14ca3e2 100644
> --- a/kernel/time/Kconfig
> +++ b/kernel/time/Kconfig
> @@ -211,3 +211,16 @@ config CLOCKSOURCE_WATCHDOG_MAX_SKEW_US
>
>  endmenu
>  endif
> +
> +config RUST_HRTIMER
> +       bool "Enable Rust hrtimer API"
> +       depends on RUST
> +       default y
> +       help
> +         This option allows exclusion of the Rust hrtimer API from the build.
> +         This allows testing out changes to the C API without having to update
> +         the Rust abstractions during initial development.
> +
> +         Say Y if you wish to build the Rust hrtimer API.
> +
> +         Say N if you wish to exclude the Rust hrtimer API from the build.
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 379c0f5772e5..e928b1340ee3 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -8,6 +8,9 @@
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>
> +#[cfg(CONFIG_RUST_HRTIMER)]
> +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 000000000000..fe20405d8bfe
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,347 @@
> +// 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
> +//!
> +//! States:
> +//!
> +//! - Stopped: initialized but not started, or cancelled, or not restarted.
> +//! - Started: initialized and started or restarted.
> +//! - Running: executing the callback.
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +//!
> +//! ## State Diagram
> +//!
> +//! ```text
> +//!                                                   Return NoRestart
> +//!                       +---------------------------------------------------------------------+
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                         Return Restart              |
> +//!                       |                                      +------------------------+     |
> +//!                       |                                      |                        |     |
> +//!                       |                                      |                        |     |
> +//!                       v                                      v                        |     |
> +//!           +-----------------+      Start      +------------------+           +--------+-----+--+
> +//!           |                 +---------------->|                  |           |                 |
> +//! Init      |                 |                 |                  |  Expire   |                 |
> +//! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
> +//!           |                 |     Cancel      |                  |           |                 |
> +//!           |                 |<----------------+                  |           |                 |
> +//!           +-----------------+                 +---------------+--+           +-----------------+
> +//!                                                     ^         |
> +//!                                                     |         |
> +//!                                                     +---------+
> +//!                                                      Restart
> +//! ```
> +//!
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** by the `start` operation, with an **expiry** time. After the
> +//! `start` operation, the timer is in the **started** state. When the timer
> +//! **expires**, the timer enters the **running** state and the handler is
> +//! executed. After the handler has finished executing, the timer may enter the
> +//! **started* or **stopped** state, depending on the return value of the
> +//! handler. A running timer can be **canceled** by the `cancel` operation. A
> +//! timer that is cancelled enters the **stopped** state.

This is a bit confusing because it sounds like you're describing a
*started* timer. After reading the next paragraph I think this wording
applies to both *started* and *running*, but it isn't unambiguous.

> +//!
> +//! A `cancel` or `restart` operation on a timer in the **running** state takes
> +//! effect after the handler has finished executing and the timer has transitioned
> +//! out of the **running** state.

There's no external restart, right? I think this wording is confused
by the unification of cancel and restart under operations, though they
are not isomorphic. Restart (as I understand it) can only happen from
the handler, and cancel can only happen via a call to hrtimer_cancel.
It's also a bit strange that start isn't mentioned whenever cancel and
restart are mentioned.

> +//!
> +//! A `restart` operation on a timer in the **stopped** state is equivalent to a
> +//! `start` operation.
> +
> +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<T> {
> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<T>,
> +}
> +
> +// SAFETY: Ownership of an `HrTimer` can be moved to other threads and
> +// used/dropped from there.
> +unsafe impl<T> Send for HrTimer<T> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads

nit: missing article ("the" C side) and missing period.

> +unsafe impl<T> Sync for HrTimer<T> {}
> +
> +impl<T> HrTimer<T> {
> +    /// Return an initializer for a new timer instance.
> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: HrTimerCallback,
> +    {
> +        pin_init!(Self {
> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.

Why plural INVARIANTS?

> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> +                // SAFETY: By design of `pin_init!`, `place` is a pointer to a
> +                // live 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::Pointer::run),
> +                        bindings::CLOCK_MONOTONIC as i32,
> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// This function do not create any references.

s/do/does/

But maybe this should use the same wording from Opaque::raw_get?

    /// This function is useful to get access to the value without
creating intermediate
    /// references.

If so, consider also naming the argument "this" for consistency. Same
for other methods below.

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

nit: s/is finished/returns/ and maybe clarify the ordering, namely
that the timer is definitely in a stopped state after this returns.

> +    ///
> +    /// Users of the `HrTimer` API would not usually call this method directly.
> +    /// Instead they would use the safe [`HrTimerHandle::cancel`] on the handle
> +    /// returned when the timer was started.
> +    ///
> +    /// This function does not create any references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.

Why use different phrasing here than on raw_get? The parameter name is
also different. Would be nice to be consistent.

> +    #[allow(dead_code)]
> +    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 the handler is running, this will wait for the handler to finish
> +        // before returning.
> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.

missing article here.

> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
> +///
> +/// Target (pointee) must be [`Sync`] because timer callbacks happen in another
> +/// thread of execution (hard or soft interrupt context).

Is this explaining the bound on the trait, or something that exists
outside the type system? If it's the former, isn't the Sync bound on
the trait going to apply to the pointer rather than the pointee?

> +///
> +/// 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.
> +pub trait HrTimerPointer: Sync + Sized {
> +    /// A handle representing a started or restarted timer.
> +    ///
> +    /// 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: When implementing this trait, consider that it is not unsafe to
> +    /// leak the handle.

What does leak mean in this context?

> +    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 {
> +    /// This type is passed to [`HrTimerCallback::run`]. It may be a borrow of
> +    /// [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
> +    /// implementation can guarantee correct access (exclusive or shared
> +    /// depending on the type) to the target during timer handler execution.
> +    type CallbackTarget<'a>;
> +
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to

missing article, should be "...in the `hrtimer`..."

> +    /// 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 be the target of a timer callback.
> +pub trait HrTimerCallback {
> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
> +    /// the timer expires.
> +    type Pointer<'a>: RawHrTimerCallback;
> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>)
> +    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.
> +///
> +/// Note: One way to satisfy the safety requirement is to call `Self::cancel` in
> +/// the drop implementation for `Self.`
> +pub unsafe trait HrTimerHandle {
> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> +    /// till the handler has finished.

Here's another case where "running" is confusingly used to refer to
the timer being in the state before the handler has begun to execute,
and also to the state after the handler has begun to execute.

> +    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<T> {
> +    /// Offset of the [`HrTimer`] field within `Self`
> +    const OFFSET: usize;
> +
> +    /// Return a pointer to the [`HrTimer`] within `Self`.
> +    ///
> +    /// This function does not create any references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid struct of type `Self`.
> +    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<T> {
> +        // 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<T>>() }
> +    }
> +
> +    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
> +    /// to by `ptr`.
> +    ///
> +    /// This function does not create any references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a [`HrTimer<T>`] field in a struct of type `Self`.
> +    unsafe fn timer_container_of(ptr: *mut HrTimer<T>) -> *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.
> +    ///
> +    /// This function does not create any references.
> +    ///
> +    /// # 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`.
> +    /// - Caller must ensure that `self` lives until the timer fires or is
> +    ///   canceled.
> +    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)
> +                }
> +            }
> +        }
> +    }
> +}
>
> --
> 2.47.0
>
>

Cheers.
Tamir

Powered by blists - more mailing lists