Date: Wed, 01 May 2024 13:37:00 +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

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

> Andreas!
>
> On Tue, Apr 30 2024 at 20:18, Andreas Hindborg wrote:
>> Thomas Gleixner <tglx@...utronix.de> writes:
>>> On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
>>>> +// 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.
>
> That's correct. All operations happen with the associated base lock held.
>
>>>> +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.
>
> Wouldn't expanding scope later require to change already existing call sites?

Yes, potentially. But I hear that Coccinelle is gaining Rust support 👍

>
>>>> +                    );
>>>> +                }
>>>> +
>>>> +                // 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?
>
> Please create that patch yourself and convert at least one C location to
> this new interface in a separate patch. THe remaining C cleanup can go
> from there and mostly be scripted with coccinelle.

Ok.

>
>>>> +/// [`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?
>
> The initial set is queued in
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
>
> for 6.10. Boqun has some updates on top IIRC. Your stuff should go
> through that branch too.

Ok.

>
>>>> +        // 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.
>
> I'm fine with taking a more complete API which does not require to
> change usage sites later on.

I will expand the API and send an updated patch when that is done 👍

BR Andreas

Powered by blists - more mailing lists