Message-ID: <87le4uk936.fsf@metaspace.dk>
Date: Tue, 30 Apr 2024 20:18:37 +0200
From: Andreas Hindborg <nmi@...aspace.dk>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Miguel Ojeda <ojeda@...nel.org>,  Alex Gaynor <alex.gaynor@...il.com>,
  Wedson Almeida Filho <wedsonaf@...il.com>,  Anna-Maria Behnsen
 <anna-maria@...utronix.de>,  Frederic Weisbecker <frederic@...nel.org>,
  Andreas Hindborg <a.hindborg@...sung.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>,  rust-for-linux@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support


Hi Thomas,

Thomas Gleixner <tglx@...utronix.de> writes:

> 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.

Thanks for taking a look!

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

That is all fine. What is meant here is that we will not get UB in the
`hrtimer` subsystem when racing these operations. As far as I can tell
from the C source, the operations are atomic, even though their
interleaving will not be deterministic.

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

I implemented the minimum set of features to satisfy the requirements
for the Rust null block driver. It is my understanding that most
maintainers of existing infrastructure prefers to have a user for the
implemented features, before wanting to merge them.

I can try to extend the abstractions to cover a more complete `hrtimer`
API. Or we can work on this subset and try to get that ready to merge,
and then expand scope later.

What would you prefer?

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

Do you want me to cook up a C patch for that, or would you prefer to do
that yourself?

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

Thanks, I'll be sure to add that to the documentation.

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

There are patches on the list, but I think they are not applied to any
tree yet? I did not want to depend on those patches before they are
staged somewhere. Would you prefer this patch on top of the Rust `ktime`
patches?

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

If you are OK with taking that feature without a user, I will gladly add
it.

Best regards,
Andreas

Powered by blists - more mailing lists