Message-ID: <f502f6c6-40b7-4ec4-8d0c-09adb0df1a30@proton.me>
Date: Wed, 18 Sep 2024 18:13:11 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <a.hindborg@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>
Cc: 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 v2 02/14] rust: hrtimer: introduce hrtimer support

On 18.09.24 00:27, Andreas Hindborg 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/hrtimer.rs | 231 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  2 files changed, 232 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..5c92afd8eb2c
> --- /dev/null
> +++ b/rust/kernel/hrtimer.rs
> @@ -0,0 +1,231 @@
> +// 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.
> +
> +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_init`.
> +#[repr(transparent)]
> +#[pin_data]
> +pub struct Timer<U> {
> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<U>,
> +}
> +
> +// SAFETY: A `Timer` can be moved to other threads and used/dropped from there.
> +unsafe impl<U> Send for Timer<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 Timer<U> {}
> +
> +impl<T> Timer<T> {
> +    /// Return an initializer for a new timer instance.
> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: TimerCallback,
> +    {
> +        pin_init!( Self {

I would remove the space after the `(`.
Would be great if we had rustfmt support for custom macros.

> +            // INVARIANTS: We initialize `timer` with `hrtimer_init` below.
> +            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.
> +                let function: *mut Option<_> =

Do you really need this type hint?

> +                    unsafe { core::ptr::addr_of_mut!((*place).function) };
> +
> +                // SAFETY: `function` points to a valid allocation and we have
> +                // exclusive access.
> +                unsafe { core::ptr::write(function, Some(T::CallbackTarget::run)) };
> +            }),
> +            _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`.
> +    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)) }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`Timer`].
> +///
> +/// 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 (hard or soft interrupt context).
> +///
> +/// Scheduling a timer returns a [`TimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the schedule function repeatedly, and
> +/// that more than one [`TimerHandle`] associated with a `TimerPointer` may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.
> +///
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: crate::sync::Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait TimerPointer: Sync + Sized {
> +    /// A handle representing a scheduled timer.
> +    ///
> +    /// If the timer is armed or if the timer callback is running when the
> +    /// handle is dropped, the drop method of `TimerHandle` should not return
> +    /// until the timer is unarmed and the callback has completed.
> +    ///
> +    /// Note: It must be safe to leak the handle.
> +    type TimerHandle: TimerHandle;

Why does this need to be an associated type? Couldn't we have a
`TimerHandle<T>` struct? The schedule function below could then return
`TimerHandle<Self>`.

> +
> +    /// Schedule the timer after `expires` time units. If the timer was already
> +    /// scheduled, it is rescheduled at the new expiry time.
> +    fn schedule(self, expires: Ktime) -> Self::TimerHandle;
> +}



> +/// Use to implement the [`HasTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_timer {
> +    (
> +        impl$({$($generics:tt)*})?
> +            HasTimer<$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::hrtimer::HasTimer<$timer_type>  for $self {

There is a double space in front of `for`.

> +            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>
> +            {
> +                // 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 274bdc1b0a82..55f846c5a849 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -34,6 +34,7 @@
>  pub mod error;
>  #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
>  pub mod firmware;
> +pub mod hrtimer;
>  pub mod init;
>  pub mod ioctl;
>  #[cfg(CONFIG_KUNIT)]
> --
> 2.46.0
> 
> 

Powered by blists - more mailing lists