Received-SPF: pass (google.com: domain of linux-kernel+bounces-162279-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Apr 2024 12:49:37 +0000
In-Reply-To: <20240425094634.262674-1-nmi@metaspace.dk>
Precedence: bulk
Mime-Version: 1.0
References: <20240425094634.262674-1-nmi@metaspace.dk>
Message-ID: <20240429124937.414056-1-aliceryhl@google.com>
Subject: [PATCH] rust: hrtimer: introduce hrtimer support
From: Alice Ryhl <aliceryhl@google.com>
To: nmi@metaspace.dk
Cc: a.hindborg@samsung.com, alex.gaynor@gmail.com, aliceryhl@google.com, 
	anna-maria@linutronix.de, benno.lossin@proton.me, bjorn3_gh@protonmail.com, 
	boqun.feng@gmail.com, frederic@kernel.org, gary@garyguo.net, 
	linux-kernel@vger.kernel.org, ojeda@kernel.org, 
	rust-for-linux@vger.kernel.org, tglx@linutronix.de, wedsonaf@gmail.com
Content-Type: text/plain; charset="utf-8"

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