Message-ID: <8734ilmi5q.fsf@kernel.org>
Date: Wed, 18 Dec 2024 10:53:21 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Frederic Weisbecker" <frederic@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "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 v5 02/14] rust: hrtimer: introduce hrtimer support

"Frederic Weisbecker" <frederic@...nel.org> writes:

> Le Tue, Dec 17, 2024 at 04:17:33PM +0100, Andreas Hindborg a écrit :
>> 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..b09bdb8bc2037bf116a9a87b16271f4045f53aa9
>> --- /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.
>> +//!
>> +//! 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 Timer<U> {
>
> I seem to remember we had a debate on that. Why not call that Hrtimer?
> Timer is a bit too generic for a name. I suspect you'll need to introduce
> jiffies based timer bindings in the future as well and then some confusion
> may arise.

Yes, let's rename it. Good call.

>
>> +    #[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 {
>> +            // 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
>> +                // 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,
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>
> Always relative?

`TimerMode` is introduced in patch 12.

>
>> +                    );
>> +                }
>> +            }),
>> +            _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)) }
>> +    }
>> +
>> +    /// 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)]
>> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
>> +        // SAFETY: timer_ptr points to an allocation of at least `Timer` size.
>> +        let c_timer_ptr = unsafe { Timer::raw_get(self_ptr) };
>> +
>> +        // If handler is running, this will wait for handler to finish before
>> +        // returning.
>> +        // 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 [`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).
>> +///
>> +/// Starting a timer returns a [`TimerHandle`] that can be used to manipulate
>> +/// the timer. Note that it is OK to call the start 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 running timer.
>> +    ///
>> +    /// If the timer is running or if the timer callback is executing when the
>> +    /// handle is dropped, the drop method of `TimerHandle` should not return
>> +    /// until the timer is stopped and the callback has completed.
>> +    ///
>> +    /// Note: It must be safe to leak the handle.
>> +    type TimerHandle: TimerHandle;
>> +
>> +    /// 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 [`TimerPointer`] implementers to give the C timer callback a
>> +/// function to call.
>> +// This is split from `TimerPointer` to make it easier to specify trait bounds.
>> +pub trait RawTimerCallback {
>
> RawHrtimerCallback ?

Yes!

>
>> +    /// 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.
>> +pub trait TimerCallback {
>
> HrtimerCallback ? etc...

Will do a rename pass.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists