Messages in this thread | | | From | Andreas Hindborg <> | Subject | Re: [PATCH] rust: hrtimer: introduce hrtimer support | Date | Tue, 30 Apr 2024 20:18:37 +0200 |
| |
Hi Thomas,
Thomas Gleixner <tglx@linutronix.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
| |