Message-ID: <67fe9efe.d40a0220.aa401.b05f@mx.google.com>
Date: Tue, 15 Apr 2025 11:01:30 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: a.hindborg@...nel.org, rust-for-linux@...r.kernel.org, gary@...yguo.net,
	me@...enk.dev, daniel.almeida@...labora.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	andrew@...n.ch, hkallweit1@...il.com, tmgross@...ch.edu,
	ojeda@...nel.org, alex.gaynor@...il.com, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com,
	aliceryhl@...gle.com, anna-maria@...utronix.de, frederic@...nel.org,
	tglx@...utronix.de, arnd@...db.de, jstultz@...gle.com,
	sboyd@...nel.org, mingo@...hat.com, peterz@...radead.org,
	juri.lelli@...hat.com, vincent.guittot@...aro.org,
	dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
	mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com,
	david.laight.linux@...il.com
Subject: Re: [PATCH v13 3/5] rust: time: Introduce Instant type

On Mon, Apr 14, 2025 at 08:59:54PM +0900, FUJITA Tomonori wrote:
> On Mon, 14 Apr 2025 09:04:14 +0200
> Andreas Hindborg <a.hindborg@...nel.org> wrote:
> 
> > "Boqun Feng" <boqun.feng@...il.com> writes:
> > 
> >> On Sun, Apr 13, 2025 at 07:43:08PM +0900, FUJITA Tomonori wrote:
> >>> Introduce a type representing a specific point in time. We could use
> >>> the Ktime type but C's ktime_t is used for both timestamp and
> >>> timedelta. To avoid confusion, introduce a new Instant type for
> >>> timestamp.
> >>>
> >>> Rename Ktime to Instant and modify their methods for timestamp.
> >>>
> >>> Implement the subtraction operator for Instant:
> >>>
> >>> Delta = Instant A - Instant B
> >>>
> >>> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> >>
> >> I probably need to drop my Reviewed-by because of something below:
> >>
> >>> Reviewed-by: Gary Guo <gary@...yguo.net>
> >>> Reviewed-by: Fiona Behrens <me@...enk.dev>
> >>> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
> >>> Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> >>> ---
> >> [...]
> >>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> >>> index ce53f8579d18..27243eaaf8ed 100644
> >>> --- a/rust/kernel/time/hrtimer.rs
> >>> +++ b/rust/kernel/time/hrtimer.rs
> >>> @@ -68,7 +68,7 @@
> >>>  //! `start` operation.
> >>>
> >>>  use super::ClockId;
> >>> -use crate::{prelude::*, time::Ktime, types::Opaque};
> >>> +use crate::{prelude::*, time::Instant, types::Opaque};
> >>>  use core::marker::PhantomData;
> >>>  use pin_init::PinInit;
> >>>
> >>> @@ -189,7 +189,7 @@ pub trait HrTimerPointer: Sync + Sized {
> >>>
> >>>      /// Start the timer with expiry after `expires` time units. If the timer was
> >>>      /// already running, it is restarted with the new expiry time.
> >>> -    fn start(self, expires: Ktime) -> Self::TimerHandle;
> >>> +    fn start(self, expires: Instant) -> Self::TimerHandle;
> >>
> >> We should be able to use what I suggested:
> >>
> >> 	https://lore.kernel.org/rust-for-linux/Z_ALZsnwN53ZPBrB@boqun-archlinux/
> >>
> >> to make different timer modes (rel or abs) choose different expire type.
> >>
> >> I don't think we can merge this patch as it is, unfortunately, because
> >> it doesn't make sense for a relative timer to take an Instant as expires
> >> value.
> > 
> > I told Tomo he could use `Instant` in this location and either he or I
> > would fix it up later [1].
> > 

I saw that, however, I don't think we can put `Instant` as the parameter
for HrTimerPointer::start() because we don't yet know how long would the
fix-it-up-later take. And it would confuse users if they need a put an
Instant for relative time.

> > I don't want to block the series on this since the new API is not worse
> > than the old one where Ktime is overloaded for both uses.

How about we keep Ktime? That is HrTimerPointer::start() still uses
Ktime, until we totally finish the refactoring as Tomo show below?
`Ktime` is much better here because it at least matches C API behavior,
we can remove `Ktime` once the dust is settled. Thoughts?

Regards,
Boqun

> 
> Here's the fix that I've worked on. As Boqun suggested, I added
> `HrTimerExpireMode` trait since `HrTimerMode` is already used. It
> compiles, but I'm not sure if it's what everyone had in mind.
> 
> Since many parts need to be made generic, I think the changes will be
> complicated. Rather than including this in the instant and duration
> patchset, I think it would be better to review this change separately.
> 
> 
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 27243eaaf8ed..db3f99662222 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -68,10 +68,16 @@
>  //! `start` operation.
>  
>  use super::ClockId;
> -use crate::{prelude::*, time::Instant, types::Opaque};
> +use crate::{prelude::*, types::Opaque};
>  use core::marker::PhantomData;
>  use pin_init::PinInit;
>  
> +pub trait HrTimerExpireMode {
> +    type Expires; /* either Delta or Instant */
> +
> +    fn as_nanos(expires: Self::Expires) -> bindings::ktime_t;
> +}
> +
>  /// A timer backed by a C `struct hrtimer`.
>  ///
>  /// # Invariants
> @@ -176,7 +182,7 @@ pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
>  /// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
>  /// exist. A timer can be manipulated through any of the handles, and a handle
>  /// may represent a cancelled timer.
> -pub trait HrTimerPointer: Sync + Sized {
> +pub trait HrTimerPointer<Mode: HrTimerExpireMode>: Sync + Sized {
>      /// A handle representing a started or restarted timer.
>      ///
>      /// If the timer is running or if the timer callback is executing when the
> @@ -189,7 +195,7 @@ pub trait HrTimerPointer: Sync + Sized {
>  
>      /// Start the timer with expiry after `expires` time units. If the timer was
>      /// already running, it is restarted with the new expiry time.
> -    fn start(self, expires: Instant) -> Self::TimerHandle;
> +    fn start(self, expires: Mode::Expires) -> Self::TimerHandle;
>  }
>  
>  /// Unsafe version of [`HrTimerPointer`] for situations where leaking the
> @@ -203,7 +209,7 @@ pub trait HrTimerPointer: Sync + Sized {
>  /// Implementers of this trait must ensure that instances of types implementing
>  /// [`UnsafeHrTimerPointer`] outlives any associated [`HrTimerPointer::TimerHandle`]
>  /// instances.
> -pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
> +pub unsafe trait UnsafeHrTimerPointer<Mode: HrTimerExpireMode>: Sync + Sized {
>      /// A handle representing a running timer.
>      ///
>      /// # Safety
> @@ -220,7 +226,7 @@ pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
>      ///
>      /// Caller promises keep the timer structure alive until the timer is dead.
>      /// Caller can ensure this by not leaking the returned [`Self::TimerHandle`].
> -    unsafe fn start(self, expires: Instant) -> Self::TimerHandle;
> +    unsafe fn start(self, expires: Mode::Expires) -> Self::TimerHandle;
>  }
>  
>  /// A trait for stack allocated timers.
> @@ -229,10 +235,10 @@ pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
>  ///
>  /// Implementers must ensure that `start_scoped` does not return until the
>  /// timer is dead and the timer handler is not running.
> -pub unsafe trait ScopedHrTimerPointer {
> +pub unsafe trait ScopedHrTimerPointer<Mode: HrTimerExpireMode> {
>      /// Start the timer to run after `expires` time units and immediately
>      /// after call `f`. When `f` returns, the timer is cancelled.
> -    fn start_scoped<T, F>(self, expires: Instant, f: F) -> T
> +    fn start_scoped<T, F>(self, expires: Mode::Expires, f: F) -> T
>      where
>          F: FnOnce() -> T;
>  }
> @@ -240,11 +246,12 @@ fn start_scoped<T, F>(self, expires: Instant, f: F) -> T
>  // SAFETY: By the safety requirement of [`UnsafeHrTimerPointer`], dropping the
>  // handle returned by [`UnsafeHrTimerPointer::start`] ensures that the timer is
>  // killed.
> -unsafe impl<T> ScopedHrTimerPointer for T
> +unsafe impl<T, Mode> ScopedHrTimerPointer<Mode> for T
>  where
> -    T: UnsafeHrTimerPointer,
> +    T: UnsafeHrTimerPointer<Mode>,
> +    Mode: HrTimerExpireMode,
>  {
> -    fn start_scoped<U, F>(self, expires: Instant, f: F) -> U
> +    fn start_scoped<U, F>(self, expires: Mode::Expires, f: F) -> U
>      where
>          F: FnOnce() -> U,
>      {
> @@ -319,6 +326,7 @@ pub unsafe trait HrTimerHandle {
>  /// their documentation. All the methods of this trait must operate on the same
>  /// field.
>  pub unsafe trait HasHrTimer<T> {
> +    type Mode: HrTimerExpireMode;
>      /// Return a pointer to the [`HrTimer`] within `Self`.
>      ///
>      /// This function is useful to get access to the value without creating
> @@ -366,12 +374,15 @@ unsafe fn c_timer_ptr(this: *const Self) -> *const bindings::hrtimer {
>      /// - `this` must point to a valid `Self`.
>      /// - Caller must ensure that the pointee of `this` lives until the timer
>      ///   fires or is canceled.
> -    unsafe fn start(this: *const Self, expires: Instant) {
> +    unsafe fn start(
> +        this: *const Self,
> +        expires: <<Self as HasHrTimer<T>>::Mode as HrTimerExpireMode>::Expires,
> +    ) {
>          // SAFETY: By function safety requirement, `this` is a valid `Self`.
>          unsafe {
>              bindings::hrtimer_start_range_ns(
>                  Self::c_timer_ptr(this).cast_mut(),
> -                expires.as_nanos(),
> +                Self::Mode::as_nanos(expires),
>                  0,
>                  (*Self::raw_get_timer(this)).mode.into_c(),
>              );
> diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
> index acc70a0ea1be..90cf0edf4509 100644
> --- a/rust/kernel/time/hrtimer/arc.rs
> +++ b/rust/kernel/time/hrtimer/arc.rs
> @@ -3,12 +3,12 @@
>  use super::HasHrTimer;
>  use super::HrTimer;
>  use super::HrTimerCallback;
> +use super::HrTimerExpireMode;
>  use super::HrTimerHandle;
>  use super::HrTimerPointer;
>  use super::RawHrTimerCallback;
>  use crate::sync::Arc;
>  use crate::sync::ArcBorrow;
> -use crate::time::Instant;
>  
>  /// A handle for an `Arc<HasHrTimer<T>>` returned by a call to
>  /// [`HrTimerPointer::start`].
> @@ -47,7 +47,7 @@ fn drop(&mut self) {
>      }
>  }
>  
> -impl<T> HrTimerPointer for Arc<T>
> +impl<T> HrTimerPointer<<T as HasHrTimer<T>>::Mode> for Arc<T>
>  where
>      T: 'static,
>      T: Send + Sync,
> @@ -56,7 +56,10 @@ impl<T> HrTimerPointer for Arc<T>
>  {
>      type TimerHandle = ArcHrTimerHandle<T>;
>  
> -    fn start(self, expires: Instant) -> ArcHrTimerHandle<T> {
> +    fn start(
> +        self,
> +        expires: <<T as HasHrTimer<T>>::Mode as HrTimerExpireMode>::Expires,
> +    ) -> ArcHrTimerHandle<T> {
>          // SAFETY:
>          //  - We keep `self` alive by wrapping it in a handle below.
>          //  - Since we generate the pointer passed to `start` from a valid
> diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
> index dba22d11a95f..5b79cbcaca3f 100644
> --- a/rust/kernel/time/hrtimer/pin.rs
> +++ b/rust/kernel/time/hrtimer/pin.rs
> @@ -3,6 +3,7 @@
>  use super::HasHrTimer;
>  use super::HrTimer;
>  use super::HrTimerCallback;
> +use super::HrTimerExpireMode;
>  use super::HrTimerHandle;
>  use super::RawHrTimerCallback;
>  use super::UnsafeHrTimerPointer;
> @@ -48,7 +49,7 @@ fn drop(&mut self) {
>  
>  // SAFETY: We capture the lifetime of `Self` when we create a `PinHrTimerHandle`,
>  // so `Self` will outlive the handle.
> -unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a T>
> +unsafe impl<'a, T> UnsafeHrTimerPointer<<T as HasHrTimer<T>>::Mode> for Pin<&'a T>
>  where
>      T: Send + Sync,
>      T: HasHrTimer<T>,
> @@ -56,7 +57,10 @@ unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a T>
>  {
>      type TimerHandle = PinHrTimerHandle<'a, T>;
>  
> -    unsafe fn start(self, expires: Instant) -> Self::TimerHandle {
> +    unsafe fn start(
> +        self,
> +        expires: <<T as HasHrTimer<T>>::Mode as HrTimerExpireMode>::Expires,
> +    ) -> Self::TimerHandle {
>          // Cast to pointer
>          let self_ptr: *const T = self.get_ref();
>  
> diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
> index aeff8e102e1d..82d7ecdbbfb6 100644
> --- a/rust/kernel/time/hrtimer/pin_mut.rs
> +++ b/rust/kernel/time/hrtimer/pin_mut.rs
> @@ -1,7 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  use super::{
> -    HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, RawHrTimerCallback, UnsafeHrTimerPointer,
> +    HasHrTimer, HrTimer, HrTimerCallback, HrTimerExpireMode, HrTimerHandle, RawHrTimerCallback,
> +    UnsafeHrTimerPointer,
>  };
>  use crate::time::Instant;
>  use core::{marker::PhantomData, pin::Pin, ptr::NonNull};
> @@ -46,7 +47,7 @@ fn drop(&mut self) {
>  
>  // SAFETY: We capture the lifetime of `Self` when we create a
>  // `PinMutHrTimerHandle`, so `Self` will outlive the handle.
> -unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a mut T>
> +unsafe impl<'a, T> UnsafeHrTimerPointer<<T as HasHrTimer<T>>::Mode> for Pin<&'a mut T>
>  where
>      T: Send + Sync,
>      T: HasHrTimer<T>,
> @@ -54,7 +55,10 @@ unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a mut T>
>  {
>      type TimerHandle = PinMutHrTimerHandle<'a, T>;
>  
> -    unsafe fn start(mut self, expires: Instant) -> Self::TimerHandle {
> +    unsafe fn start(
> +        mut self,
> +        expires: <<T as HasHrTimer<T>>::Mode as HrTimerExpireMode>::Expires,
> +    ) -> Self::TimerHandle {
>          // SAFETY:
>          // - We promise not to move out of `self`. We only pass `self`
>          //   back to the caller as a `Pin<&mut self>`.
> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
> index 3df4e359e9bb..21aa0aa61cc8 100644
> --- a/rust/kernel/time/hrtimer/tbox.rs
> +++ b/rust/kernel/time/hrtimer/tbox.rs
> @@ -3,6 +3,7 @@
>  use super::HasHrTimer;
>  use super::HrTimer;
>  use super::HrTimerCallback;
> +use super::HrTimerExpireMode;
>  use super::HrTimerHandle;
>  use super::HrTimerPointer;
>  use super::RawHrTimerCallback;
> @@ -56,7 +57,7 @@ fn drop(&mut self) {
>      }
>  }
>  
> -impl<T, A> HrTimerPointer for Pin<Box<T, A>>
> +impl<T, A> HrTimerPointer<<T as HasHrTimer<T>>::Mode> for Pin<Box<T, A>>
>  where
>      T: 'static,
>      T: Send + Sync,
> @@ -66,7 +67,10 @@ impl<T, A> HrTimerPointer for Pin<Box<T, A>>
>  {
>      type TimerHandle = BoxHrTimerHandle<T, A>;
>  
> -    fn start(self, expires: Instant) -> Self::TimerHandle {
> +    fn start(
> +        self,
> +        expires: <<T as HasHrTimer<T>>::Mode as HrTimerExpireMode>::Expires,
> +    ) -> Self::TimerHandle {
>          // SAFETY:
>          //  - We will not move out of this box during timer callback (we pass an
>          //    immutable reference to the callback).
> 

Powered by blists - more mailing lists