Message-Id: <20250414.205954.2258973048785103265.fujita.tomonori@gmail.com>
Date: Mon, 14 Apr 2025 20:59:54 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: a.hindborg@...nel.org
Cc: boqun.feng@...il.com, fujita.tomonori@...il.com,
 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, 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 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.

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