Received-SPF: pass (google.com: domain of linux-kernel+bounces-164485-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Thomas Gleixner <tglx@linutronix.de>
To: Andreas Hindborg <nmi@metaspace.dk>, Miguel Ojeda <ojeda@kernel.org>,
 Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho
 <wedsonaf@gmail.com>, Anna-Maria Behnsen <anna-maria@linutronix.de>,
 Frederic Weisbecker <frederic@kernel.org>
Cc: Andreas Hindborg <a.hindborg@samsung.com>, Boqun Feng
 <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn?=
 Roy Baron
 <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Alice
 Ryhl <aliceryhl@google.com>, rust-for-linux@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support
In-Reply-To: <20240425094634.262674-1-nmi@metaspace.dk>
References: <20240425094634.262674-1-nmi@metaspace.dk>
Date: Tue, 30 Apr 2024 19:02:36 +0200
Message-ID: <87r0emss0j.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Andreas!

On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:

I'm looking at this purely from a hrtimer perspective and please excuse
my minimal rust knowledge.

> +// 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

Kinda. Using an hrtimer from different threads needs some thought in the
implementation as obviously ordering matters:

     T1                              T2
     hrtimer_start()                 hrtimer_cancel()

So depending on whether T1 gets the internal lock first or T2 the
outcome is different. If T1 gets it first the timer is canceled by
T2. If T2 gets it first the timer ends up armed.

> +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 {
> +            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,

This is odd. The initializer really should take a clock ID and a mode
argument. Otherwise you end up implementing a gazillion of different
timers.

> +                    );
> +                }
> +
> +                // 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)) };

We probably should introduce hrtimer_setup(timer, clockid, mode, function)
to avoid this construct. That would allow to cleanup existing C code too.

> +            }),
> +            _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());
> +        }
> +    }
> +}
> +
> +/// 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.

Timer callbacks happen in hard or soft interrupt context.

> +/// [`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);

Don't we have some time related rust types in the kernel by now?

> +}

> +/// Implemented by pointer types that can be the target of a C timer callback.
> +pub trait RawTimerCallback: RawTimer {
> +    /// 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,

same comment vs. time

> +                0,
> +                bindings::hrtimer_mode_HRTIMER_MODE_REL,

and mode.

> +            );
> +        }
> +    }
> +}
> +
> +impl<T> kernel::hrtimer::RawTimerCallback for Arc<T>
> +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

One of the common use cases of hrtimers is to create periodic schedules
where the timer callback advances the expiry value and returns
HRTIMER_RESTART. It might be not required for your initial use case at
hand, but you'll need that in the long run IMO.

Thanks,

        tglx