Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 3:37 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> "Tamir Duberstein" <tamird@xxxxxxxxx> writes:
>
> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
> >>
> >> "Tamir Duberstein" <tamird@xxxxxxxxx> writes:
> >>
> >> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
> >> >>
> >>
> >> [...]
> >>
> >> >> +//! ## State Diagram
> >> >> +//!
> >> >> +//! ```text
> >> >> +//!                  <-- Stop ----
> >> >> +//!                  <-- Cancel --
> >> >> +//!                  --- Start -->
> >> >> +//!        +---------+        +---------+
> >> >> +//!   O--->| Stopped |        | Running |---o
> >> >> +//!        +---------+        +---------+   |
> >> >> +//!                                  ^      |
> >> >> +//!                  <- Expire --    |      |
> >> >> +//!                                  o------o
> >> >> +//!                                   Restart
> >> >> +//! ```
> >> >> +//!
> >> >> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> >> >> +//! **started** with an **expiry** time. After the timer is started, it is
> >> >> +//! **running**. When the timer **expires**, the timer handler is executed.
> >> >> +//! After the handler has executed, the timer may be **restarted** or
> >> >> +//! **stopped**. A running timer can be **canceled** before it's handler is
> >> >
> >> > "it's" = it is. This should be "its" (possessive).
> >>
> >> Thanks 👍
> >>
> >> > Just to be clear, after the handler has executed and before the timer
> >> > has been **restarted** or **stopped** the timer is still in the
> >> > **running** state?
> >>
> >> It depends on the return value of the handler. If the handler returns restart,
> >> the timer the timer does not enter the stopped state. If the handler
> >> returns stop, the timer enters the stopped state.
> >>
> >> The timer is still considered to be in running state the handler is
> >> running.
> >>
> >> I can add this info to the section.
> >
> > Yeah, some clarification here would be useful.
>
> I'll add a paragraph 👍
>
> [...]
>
> >> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
> >> >> +    ///
> >> >> +    /// # Safety
> >> >> +    ///
> >> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> >> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> >> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> >> >> +        // because the caller of this function promises that `ptr` points to an
> >> >> +        // allocation of at least the size of `Self`.
> >> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> >> >> +    }
> >> >
> >> > Can you help me understand why the various functions here operate on
> >> > *const Self? I understand the need to obtain a C pointer to interact
> >> > with bindings, but I don't understand why we're dealing in raw
> >> > pointers to the abstraction rather than references.
> >>
> >> We cannot reference the `bindings::hrtimer` without wrapping it in
> >> `Opaque`. This would be the primary reason. At other times, we cannot
> >> produce references because we might not be able to prove that we satisfy
> >> the safety requirements for turning a pointer into a reference. If we
> >> are just doing offset arithmetic anyway, we don't need a reference.
> >
> > Why do we have a pointer, rather than a reference, to Self in the
> > first place? I think this is the key thing I don't understand.
>
> Perhaps it makes more sense if you look at the context. One of the entry
> points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
> HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
> handle contains an arc to a type that contains a `Timer` and implements
> `HasHrTImer`. To get to the timer, we need to do pointer manipulation.

This is the part I don't entirely understand. Why can't we chase
references all the way to the Timer? AFAICT the reason is that the
HasHrTimer trait is in terms of pointers, but couldn't it be in terms
of references? At least for getting the timer, not necessarily for
getting the timer's container.

> We only know how to get the `Timer` field via the `OFFSET`. The natural
> return value from the offset operation is a raw pointer. Rather than
> convert back to a reference, we stay in pointer land when we call
> `HrTimer::raw_cancel`, because we need a pointer to the
> `bindings::hrtimer` anyway, not a reference.

Sure, but this is breaking encapsulation in a way. Rather than having
a Timer type that owns the raw pointer and is responsible for all
operations on it, a variety of callers are allowed to get the raw
pointer and interact with the bindings.

>
> >
> >>
> >>
> >> > This extends to
> >> > HrTimerPointer, which is intended to be implemented by *pointers to*
> >> > structs that embed `HrTimer`; why isn't it implemented on by the
> >> > embedder itself?
> >>
> >> Not sure what you mean here. If you refer to for instance the
> >> implementation of `HrTimerPointer for Arc<T>`, I get why you might
> >> wonder, why does `HasHrTimer::start` not take a reference instead of a
> >> pointer? We could do that, but we would just immediately break it down
> >> again in the implementation of `HasHrTimer::start`. Might still be a
> >> good idea though.
> >
> > I was trying to say that my question (which I clarified above,
> > hopefully) extends to the description and name of this trait.
> > Specifically for this trait I don't understand why its semantics are
> > described in terms of pointers rather than references (and AsRef, to
> > allow for Arc and friends).
>
> All user facing APIs use references, not pointers. The raw pointer
> interfaces are for internal use only. I don't think we would gain
> anything from using `AsRef` internally. Perhaps you could clarify a bit more?

It's hard to prove the counter-factual, I don't know exactly what the
code would look like if we went with references everywhere. It just
seems to me that we're writing quite C-like Rust here, which implies
to me that we're not leveraging the compiler as much as we could.

>
> >
> >> >
> >> > I realize we discussed this on v6, sorry for not keeping up there.
> >>
> >> No worries, it is good that we discuss this.
> >>
> >> [...]
> >>
> >> >> +
> >> >> +/// A handle representing a potentially running timer.
> >> >> +///
> >> >> +/// More than one handle representing the same timer might exist.
> >> >> +///
> >> >> +/// # Safety
> >> >> +///
> >> >> +/// When dropped, the timer represented by this handle must be cancelled, if it
> >> >> +/// is running. If the timer handler is running when the handle is dropped, the
> >> >> +/// drop method must wait for the handler to finish before returning.
> >> >
> >> > Between this comment and the comment on cancel we say "if it is
> >> > running" 3 times. Can we say it just once, on the method, and here say
> >> > that cancel must be called in Drop?
> >>
> >> Well, the comment on `cancel` is just a description of what the function
> >> does. This piece of text is a safety requirement.
> >>
> >> We could make the safety requirement for implementing the trait "Implement
> >> the methods according to their documentation". But that would not help with
> >> the drop requirement.
> >
> > My suggestion is that the safety comment read: when dropped,
> > [Self::cancel] must be called. Something like that.
>
> We don't care how the timer is canceled, it just has to be canceled. It
> does not have to be by calling `Self::cancel`.

I understand. I'm only suggesting that we could reduce the repetition a bit.