Message-ID: <87ldtzhexi.fsf@kernel.org>
Date: Fri, 21 Feb 2025 09:36:57 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Tamir Duberstein" <tamird@...il.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "Frederic Weisbecker" <frederic@...nel.org>,
  "Thomas Gleixner" <tglx@...utronix.de>,  "Danilo Krummrich"
 <dakr@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno Lossin" <benno.lossin@...ton.me>,
  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Lyude Paul" <lyude@...hat.com>,  "Guangbo Cui" <2407018371@...com>,
  "Dirk Behme" <dirk.behme@...il.com>,  "Daniel Almeida"
 <daniel.almeida@...labora.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

"Tamir Duberstein" <tamird@...il.com> writes:

> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@...il.com> writes:
>>
>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@...nel.org> 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.
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.

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

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


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists