Re: [PATCH] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg <nmi@metaspace.dk>]

Alice Ryhl <aliceryhl@google.com> writes:

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

👍

>
>> +/// 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.

Thanks.

>
>> +/// 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.

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 = 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`.
>

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