Messages in this thread | | | Date | Mon, 29 Apr 2024 12:49:37 +0000 | Subject | [PATCH] rust: hrtimer: introduce hrtimer support | From | Alice Ryhl <> |
| |
Andreas Hindborg <nmi@metaspace.dk> writes: > From: Andreas Hindborg <a.hindborg@samsung.com> > > 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@samsung.com>
This patch is very similar to the workqueue I implemented. It seems like we have the following correspondence between them:
* Your HasTimer is my HasWork. * Your RawTimerCallback is my WorkItemPointer. * Your TimerCallback is my WorkItem. * Your RawTimer is my RawWorkItem. (but the match isn't great here)
I think it would make sense to have the names be more consistent. I propose renaming RawTimerCallback to TimerCallbackPointer.
Or we can name them TimerEntry and RawTimerEntry?
I also note that the method on your RawTimer trait seems to be the public API of how you're supposed to schedule a timer, whereas the workqueue RawWorkItem only provides a raw low-level method, and instead has the "public API" be a function on the Workqueue struct.
I'm not such a big fan of having the primary method everying is supposed to use be a method on a trait whose name starts with "Raw". It's worth considering whether it makes more sense to have a free-standing function called `schedule_timer` and have that be how you're supposed to schedule timers, instead of the RawTimer trait.
> +#[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()); > + } > + } > +}
Assuming that this is mirroring the workqueue, then this is not necessary. The timer owns a refcount to the element, so the destructor cannot run while the timer is scheduled.
Also, as a generaly note, putting semicolons outside of unsafe blocks formats better.
> +/// 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; > +}
Safety comment is missing a space.
> +/// 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); > +}
The documentation says that this is implemented by pointers to structs, but that is not the case.
> +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>();
I would add an `raw_get` method to `Timer` instead of this cast, analogous to `Work::raw_get`.
> + // 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, > + 0, > + bindings::hrtimer_mode_HRTIMER_MODE_REL, > + ); > + } > + } > +}
Alice
| |