lkml.org 
[lkml]   [2024]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
Subject[PATCH] rust: hrtimer: introduce hrtimer support
From
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

\
 
 \ /
  Last update: 2024-04-29 14:50    [W:0.122 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site