Message-ID: <CAJ-ks9mNidHZvWkFJE1jExc2oVk_bbJpiO_DRMrWu5nYhTpKgg@mail.gmail.com>
Date: Thu, 20 Feb 2025 16:37:13 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
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

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.

> >
> >
> >> +//! executed. A timer that is cancelled enters the **stopped** state.
> >> +//!
> >> +
> >> +use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
> >> +use core::marker::PhantomData;
> >> +
> >> +/// A timer backed by a C `struct hrtimer`.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
> >> +#[pin_data]
> >> +#[repr(C)]
> >> +pub struct HrTimer<T> {
> >> +    #[pin]
> >> +    timer: Opaque<bindings::hrtimer>,
> >> +    _t: PhantomData<T>,
> >> +}
> >> +
> >> +// SAFETY: Ownership of an `HrTimer` can be moved to other threads and
> >> +// used/dropped from there.
> >> +unsafe impl<T> Send for HrTimer<T> {}
> >> +
> >> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> >> +// timer from multiple threads
> >> +unsafe impl<T> Sync for HrTimer<T> {}
> >> +
> >> +impl<T> HrTimer<T> {
> >> +    /// Return an initializer for a new timer instance.
> >> +    pub fn new() -> impl PinInit<Self>
> >> +    where
> >> +        T: HrTimerCallback,
> >> +    {
> >> +        pin_init!(Self {
> >> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> >> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> >> +                // SAFETY: By design of `pin_init!`, `place` is a pointer to a
> >> +                // live allocation. hrtimer_setup will initialize `place` and
> >> +                // does not require `place` to be initialized prior to the call.
> >> +                unsafe {
> >> +                    bindings::hrtimer_setup(
> >> +                        place,
> >> +                        Some(T::CallbackTarget::run),
> >> +                        bindings::CLOCK_MONOTONIC as i32,
> >> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> >> +                    );
> >> +                }
> >> +            }),
> >> +            _t: PhantomData,
> >> +        })
> >> +    }
> >> +
> >> +    /// 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.

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

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

>
> >
> >> +pub unsafe trait HrTimerHandle {
> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> +    /// till the handler has finished.
> >> +    fn cancel(&mut self) -> bool;
> >> +}
> >> +
> >> +/// Implemented by structs that contain timer nodes.
> >> +///
> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> +/// implemented according to their documentation.
> >> +///
> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> +pub unsafe trait HasHrTimer<T> {
> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> +    const OFFSET: usize;
> >
> > Does this need to be part of the trait? As an alternative the provided
> > methods could be generated in the macro below and reduce the
> > opportunity to implement this trait incorrectly.
>
> There is no risk of implementing the trait wrong, because it is usually
> derived by a macro.

There's no risk when it's implemented by the macro, but you used the
word usually,  which means there is a risk.

> We need at least one of the methods to be able to have the type system
> verify that the type for which we implement `HasHrTImer` actually has a
> field with the name we specify, and that this field has the right type.
> And to have that, we need the OFFSET.

I don't follow this logic. OFFSET is calculated in the body of the
macro. I'm suggesting that the macro generate the method
implementations (which would no longer be provided). In effect I'm
saying: keep OFFSET private.

I'm also noticing now that the macro generates an implementation of
raw_get_timer *in addition to* the provided implementation. Why are
both needed?

>
>
>
> Best regards,
> Andreas Hindborg
>
>

Powered by blists - more mailing lists