Received-SPF: pass (google.com: domain of linux-kernel+bounces-162389-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Andreas Hindborg <nmi@metaspace.dk>
To: Alice Ryhl <aliceryhl@google.com>
Cc: a.hindborg@samsung.com,  alex.gaynor@gmail.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
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support
In-Reply-To: <20240429124937.414056-1-aliceryhl@google.com> (Alice Ryhl's
	message of "Mon, 29 Apr 2024 12:49:37 +0000")
References: <20240425094634.262674-1-nmi@metaspace.dk>
	<20240429124937.414056-1-aliceryhl@google.com>
User-Agent: mu4e 1.12.4; emacs 29.3
Date: Mon, 29 Apr 2024 15:47:47 +0200
Message-ID: <87ttjkjn5o.fsf@metaspace.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Alice Ryhl <aliceryhl@google.com> writes:

> Andreas Hindborg <nmi@metaspace.dk> writes:
>> From: Andreas Hindborg <a.hindborg@samsung.com>
>>=20
>> This patch adds support for intrusive use of the hrtimer system. For now=
, only
>> one timer can be embedded in a Rust struct.
>>=20
>> The hrtimer Rust API is based on the intrusive style pattern introduced =
by the
>> Rust workqueue API.
>>=20
>> 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 took some advice from Benno and merged `RawTimerCallback` with
`RawTimer` and renamed the resulting trait `TimerPointer`. There is not
really any reason they should be split for the `hrtimer` as far as I can
tell.

> 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".

I would remove the `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.

I think being able to call `my_timer_containing_struct.schedule()` is
nice.

>
>> +#[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.

Yes, it is very much a mirror. Yes, it is a leftover from trying to
support stack allocated timers. I will remove it.


> Also, as a generaly note, putting semicolons outside of unsafe blocks
> formats better.

=F0=9F=91=8D

>
>> +/// Implemented by pointer types that can be the target of a C timer ca=
llback.
>> +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.

Thanks.

>
>> +/// Implemented by pointers to structs that can the target of a timer c=
allback
>> +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.

I will update the doc comment, it should say "implemented by structs that
can be the target...". Thanks.

>
>> +impl<T> RawTimer for Arc<T>
>> +where
>> +    T: Send + Sync,
>> +    T: HasTimer<T>,
>> +{
>> +    fn schedule(self, expires: u64) {
>> +        let self_ptr =3D Arc::into_raw(self);
>> +
>> +        // SAFETY: `self_ptr` is a valid pointer to a `T`
>> +        let timer_ptr =3D unsafe { T::raw_get_timer(self_ptr) };
>> +
>> +        // `Timer` is `repr(transparent)`
>> +        let c_timer_ptr =3D timer_ptr.cast::<bindings::hrtimer>();
>
> I would add an `raw_get` method to `Timer` instead of this cast,
> analogous to `Work::raw_get`.
>

Why is that? It is a lot of extra code, extra safety comments, etc.

In any case, would you prefer to implement said method with a cast
(which we can because `Timer` is transparent), or by `Opaque::raw_get`:

`Opaque::raw_get(core::ptr::addr_of!((*ptr).timer))`


Best regards,
Andreas