[PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

Convert hrtimer to use `Instant` and `Delta`; remove the use of
`Ktime` from the hrtimer code, which was originally introduced as a
temporary workaround.

hrtimer uses either an `Instant` or a `Delta` as its expiration value,
depending on the mode specified at creation time. This patchset
replaces `HrTimerMode` enum with a trait-based abstraction and
associates each mode with either an `Instant` or a `Delta`. By
leveraging Rust's type system, this change enables `HrTimer` to be
statically associated with a specific `HrTimerMode`, the corresponding
`Instant` or `Delta`, and a `ClockSource`.

The `impl_has_hr_timer` macro is extended to allow specifying the
`HrTimerMode`. In the following example, it guarantees that the
`start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
`Delta` or an `Instant` with a different clock source will result in a
compile-time error:

struct Foo {
    #[pin]
    timer: HrTimer<Self>,
}

impl_has_hr_timer! {
    impl HasHrTimer<Self> for Foo {
        mode : AbsoluteMode<Monotonic>,
        field : self.timer
    }
}

This design eliminates runtime mismatches between expires types and
clock sources, and enables stronger type-level guarantees throughout
hrtimer.

This patchset can be applied on top of the typed clock sources patchset (v4):

https://lore.kernel.org/lkml/20250610093258.3435874-1-fujita.tomonori@gmail.com/

v3
- allow optional trailing comma for the field entry in impl_has_hr_timer! macro
v2: https://lore.kernel.org/lkml/20250609102418.3345792-1-fujita.tomonori@gmail.com/
- rebased on 6.16-rc1
- change impl_has_hr_timer! macro format
- remove define_hrtimer_mode! macro
- drop patch to change Delta's methods to take &self instead of self
- add patch to rename Delta's methods from as_* to into_*
v1: https://lore.kernel.org/lkml/20250504045959.238068-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (5):
  rust: time: Rename Delta's methods from as_* to into_*
  rust: time: Replace HrTimerMode enum with trait-based mode types
  rust: time: Add HrTimerExpires trait
  rust: time: Make HasHrTimer generic over HrTimerMode
  rust: time: Remove Ktime in hrtimer

 rust/kernel/time.rs                 |  19 +-
 rust/kernel/time/hrtimer.rs         | 281 ++++++++++++++++++----------
 rust/kernel/time/hrtimer/arc.rs     |   8 +-
 rust/kernel/time/hrtimer/pin.rs     |   8 +-
 rust/kernel/time/hrtimer/pin_mut.rs |   8 +-
 rust/kernel/time/hrtimer/tbox.rs    |   8 +-
 6 files changed, 218 insertions(+), 114 deletions(-)


base-commit: 8bffa361fb76742eb953ca024a9363c6e9357d65
-- 
2.43.0

[PATCH v3 1/5] rust: time: Rename Delta's methods from as_* to into_*

[FUJITA Tomonori]

Rename Delta's methods that take self from as_* to into_* to align
with Rust naming conventions.

Using `self` is more common Rust practice for small values that can be
freely copied [1].

Clippy warns against using as_* names for trait methods that take self
as follows:

warning: methods called as_* usually take self by reference or self by mutable reference
--> ~/linux/rust/kernel/time/hrtimer.rs:421:17
|
421 |     fn as_nanos(self) -> i64;

Rename the `Delta` struct's methods from as_nanos(), as_micros_ceil(),
and as_millis() to into_nanos(), into_micros_ceil(), and
into_millis(), respectively to maintain consistency with the other
function names.

Link: https://lore.kernel.org/lkml/aD1fgizC4FPT07vt@google.com/ [1]
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 9fd487276457..2a231c321afa 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -261,31 +261,31 @@ pub const fn from_secs(secs: i64) -> Self {
     /// Return `true` if the [`Delta`] spans no time.
     #[inline]
     pub fn is_zero(self) -> bool {
-        self.as_nanos() == 0
+        self.into_nanos() == 0
     }
 
     /// Return `true` if the [`Delta`] spans a negative amount of time.
     #[inline]
     pub fn is_negative(self) -> bool {
-        self.as_nanos() < 0
+        self.into_nanos() < 0
     }
 
     /// Return the number of nanoseconds in the [`Delta`].
     #[inline]
-    pub const fn as_nanos(self) -> i64 {
+    pub const fn into_nanos(self) -> i64 {
         self.nanos
     }
 
     /// Return the smallest number of microseconds greater than or equal
     /// to the value in the [`Delta`].
     #[inline]
-    pub const fn as_micros_ceil(self) -> i64 {
-        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+    pub const fn into_micros_ceil(self) -> i64 {
+        self.into_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
     }
 
     /// Return the number of milliseconds in the [`Delta`].
     #[inline]
-    pub const fn as_millis(self) -> i64 {
-        self.as_nanos() / NSEC_PER_MSEC
+    pub const fn into_millis(self) -> i64 {
+        self.into_nanos() / NSEC_PER_MSEC
     }
 }
-- 
2.43.0

[PATCH v3 2/5] rust: time: Replace HrTimerMode enum with trait-based mode types

[FUJITA Tomonori]

Replace the `HrTimerMode` enum with a trait-based approach that uses
zero-sized types to represent each mode of operation. Each mode now
implements the `HrTimerMode` trait.

This refactoring is a preparation for replacing raw `Ktime` in HrTimer
with the `Instant` and `Delta` types, and for making `HrTimer` generic
over a `ClockSource`.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time/hrtimer.rs | 164 ++++++++++++++++++++----------------
 1 file changed, 90 insertions(+), 74 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 280128d7e982..23f9f1ba8607 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -98,7 +98,7 @@ pub fn to_ns(self) -> i64 {
 pub struct HrTimer<T> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
-    mode: HrTimerMode,
+    mode: bindings::hrtimer_mode,
     _t: PhantomData<T>,
 }
 
@@ -112,7 +112,7 @@ unsafe impl<T> Sync for HrTimer<T> {}
 
 impl<T> HrTimer<T> {
     /// Return an initializer for a new timer instance.
-    pub fn new<U: ClockSource>(mode: HrTimerMode) -> impl PinInit<Self>
+    pub fn new<U: ClockSource, M: HrTimerMode>() -> impl PinInit<Self>
     where
         T: HrTimerCallback,
     {
@@ -127,11 +127,11 @@ pub fn new<U: ClockSource>(mode: HrTimerMode) -> impl PinInit<Self>
                         place,
                         Some(T::Pointer::run),
                         U::ID,
-                        mode.into_c(),
+                        M::C_MODE,
                     );
                 }
             }),
-            mode: mode,
+            mode: M::C_MODE,
             _t: PhantomData,
         })
     }
@@ -389,7 +389,7 @@ unsafe fn start(this: *const Self, expires: Ktime) {
                 Self::c_timer_ptr(this).cast_mut(),
                 expires.to_ns(),
                 0,
-                (*Self::raw_get_timer(this)).mode.into_c(),
+                (*Self::raw_get_timer(this)).mode,
             );
         }
     }
@@ -412,77 +412,93 @@ fn into_c(self) -> bindings::hrtimer_restart {
 }
 
 /// Operational mode of [`HrTimer`].
-// NOTE: Some of these have the same encoding on the C side, so we keep
-// `repr(Rust)` and convert elsewhere.
-#[derive(Clone, Copy, PartialEq, Eq, Debug)]
-pub enum HrTimerMode {
-    /// Timer expires at the given expiration time.
-    Absolute,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    Relative,
-    /// Timer does not move between CPU cores.
-    Pinned,
-    /// Timer handler is executed in soft irq context.
-    Soft,
-    /// Timer handler is executed in hard irq context.
-    Hard,
-    /// Timer expires at the given expiration time.
-    /// Timer does not move between CPU cores.
-    AbsolutePinned,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    /// Timer does not move between CPU cores.
-    RelativePinned,
-    /// Timer expires at the given expiration time.
-    /// Timer handler is executed in soft irq context.
-    AbsoluteSoft,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    /// Timer handler is executed in soft irq context.
-    RelativeSoft,
-    /// Timer expires at the given expiration time.
-    /// Timer does not move between CPU cores.
-    /// Timer handler is executed in soft irq context.
-    AbsolutePinnedSoft,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    /// Timer does not move between CPU cores.
-    /// Timer handler is executed in soft irq context.
-    RelativePinnedSoft,
-    /// Timer expires at the given expiration time.
-    /// Timer handler is executed in hard irq context.
-    AbsoluteHard,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    /// Timer handler is executed in hard irq context.
-    RelativeHard,
-    /// Timer expires at the given expiration time.
-    /// Timer does not move between CPU cores.
-    /// Timer handler is executed in hard irq context.
-    AbsolutePinnedHard,
-    /// Timer expires after the given expiration time interpreted as a duration from now.
-    /// Timer does not move between CPU cores.
-    /// Timer handler is executed in hard irq context.
-    RelativePinnedHard,
+pub trait HrTimerMode {
+    /// The C representation of hrtimer mode.
+    const C_MODE: bindings::hrtimer_mode;
 }
 
-impl HrTimerMode {
-    fn into_c(self) -> bindings::hrtimer_mode {
-        use bindings::*;
-        match self {
-            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
-            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
-            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
-            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
-            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
-            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
-            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
-            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
-            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
-            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
-            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
-            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
-            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
-            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
-            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
-        }
-    }
+/// Timer that expires at a fixed point in time.
+pub struct AbsoluteMode;
+
+impl HrTimerMode for AbsoluteMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS;
+}
+
+/// Timer that expires after a delay from now.
+pub struct RelativeMode;
+
+impl HrTimerMode for RelativeMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL;
+}
+
+/// Timer with absolute expiration time, pinned to its current CPU.
+pub struct AbsolutePinnedMode;
+
+impl HrTimerMode for AbsolutePinnedMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED;
+}
+
+/// Timer with relative expiration time, pinned to its current CPU.
+pub struct RelativePinnedMode;
+
+impl HrTimerMode for RelativePinnedMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED;
+}
+
+/// Timer with absolute expiration, handled in soft irq context.
+pub struct AbsoluteSoftMode;
+
+impl HrTimerMode for AbsoluteSoftMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_SOFT;
+}
+
+/// Timer with relative expiration, handled in soft irq context.
+pub struct RelativeSoftMode;
+
+impl HrTimerMode for RelativeSoftMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_SOFT;
+}
+
+/// Timer with absolute expiration, pinned to CPU and handled in soft irq context.
+pub struct AbsolutePinnedSoftMode;
+
+impl HrTimerMode for AbsolutePinnedSoftMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT;
+}
+
+/// Timer with relative expiration, pinned to CPU and handled in soft irq context.
+pub struct RelativePinnedSoftMode;
+
+impl HrTimerMode for RelativePinnedSoftMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT;
+}
+
+/// Timer with absolute expiration, handled in hard irq context.
+pub struct AbsoluteHardMode;
+
+impl HrTimerMode for AbsoluteHardMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_HARD;
+}
+
+/// Timer with relative expiration, handled in hard irq context.
+pub struct RelativeHardMode;
+
+impl HrTimerMode for RelativeHardMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_HARD;
+}
+
+/// Timer with absolute expiration, pinned to CPU and handled in hard irq context.
+pub struct AbsolutePinnedHardMode;
+
+impl HrTimerMode for AbsolutePinnedHardMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD;
+}
+
+/// Timer with relative expiration, pinned to CPU and handled in hard irq context.
+pub struct RelativePinnedHardMode;
+
+impl HrTimerMode for RelativePinnedHardMode {
+    const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD;
 }
 
 /// Use to implement the [`HasHrTimer<T>`] trait.
-- 
2.43.0

[PATCH v3 3/5] rust: time: Add HrTimerExpires trait

[FUJITA Tomonori]

Introduce the `HrTimerExpires` trait to represent types that can be
used as expiration values for high-resolution timers. Define a
required method, `into_nanos()`, which returns the expiration time as a
raw nanosecond value suitable for use with C's hrtimer APIs.

Also extend the `HrTimerMode` to use the `HrTimerExpires` trait.

This refactoring is a preparation for enabling hrtimer code to work
uniformly with both absolute and relative expiration modes.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs         |   5 ++
 rust/kernel/time/hrtimer.rs | 126 +++++++++++++++++++++++++-----------
 2 files changed, 95 insertions(+), 36 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 2a231c321afa..70bd3be0facc 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -194,6 +194,11 @@ pub fn now() -> Self {
     pub fn elapsed(&self) -> Delta {
         Self::now() - *self
     }
+
+    #[inline]
+    pub(crate) fn into_nanos(self) -> i64 {
+        self.inner
+    }
 }
 
 impl<C: ClockSource> core::ops::Sub for Instant<C> {
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 23f9f1ba8607..0ba87f1233cd 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -67,7 +67,7 @@
 //! A `restart` operation on a timer in the **stopped** state is equivalent to a
 //! `start` operation.
 
-use super::ClockSource;
+use super::{ClockSource, Delta, Instant};
 use crate::{prelude::*, types::Opaque};
 use core::marker::PhantomData;
 use pin_init::PinInit;
@@ -411,94 +411,148 @@ fn into_c(self) -> bindings::hrtimer_restart {
     }
 }
 
+/// Time representations that can be used as expiration values in [`HrTimer`].
+pub trait HrTimerExpires {
+    /// Converts the expiration time into a nanosecond representation.
+    ///
+    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
+    /// timer functions. The interpretation (absolute vs relative) depends on the
+    /// associated [HrTimerMode] in use.
+    fn into_nanos(self) -> i64;
+}
+
+impl<C: ClockSource> HrTimerExpires for Instant<C> {
+    fn into_nanos(self) -> i64 {
+        Instant::<C>::into_nanos(self)
+    }
+}
+
+impl HrTimerExpires for Delta {
+    fn into_nanos(self) -> i64 {
+        Delta::into_nanos(self)
+    }
+}
+
 /// Operational mode of [`HrTimer`].
 pub trait HrTimerMode {
     /// The C representation of hrtimer mode.
     const C_MODE: bindings::hrtimer_mode;
+
+    /// Type representing the clock source.
+    type Clock: ClockSource;
+
+    /// Type representing the expiration specification (absolute or relative time).
+    type Expires: HrTimerExpires;
 }
 
 /// Timer that expires at a fixed point in time.
-pub struct AbsoluteMode;
+pub struct AbsoluteMode<C: ClockSource>(PhantomData<C>);
 
-impl HrTimerMode for AbsoluteMode {
+impl<C: ClockSource> HrTimerMode for AbsoluteMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS;
+
+    type Clock = C;
+    type Expires = Instant<C>;
 }
 
 /// Timer that expires after a delay from now.
-pub struct RelativeMode;
+pub struct RelativeMode<C: ClockSource>(PhantomData<C>);
 
-impl HrTimerMode for RelativeMode {
+impl<C: ClockSource> HrTimerMode for RelativeMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Timer with absolute expiration time, pinned to its current CPU.
-pub struct AbsolutePinnedMode;
-
-impl HrTimerMode for AbsolutePinnedMode {
+pub struct AbsolutePinnedMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for AbsolutePinnedMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED;
+
+    type Clock = C;
+    type Expires = Instant<C>;
 }
 
 /// Timer with relative expiration time, pinned to its current CPU.
-pub struct RelativePinnedMode;
-
-impl HrTimerMode for RelativePinnedMode {
+pub struct RelativePinnedMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for RelativePinnedMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Timer with absolute expiration, handled in soft irq context.
-pub struct AbsoluteSoftMode;
-
-impl HrTimerMode for AbsoluteSoftMode {
+pub struct AbsoluteSoftMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for AbsoluteSoftMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_SOFT;
+
+    type Clock = C;
+    type Expires = Instant<C>;
 }
 
 /// Timer with relative expiration, handled in soft irq context.
-pub struct RelativeSoftMode;
-
-impl HrTimerMode for RelativeSoftMode {
+pub struct RelativeSoftMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for RelativeSoftMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_SOFT;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Timer with absolute expiration, pinned to CPU and handled in soft irq context.
-pub struct AbsolutePinnedSoftMode;
-
-impl HrTimerMode for AbsolutePinnedSoftMode {
+pub struct AbsolutePinnedSoftMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for AbsolutePinnedSoftMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT;
-}
 
-/// Timer with relative expiration, pinned to CPU and handled in soft irq context.
-pub struct RelativePinnedSoftMode;
+    type Clock = C;
+    type Expires = Instant<C>;
+}
 
-impl HrTimerMode for RelativePinnedSoftMode {
+/// Timer with absolute expiration, pinned to CPU and handled in soft irq context.
+pub struct RelativePinnedSoftMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for RelativePinnedSoftMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Timer with absolute expiration, handled in hard irq context.
-pub struct AbsoluteHardMode;
-
-impl HrTimerMode for AbsoluteHardMode {
+pub struct AbsoluteHardMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for AbsoluteHardMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_HARD;
+
+    type Clock = C;
+    type Expires = Instant<C>;
 }
 
 /// Timer with relative expiration, handled in hard irq context.
-pub struct RelativeHardMode;
-
-impl HrTimerMode for RelativeHardMode {
+pub struct RelativeHardMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for RelativeHardMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_HARD;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Timer with absolute expiration, pinned to CPU and handled in hard irq context.
-pub struct AbsolutePinnedHardMode;
-
-impl HrTimerMode for AbsolutePinnedHardMode {
+pub struct AbsolutePinnedHardMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for AbsolutePinnedHardMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD;
+
+    type Clock = C;
+    type Expires = Instant<C>;
 }
 
 /// Timer with relative expiration, pinned to CPU and handled in hard irq context.
-pub struct RelativePinnedHardMode;
-
-impl HrTimerMode for RelativePinnedHardMode {
+pub struct RelativePinnedHardMode<C: ClockSource>(PhantomData<C>);
+impl<C: ClockSource> HrTimerMode for RelativePinnedHardMode<C> {
     const C_MODE: bindings::hrtimer_mode = bindings::hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD;
+
+    type Clock = C;
+    type Expires = Delta;
 }
 
 /// Use to implement the [`HasHrTimer<T>`] trait.
-- 
2.43.0

[PATCH v3 4/5] rust: time: Make HasHrTimer generic over HrTimerMode

[FUJITA Tomonori]

Add a `TimerMode` associated type to the `HasHrTimer` trait to
represent the operational mode of the timer, such as absolute or
relative expiration. This new type must implement the `HrTimerMode`
trait, which defines how expiration values are interpreted.

Update the `start()` method to accept an `expires` parameter of type
`<Self::TimerMode as HrTimerMode>::Expires` instead of the fixed `Ktime`.
This enables different timer modes to provide strongly typed expiration
values, such as `Instant<C>` or `Delta`.

The `impl_has_hr_timer` macro is also extended to allow specifying the
`HrTimerMode`. In the following example, it guarantees that the
`start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
`Delta` or an `Instant` with a different clock source will result in a
compile-time error:

struct Foo {
    #[pin]
    timer: HrTimer<Self>,
}

impl_has_hr_timer! {
    impl HasHrTimer<Self> for Foo {
        mode : AbsoluteMode<Monotonic>,
        field : self.timer
    }
}

This design eliminates runtime mismatches between expires types and
clock sources, and enables stronger type-level guarantees throughout
hrtimer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time/hrtimer.rs         | 55 ++++++++++++++++++++++-------
 rust/kernel/time/hrtimer/arc.rs     |  8 +++--
 rust/kernel/time/hrtimer/pin.rs     |  8 +++--
 rust/kernel/time/hrtimer/pin_mut.rs |  8 +++--
 rust/kernel/time/hrtimer/tbox.rs    |  8 +++--
 5 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 0ba87f1233cd..4ca153f8fce6 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -98,7 +98,6 @@ pub fn to_ns(self) -> i64 {
 pub struct HrTimer<T> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
-    mode: bindings::hrtimer_mode,
     _t: PhantomData<T>,
 }
 
@@ -112,9 +111,10 @@ unsafe impl<T> Sync for HrTimer<T> {}
 
 impl<T> HrTimer<T> {
     /// Return an initializer for a new timer instance.
-    pub fn new<U: ClockSource, M: HrTimerMode>() -> impl PinInit<Self>
+    pub fn new() -> impl PinInit<Self>
     where
         T: HrTimerCallback,
+        T: HasHrTimer<T>,
     {
         pin_init!(Self {
             // INVARIANT: We initialize `timer` with `hrtimer_setup` below.
@@ -126,12 +126,11 @@ pub fn new<U: ClockSource, M: HrTimerMode>() -> impl PinInit<Self>
                     bindings::hrtimer_setup(
                         place,
                         Some(T::Pointer::run),
-                        U::ID,
-                        M::C_MODE,
+                        <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::Clock::ID,
+                        <T as HasHrTimer<T>>::TimerMode::C_MODE,
                     );
                 }
             }),
-            mode: M::C_MODE,
             _t: PhantomData,
         })
     }
@@ -193,6 +192,11 @@ pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
 /// exist. A timer can be manipulated through any of the handles, and a handle
 /// may represent a cancelled timer.
 pub trait HrTimerPointer: Sync + Sized {
+    /// The operational mode associated with this timer.
+    ///
+    /// This defines how the expiration value is interpreted.
+    type TimerMode: HrTimerMode;
+
     /// A handle representing a started or restarted timer.
     ///
     /// If the timer is running or if the timer callback is executing when the
@@ -205,7 +209,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: <Self::TimerMode as HrTimerMode>::Expires) -> Self::TimerHandle;
 }
 
 /// Unsafe version of [`HrTimerPointer`] for situations where leaking the
@@ -220,6 +224,11 @@ pub trait HrTimerPointer: Sync + Sized {
 /// [`UnsafeHrTimerPointer`] outlives any associated [`HrTimerPointer::TimerHandle`]
 /// instances.
 pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
+    /// The operational mode associated with this timer.
+    ///
+    /// This defines how the expiration value is interpreted.
+    type TimerMode: HrTimerMode;
+
     /// A handle representing a running timer.
     ///
     /// # Safety
@@ -236,7 +245,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: Ktime) -> Self::TimerHandle;
+    unsafe fn start(self, expires: <Self::TimerMode as HrTimerMode>::Expires) -> Self::TimerHandle;
 }
 
 /// A trait for stack allocated timers.
@@ -246,9 +255,14 @@ 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 {
+    /// The operational mode associated with this timer.
+    ///
+    /// This defines how the expiration value is interpreted.
+    type TimerMode: HrTimerMode;
+
     /// 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: Ktime, f: F) -> T
+    fn start_scoped<T, F>(self, expires: <Self::TimerMode as HrTimerMode>::Expires, f: F) -> T
     where
         F: FnOnce() -> T;
 }
@@ -260,7 +274,13 @@ unsafe impl<T> ScopedHrTimerPointer for T
 where
     T: UnsafeHrTimerPointer,
 {
-    fn start_scoped<U, F>(self, expires: Ktime, f: F) -> U
+    type TimerMode = T::TimerMode;
+
+    fn start_scoped<U, F>(
+        self,
+        expires: <<T as UnsafeHrTimerPointer>::TimerMode as HrTimerMode>::Expires,
+        f: F,
+    ) -> U
     where
         F: FnOnce() -> U,
     {
@@ -335,6 +355,11 @@ pub unsafe trait HrTimerHandle {
 /// their documentation. All the methods of this trait must operate on the same
 /// field.
 pub unsafe trait HasHrTimer<T> {
+    /// The operational mode associated with this timer.
+    ///
+    /// This defines how the expiration value is interpreted.
+    type TimerMode: HrTimerMode;
+
     /// Return a pointer to the [`HrTimer`] within `Self`.
     ///
     /// This function is useful to get access to the value without creating
@@ -382,14 +407,14 @@ 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: Ktime) {
+    unsafe fn start(this: *const Self, expires: <Self::TimerMode as HrTimerMode>::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.to_ns(),
+                expires.into_nanos(),
                 0,
-                (*Self::raw_get_timer(this)).mode,
+                <Self::TimerMode as HrTimerMode>::Clock::ID as u32,
             );
         }
     }
@@ -566,12 +591,16 @@ macro_rules! impl_has_hr_timer {
         impl$({$($generics:tt)*})?
             HasHrTimer<$timer_type:ty>
             for $self:ty
-        { self.$field:ident }
+        {
+            mode : $mode:ty,
+            field : self.$field:ident $(,)?
+        }
         $($rest:tt)*
     ) => {
         // SAFETY: This implementation of `raw_get_timer` only compiles if the
         // field has the right type.
         unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
+            type TimerMode = $mode;
 
             #[inline]
             unsafe fn raw_get_timer(
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index ccf1e66e5b2d..ed490a7a8950 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -4,8 +4,8 @@
 use super::HrTimer;
 use super::HrTimerCallback;
 use super::HrTimerHandle;
+use super::HrTimerMode;
 use super::HrTimerPointer;
-use super::Ktime;
 use super::RawHrTimerCallback;
 use crate::sync::Arc;
 use crate::sync::ArcBorrow;
@@ -54,9 +54,13 @@ impl<T> HrTimerPointer for Arc<T>
     T: HasHrTimer<T>,
     T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
 {
+    type TimerMode = <T as HasHrTimer<T>>::TimerMode;
     type TimerHandle = ArcHrTimerHandle<T>;
 
-    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
+    fn start(
+        self,
+        expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::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 293ca9cf058c..550aad28d987 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -4,7 +4,7 @@
 use super::HrTimer;
 use super::HrTimerCallback;
 use super::HrTimerHandle;
-use super::Ktime;
+use super::HrTimerMode;
 use super::RawHrTimerCallback;
 use super::UnsafeHrTimerPointer;
 use core::pin::Pin;
@@ -54,9 +54,13 @@ unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a T>
     T: HasHrTimer<T>,
     T: HrTimerCallback<Pointer<'a> = Self>,
 {
+    type TimerMode = <T as HasHrTimer<T>>::TimerMode;
     type TimerHandle = PinHrTimerHandle<'a, T>;
 
-    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
+    unsafe fn start(
+        self,
+        expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::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 6033572d35ad..bacd3d5d972a 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use super::{
-    HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, Ktime, RawHrTimerCallback,
+    HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, HrTimerMode, RawHrTimerCallback,
     UnsafeHrTimerPointer,
 };
 use core::{marker::PhantomData, pin::Pin, ptr::NonNull};
@@ -52,9 +52,13 @@ unsafe impl<'a, T> UnsafeHrTimerPointer for Pin<&'a mut T>
     T: HasHrTimer<T>,
     T: HrTimerCallback<Pointer<'a> = Self>,
 {
+    type TimerMode = <T as HasHrTimer<T>>::TimerMode;
     type TimerHandle = PinMutHrTimerHandle<'a, T>;
 
-    unsafe fn start(mut self, expires: Ktime) -> Self::TimerHandle {
+    unsafe fn start(
+        mut self,
+        expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::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 29526a5da203..ec08303315f2 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -4,8 +4,8 @@
 use super::HrTimer;
 use super::HrTimerCallback;
 use super::HrTimerHandle;
+use super::HrTimerMode;
 use super::HrTimerPointer;
-use super::Ktime;
 use super::RawHrTimerCallback;
 use crate::prelude::*;
 use core::ptr::NonNull;
@@ -64,9 +64,13 @@ impl<T, A> HrTimerPointer for Pin<Box<T, A>>
     T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
     A: crate::alloc::Allocator,
 {
+    type TimerMode = <T as HasHrTimer<T>>::TimerMode;
     type TimerHandle = BoxHrTimerHandle<T, A>;
 
-    fn start(self, expires: Ktime) -> Self::TimerHandle {
+    fn start(
+        self,
+        expires: <<T as HasHrTimer<T>>::TimerMode as HrTimerMode>::Expires,
+    ) -> Self::TimerHandle {
         // SAFETY:
         //  - We will not move out of this box during timer callback (we pass an
         //    immutable reference to the callback).
-- 
2.43.0

Re: [PATCH v3 4/5] rust: time: Make HasHrTimer generic over HrTimerMode

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> Add a `TimerMode` associated type to the `HasHrTimer` trait to
> represent the operational mode of the timer, such as absolute or
> relative expiration. This new type must implement the `HrTimerMode`
> trait, which defines how expiration values are interpreted.
>
> Update the `start()` method to accept an `expires` parameter of type
> `<Self::TimerMode as HrTimerMode>::Expires` instead of the fixed `Ktime`.
> This enables different timer modes to provide strongly typed expiration
> values, such as `Instant<C>` or `Delta`.
>
> The `impl_has_hr_timer` macro is also extended to allow specifying the
> `HrTimerMode`. In the following example, it guarantees that the
> `start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
> `Delta` or an `Instant` with a different clock source will result in a
> compile-time error:
>
> struct Foo {
>     #[pin]
>     timer: HrTimer<Self>,
> }
>
> impl_has_hr_timer! {
>     impl HasHrTimer<Self> for Foo {
>         mode : AbsoluteMode<Monotonic>,
>         field : self.timer
>     }
> }
>
> This design eliminates runtime mismatches between expires types and
> clock sources, and enables stronger type-level guarantees throughout
> hrtimer.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

[PATCH v3 5/5] rust: time: Remove Ktime in hrtimer

[FUJITA Tomonori]

Remove the use of `Ktime` from the hrtimer code, which was originally
introduced as a temporary workaround. The hrtimer has now been fully
converted to use the `Instant` and `Delta` types instead.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time/hrtimer.rs | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 4ca153f8fce6..3980a7c5f7db 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -72,22 +72,6 @@
 use core::marker::PhantomData;
 use pin_init::PinInit;
 
-/// A Rust wrapper around a `ktime_t`.
-// NOTE: Ktime is going to be removed when hrtimer is converted to Instant/Delta.
-#[repr(transparent)]
-#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
-    inner: bindings::ktime_t,
-}
-
-impl Ktime {
-    /// Returns the number of nanoseconds.
-    #[inline]
-    pub fn to_ns(self) -> i64 {
-        self.inner
-    }
-}
-
 /// A timer backed by a C `struct hrtimer`.
 ///
 /// # Invariants
-- 
2.43.0

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Tue, 10 Jun 2025 22:28:18 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Convert hrtimer to use `Instant` and `Delta`; remove the use of
> `Ktime` from the hrtimer code, which was originally introduced as a
> temporary workaround.
> 
> hrtimer uses either an `Instant` or a `Delta` as its expiration value,
> depending on the mode specified at creation time. This patchset
> replaces `HrTimerMode` enum with a trait-based abstraction and
> associates each mode with either an `Instant` or a `Delta`. By
> leveraging Rust's type system, this change enables `HrTimer` to be
> statically associated with a specific `HrTimerMode`, the corresponding
> `Instant` or `Delta`, and a `ClockSource`.
> 
> The `impl_has_hr_timer` macro is extended to allow specifying the
> `HrTimerMode`. In the following example, it guarantees that the
> `start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
> `Delta` or an `Instant` with a different clock source will result in a
> compile-time error:
> 
> struct Foo {
>     #[pin]
>     timer: HrTimer<Self>,
> }
> 
> impl_has_hr_timer! {
>     impl HasHrTimer<Self> for Foo {
>         mode : AbsoluteMode<Monotonic>,
>         field : self.timer
>     }
> }
> 
> This design eliminates runtime mismatches between expires types and
> clock sources, and enables stronger type-level guarantees throughout
> hrtimer.
> 
> This patchset can be applied on top of the typed clock sources patchset (v4):
> 
> https://lore.kernel.org/lkml/20250610093258.3435874-1-fujita.tomonori@gmail.com/

Andreas, whenever you get a chance, could you create a
timekeeping-next branch and merge the typed clock sources patchset and
this?

That would make it easier for me to work on top of them.

Thanks a lot for the review!

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

Hi Fujita,

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> Andreas, whenever you get a chance, could you create a
> timekeeping-next branch and merge the typed clock sources patchset and
> this?
>
> That would make it easier for me to work on top of them.
>
> Thanks a lot for the review!

I will let you know when I publish timekeeping-next, probably this week.
I applied the patches, but they are currently going through my own
testing.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

On Tue, 10 Jun 2025 22:28:18 +0900, FUJITA Tomonori wrote:
> Convert hrtimer to use `Instant` and `Delta`; remove the use of
> `Ktime` from the hrtimer code, which was originally introduced as a
> temporary workaround.
> 
> hrtimer uses either an `Instant` or a `Delta` as its expiration value,
> depending on the mode specified at creation time. This patchset
> replaces `HrTimerMode` enum with a trait-based abstraction and
> associates each mode with either an `Instant` or a `Delta`. By
> leveraging Rust's type system, this change enables `HrTimer` to be
> statically associated with a specific `HrTimerMode`, the corresponding
> `Instant` or `Delta`, and a `ClockSource`.
> 
> [...]

Applied, thanks!

[1/5] rust: time: Rename Delta's methods from as_* to into_*
      commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37
[2/5] rust: time: Replace HrTimerMode enum with trait-based mode types
      commit: 1d1102d098879b5c8fcd9babeadd2930b0a19259
[3/5] rust: time: Add HrTimerExpires trait
      commit: f7fe342fc72915f5eb2280d6ea38bc75d480bed0
[4/5] rust: time: Make HasHrTimer generic over HrTimerMode
      commit: ab57261bb9dea0e552a5cf8440e0688e6967163d
[5/5] rust: time: Remove Ktime in hrtimer
      commit: 994393295c89711531583f6de8f296a30b0d944a

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 12:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> [1/5] rust: time: Rename Delta's methods from as_* to into_*
>       commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37

Do we want this given the (~ongoing) discussion at

    https://lore.kernel.org/rust-for-linux/20250617144155.3903431-2-fujita.tomonori@gmail.com/

?

I noticed due to a conflict in linux-next today.

Cheers,
Miguel

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Jun 17, 2025 at 12:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> [1/5] rust: time: Rename Delta's methods from as_* to into_*
>>       commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37
>
> Do we want this given the (~ongoing) discussion at
>
>     https://lore.kernel.org/rust-for-linux/20250617144155.3903431-2-fujita.tomonori@gmail.com/
>
> ?
>

My plan is to merge it and go with `into_*`. There are pros and cons for
both `to_*` and `into_*`. If someone has objections, they can send a new
patch with rationale and we can revisit. Sounds OK?


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Miguel Ojeda]

On Tue, Jun 24, 2025 at 1:14 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> My plan is to merge it and go with `into_*`. There are pros and cons for
> both `to_*` and `into_*`. If someone has objections, they can send a new
> patch with rationale and we can revisit. Sounds OK?

I would just drop (or revert) the patch. The issue was under
discussion, and anyway it seems clear that `into_*` is not the right
choice from both the cost and ownership perspectives that we were
discussing in the other thread.

If this were not a rename and didn't had conflicts, then it wouldn't
be a big deal. But given it is wrong and already introduces pain for
others (and likely even more pain when we need to rename it back next
cycle), it doesn't look like a good idea to keep it.

It is early in the cycle anyway.

Cheers,
Miguel

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Jun 24, 2025 at 1:14 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> My plan is to merge it and go with `into_*`. There are pros and cons for
>> both `to_*` and `into_*`. If someone has objections, they can send a new
>> patch with rationale and we can revisit. Sounds OK?
>
> I would just drop (or revert) the patch. The issue was under
> discussion, and anyway it seems clear that `into_*` is not the right
> choice from both the cost and ownership perspectives that we were
> discussing in the other thread.

None of the options are the right choice. Cost and ownership _do_ line
up for `into_*` in this case. The mismatch is `into_*` is reserved for
`T: !Copy`.

>
> If this were not a rename and didn't had conflicts, then it wouldn't
> be a big deal. But given it is wrong

I do not think that is settled.

> and already introduces pain for
> others (and likely even more pain when we need to rename it back next
> cycle), it doesn't look like a good idea to keep it.

Ok, I'll drop it.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Tue, 24 Jun 2025 15:11:31 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>> and already introduces pain for
>> others (and likely even more pain when we need to rename it back next
>> cycle), it doesn't look like a good idea to keep it.
> 
> Ok, I'll drop it.

Do you want me to send the updated hrtimer conversion patchset
(using as_* names)?

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Tue, 24 Jun 2025 15:11:31 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>>> and already introduces pain for
>>> others (and likely even more pain when we need to rename it back next
>>> cycle), it doesn't look like a good idea to keep it.
>>
>> Ok, I'll drop it.
>
> Do you want me to send the updated hrtimer conversion patchset
> (using as_* names)?

No, I am just about finished fixing up the rest. You can check if it is
OK when I push.

Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Tue, 24 Jun 2025 15:11:31 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>>> and already introduces pain for
>>>> others (and likely even more pain when we need to rename it back next
>>>> cycle), it doesn't look like a good idea to keep it.
>>>
>>> Ok, I'll drop it.
>>
>> Do you want me to send the updated hrtimer conversion patchset
>> (using as_* names)?
>
> No, I am just about finished fixing up the rest. You can check if it is
> OK when I push.

I pushed it, please check.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Tue, 24 Jun 2025 21:03:24 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>>> and already introduces pain for
>>>>> others (and likely even more pain when we need to rename it back next
>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>
>>>> Ok, I'll drop it.
>>>
>>> Do you want me to send the updated hrtimer conversion patchset
>>> (using as_* names)?
>>
>> No, I am just about finished fixing up the rest. You can check if it is
>> OK when I push.
> 
> I pushed it, please check.

Thanks!

The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
to Instant structure:

+    #[inline]
+    pub(crate) fn as_nanos(&self) -> i64 {
+        self.inner
+    }

Would it be better to take self instead of &self?

pub(crate) fn as_nanos(self) -> i64 {

Because the as_nanos method on the Delta struct takes self, wouldn’t it
be better to keep it consistent? I think that my original patch adds
into_nanos() that takes self. 

This commit also adds HrTimerExpire strait, which as_nanos() method
takes &self:

+/// Time representations that can be used as expiration values in [`HrTimer`].
+pub trait HrTimerExpires {
+    /// Converts the expiration time into a nanosecond representation.
+    ///
+    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
+    /// timer functions. The interpretation (absolute vs relative) depends on the
+    /// associated [HrTimerMode] in use.
+    fn as_nanos(&self) -> i64;
+}

That's because as I reported, Clippy warns if as_* take self.

As Alice pointed out, Clippy doesn't warn if a type implements
Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
warn about as_nanos method that takes self:

+/// Time representations that can be used as expiration values in [`HrTimer`].
+pub trait HrTimerExpires: Copy {
+    /// Converts the expiration time into a nanosecond representation.
+    ///
+    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
+    /// timer functions. The interpretation (absolute vs relative) depends on the
+    /// associated [HrTimerMode] in use.
+    fn as_nanos(self) -> i64;
+}

I'm fine with either (taking &self or Adding Copy).

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Tue, 24 Jun 2025 21:03:24 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>>>> and already introduces pain for
>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>
>>>>> Ok, I'll drop it.
>>>>
>>>> Do you want me to send the updated hrtimer conversion patchset
>>>> (using as_* names)?
>>>
>>> No, I am just about finished fixing up the rest. You can check if it is
>>> OK when I push.
>>
>> I pushed it, please check.
>
> Thanks!
>
> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
> to Instant structure:
>
> +    #[inline]
> +    pub(crate) fn as_nanos(&self) -> i64 {
> +        self.inner
> +    }
>
> Would it be better to take self instead of &self?
>
> pub(crate) fn as_nanos(self) -> i64 {
>
> Because the as_nanos method on the Delta struct takes self, wouldn’t it
> be better to keep it consistent? I think that my original patch adds
> into_nanos() that takes self.
>
> This commit also adds HrTimerExpire strait, which as_nanos() method
> takes &self:
>
> +/// Time representations that can be used as expiration values in [`HrTimer`].
> +pub trait HrTimerExpires {
> +    /// Converts the expiration time into a nanosecond representation.
> +    ///
> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
> +    /// timer functions. The interpretation (absolute vs relative) depends on the
> +    /// associated [HrTimerMode] in use.
> +    fn as_nanos(&self) -> i64;
> +}
>
> That's because as I reported, Clippy warns if as_* take self.
>
> As Alice pointed out, Clippy doesn't warn if a type implements
> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
> warn about as_nanos method that takes self:
>
> +/// Time representations that can be used as expiration values in [`HrTimer`].
> +pub trait HrTimerExpires: Copy {
> +    /// Converts the expiration time into a nanosecond representation.
> +    ///
> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
> +    /// timer functions. The interpretation (absolute vs relative) depends on the
> +    /// associated [HrTimerMode] in use.
> +    fn as_nanos(self) -> i64;
> +}
>
> I'm fine with either (taking &self or Adding Copy).

Let's wait for the whole naming discussion to resolve before we do
anything. I am honestly a bit confused as to what is the most idiomatic
resolution here.

I think taking `&self` vs `self` makes not difference in codegen if we
mark the function `#[inline(always)]`.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Wed, 25 Jun 2025 10:19:59 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> 
>> On Tue, 24 Jun 2025 21:03:24 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>
>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>
>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>
>>>>>>> and already introduces pain for
>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>
>>>>>> Ok, I'll drop it.
>>>>>
>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>> (using as_* names)?
>>>>
>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>> OK when I push.
>>>
>>> I pushed it, please check.
>>
>> Thanks!
>>
>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>> to Instant structure:
>>
>> +    #[inline]
>> +    pub(crate) fn as_nanos(&self) -> i64 {
>> +        self.inner
>> +    }
>>
>> Would it be better to take self instead of &self?
>>
>> pub(crate) fn as_nanos(self) -> i64 {
>>
>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>> be better to keep it consistent? I think that my original patch adds
>> into_nanos() that takes self.
>>
>> This commit also adds HrTimerExpire strait, which as_nanos() method
>> takes &self:
>>
>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>> +pub trait HrTimerExpires {
>> +    /// Converts the expiration time into a nanosecond representation.
>> +    ///
>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>> +    /// associated [HrTimerMode] in use.
>> +    fn as_nanos(&self) -> i64;
>> +}
>>
>> That's because as I reported, Clippy warns if as_* take self.
>>
>> As Alice pointed out, Clippy doesn't warn if a type implements
>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>> warn about as_nanos method that takes self:
>>
>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>> +pub trait HrTimerExpires: Copy {
>> +    /// Converts the expiration time into a nanosecond representation.
>> +    ///
>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>> +    /// associated [HrTimerMode] in use.
>> +    fn as_nanos(self) -> i64;
>> +}
>>
>> I'm fine with either (taking &self or Adding Copy).
> 
> Let's wait for the whole naming discussion to resolve before we do
> anything. I am honestly a bit confused as to what is the most idiomatic
> resolution here.
> 
> I think taking `&self` vs `self` makes not difference in codegen if we
> mark the function `#[inline(always)]`.

I believe we've reached a consensus on the discussion about `&self` vs
`self`.

Miguel summarized nicely:

https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/

>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>> this thread that the compiler will be smart about this and just pass the
>> value. But it still feels wrong to me.
> 
> If inlined/private, yes; but not always.
> 
> So for small ("free") functions like this, it should generally not
> matter, since they should be inlined whether by manual marking or by
> the compiler.

> But, in general, it is not the same, and you can see cases where the
> compiler will still pass a pointer, and thus dereferences and writes
> to memory to take an address to pass it.
> 
> Which means that, outside small things like `as_*`, one should still
> probably take by value. Which creates an inconsistency.


I think that another consensus from this discussion is that the table
in the API naming guidelines doesn't cover this particular case.
Therefore, it makes sense to keep the current function name and move
forward.

Delta already provides `fn as_nanos(self)` (and drm uses in
linux-next, as you know) so I believe that Instant should use the same
interface.


That table needs improvement, but reaching consensus will likely take
time, it makes sense to address it independently.

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Wed, 25 Jun 2025 10:19:59 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Tue, 24 Jun 2025 21:03:24 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>
>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>
>>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>
>>>>>>>> and already introduces pain for
>>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>>
>>>>>>> Ok, I'll drop it.
>>>>>>
>>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>>> (using as_* names)?
>>>>>
>>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>>> OK when I push.
>>>>
>>>> I pushed it, please check.
>>>
>>> Thanks!
>>>
>>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>>> to Instant structure:
>>>
>>> +    #[inline]
>>> +    pub(crate) fn as_nanos(&self) -> i64 {
>>> +        self.inner
>>> +    }
>>>
>>> Would it be better to take self instead of &self?
>>>
>>> pub(crate) fn as_nanos(self) -> i64 {
>>>
>>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>>> be better to keep it consistent? I think that my original patch adds
>>> into_nanos() that takes self.
>>>
>>> This commit also adds HrTimerExpire strait, which as_nanos() method
>>> takes &self:
>>>
>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>> +pub trait HrTimerExpires {
>>> +    /// Converts the expiration time into a nanosecond representation.
>>> +    ///
>>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>>> +    /// associated [HrTimerMode] in use.
>>> +    fn as_nanos(&self) -> i64;
>>> +}
>>>
>>> That's because as I reported, Clippy warns if as_* take self.
>>>
>>> As Alice pointed out, Clippy doesn't warn if a type implements
>>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>>> warn about as_nanos method that takes self:
>>>
>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>> +pub trait HrTimerExpires: Copy {
>>> +    /// Converts the expiration time into a nanosecond representation.
>>> +    ///
>>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>>> +    /// associated [HrTimerMode] in use.
>>> +    fn as_nanos(self) -> i64;
>>> +}
>>>
>>> I'm fine with either (taking &self or Adding Copy).
>>
>> Let's wait for the whole naming discussion to resolve before we do
>> anything. I am honestly a bit confused as to what is the most idiomatic
>> resolution here.
>>
>> I think taking `&self` vs `self` makes not difference in codegen if we
>> mark the function `#[inline(always)]`.
>
> I believe we've reached a consensus on the discussion about `&self` vs
> `self`.

But not on the function name, right?

>
> Miguel summarized nicely:
>
> https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/
>
>>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>>> this thread that the compiler will be smart about this and just pass the
>>> value. But it still feels wrong to me.
>>
>> If inlined/private, yes; but not always.
>>
>> So for small ("free") functions like this, it should generally not
>> matter, since they should be inlined whether by manual marking or by
>> the compiler.
>
>> But, in general, it is not the same, and you can see cases where the
>> compiler will still pass a pointer, and thus dereferences and writes
>> to memory to take an address to pass it.
>>
>> Which means that, outside small things like `as_*`, one should still
>> probably take by value. Which creates an inconsistency.
>
>
> I think that another consensus from this discussion is that the table
> in the API naming guidelines doesn't cover this particular case.
> Therefore, it makes sense to keep the current function name and move
> forward.
>
> Delta already provides `fn as_nanos(self)` (and drm uses in
> linux-next, as you know) so I believe that Instant should use the same
> interface.
>
>
> That table needs improvement, but reaching consensus will likely take
> time, it makes sense to address it independently.

I am still uncertain what guidelines to follow inside the kernel. Last
time I applied a patch in this situation, I had to remove it again. I
would rather not have to do that.

Perhaps the best way forward is if you send the patch with the naming
and argument type you think is best, and then we continue the discussion
on that patch?


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Wed, 25 Jun 2025 10:19:59 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Tue, 24 Jun 2025 21:03:24 +0200
>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>>
>>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>>
>>>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>>
>>>>>>>>> and already introduces pain for
>>>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>>>
>>>>>>>> Ok, I'll drop it.
>>>>>>>
>>>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>>>> (using as_* names)?
>>>>>>
>>>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>>>> OK when I push.
>>>>>
>>>>> I pushed it, please check.
>>>>
>>>> Thanks!
>>>>
>>>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>>>> to Instant structure:
>>>>
>>>> +    #[inline]
>>>> +    pub(crate) fn as_nanos(&self) -> i64 {
>>>> +        self.inner
>>>> +    }
>>>>
>>>> Would it be better to take self instead of &self?
>>>>
>>>> pub(crate) fn as_nanos(self) -> i64 {
>>>>
>>>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>>>> be better to keep it consistent? I think that my original patch adds
>>>> into_nanos() that takes self.
>>>>
>>>> This commit also adds HrTimerExpire strait, which as_nanos() method
>>>> takes &self:
>>>>
>>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>>> +pub trait HrTimerExpires {
>>>> +    /// Converts the expiration time into a nanosecond representation.
>>>> +    ///
>>>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>>>> +    /// associated [HrTimerMode] in use.
>>>> +    fn as_nanos(&self) -> i64;
>>>> +}
>>>>
>>>> That's because as I reported, Clippy warns if as_* take self.
>>>>
>>>> As Alice pointed out, Clippy doesn't warn if a type implements
>>>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>>>> warn about as_nanos method that takes self:
>>>>
>>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>>> +pub trait HrTimerExpires: Copy {
>>>> +    /// Converts the expiration time into a nanosecond representation.
>>>> +    ///
>>>> +    /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>>> +    /// timer functions. The interpretation (absolute vs relative) depends on the
>>>> +    /// associated [HrTimerMode] in use.
>>>> +    fn as_nanos(self) -> i64;
>>>> +}
>>>>
>>>> I'm fine with either (taking &self or Adding Copy).
>>>
>>> Let's wait for the whole naming discussion to resolve before we do
>>> anything. I am honestly a bit confused as to what is the most idiomatic
>>> resolution here.
>>>
>>> I think taking `&self` vs `self` makes not difference in codegen if we
>>> mark the function `#[inline(always)]`.
>>
>> I believe we've reached a consensus on the discussion about `&self` vs
>> `self`.
>
> But not on the function name, right?
>
>>
>> Miguel summarized nicely:
>>
>> https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/
>>
>>>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>>>> this thread that the compiler will be smart about this and just pass the
>>>> value. But it still feels wrong to me.
>>>
>>> If inlined/private, yes; but not always.
>>>
>>> So for small ("free") functions like this, it should generally not
>>> matter, since they should be inlined whether by manual marking or by
>>> the compiler.
>>
>>> But, in general, it is not the same, and you can see cases where the
>>> compiler will still pass a pointer, and thus dereferences and writes
>>> to memory to take an address to pass it.
>>>
>>> Which means that, outside small things like `as_*`, one should still
>>> probably take by value. Which creates an inconsistency.
>>
>>
>> I think that another consensus from this discussion is that the table
>> in the API naming guidelines doesn't cover this particular case.
>> Therefore, it makes sense to keep the current function name and move
>> forward.
>>
>> Delta already provides `fn as_nanos(self)` (and drm uses in
>> linux-next, as you know) so I believe that Instant should use the same
>> interface.
>>
>>
>> That table needs improvement, but reaching consensus will likely take
>> time, it makes sense to address it independently.
>
> I am still uncertain what guidelines to follow inside the kernel. Last
> time I applied a patch in this situation, I had to remove it again. I
> would rather not have to do that.
>
> Perhaps the best way forward is if you send the patch with the naming
> and argument type you think is best, and then we continue the discussion
> on that patch?

This was discussed [1] and consensus was reached that `as_*` iwth pass
by value plus a `Copy` bound on the trait is the way to go for this
method.


Best regards,
Andreas Hindborg


[1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Thu, 10 Jul 2025 13:59:17 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> This was discussed [1] and consensus was reached that `as_*` iwth pass
> by value plus a `Copy` bound on the trait is the way to go for this
> method.
> 
> [1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions

Great, thanks!

Would you like me to send a patch for something ― for example, a patch
that applies the above changes to the current timekeeping-next tree?

Or would you prefer to reset timekeeping-next to an earlier commit for
a cleaner history, and have me send a patchset based on that?

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Thu, 10 Jul 2025 13:59:17 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> This was discussed [1] and consensus was reached that `as_*` iwth pass
>> by value plus a `Copy` bound on the trait is the way to go for this
>> method.
>>
>> [1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions
>
> Great, thanks!
>
> Would you like me to send a patch for something ― for example, a patch
> that applies the above changes to the current timekeeping-next tree?
>
> Or would you prefer to reset timekeeping-next to an earlier commit for
> a cleaner history, and have me send a patchset based on that?

Yes, we should align the code, so if you can change all the `as_nanos`
to take by value, that would be great.

I already sent the PR for the 6.17 merge window, so please send a new
patch, then we can get it in the next cycle.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Miguel Ojeda]

On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> None of the options are the right choice.

That is fine (it is also what I have been arguing in the other thread
and in previous times), but that does not imply `into_*` is not a bad
choice if we really want to follow upstream.

> Cost and ownership _do_ line
> up for `into_*` in this case.

No, ownership definitely doesn't line up: `Delta` is not `Copy` and
there is no conceptual ownership transfer. While it says "owned ->
owned", not being `Copy` is quite important here: the guidelines
clarify in an example for a `Copy` type that if the input is not
consumed then it should not be `into_*`.

Sure, "Variable" cost means anything could go there, but that doesn't
tell us much, i.e. if it was completely free, we could just as well
pick `as_`, which would actually provide some information since you
know it needs to be cheap.

So the whole argument for `into_*` is... "it says 'Variable' cost so
it lines up"?

Now, what I argued is that we may just as well define our own rules,
since that table is confusing and doesn't cover all cases. If we do
that, then you could propose things like "all owned->owned methods are
`into_*`", which I think is what you are essentially implying here.

> I do not think that is settled.

If you think so, then the patch shouldn't be applied.

Cheers,
Miguel

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[FUJITA Tomonori]

On Tue, 24 Jun 2025 23:13:49 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> None of the options are the right choice.
> 
> That is fine (it is also what I have been arguing in the other thread
> and in previous times), but that does not imply `into_*` is not a bad
> choice if we really want to follow upstream.
> 
>> Cost and ownership _do_ line
>> up for `into_*` in this case.
> 
> No, ownership definitely doesn't line up: `Delta` is not `Copy` and
> there is no conceptual ownership transfer. While it says "owned ->
> owned", not being `Copy` is quite important here: the guidelines

Just to clarify, Delta implements Copy:

#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
pub struct Delta {
    nanos: i64,
}

But it's just i64 so your point is that there is no conceptual
ownership transfer, right?


> clarify in an example for a `Copy` type that if the input is not
> consumed then it should not be `into_*`.
> 
> Sure, "Variable" cost means anything could go there, but that doesn't
> tell us much, i.e. if it was completely free, we could just as well
> pick `as_`, which would actually provide some information since you
> know it needs to be cheap.
> 
> So the whole argument for `into_*` is... "it says 'Variable' cost so
> it lines up"?
> 
> Now, what I argued is that we may just as well define our own rules,
> since that table is confusing and doesn't cover all cases. If we do
> that, then you could propose things like "all owned->owned methods are
> `into_*`", which I think is what you are essentially implying here.
> 
>> I do not think that is settled.
> 
> If you think so, then the patch shouldn't be applied.
> 
> Cheers,
> Miguel
> 

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Miguel Ojeda]

On Wed, Jun 25, 2025 at 1:30 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Just to clarify, Delta implements Copy:

Sorry, that was a typo: I meant it is `Copy` (the table says `into_*`
is for non`-`Copy` types).

Cheers,
Miguel

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Wed, Jun 25, 2025 at 1:30 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Just to clarify, Delta implements Copy:
>
> Sorry, that was a typo: I meant it is `Copy` (the table says `into_*`
> is for non`-`Copy` types).

The confusions will have no end.

Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> None of the options are the right choice.
>
> That is fine (it is also what I have been arguing in the other thread
> and in previous times), but that does not imply `into_*` is not a bad
> choice if we really want to follow upstream.
>
>> Cost and ownership _do_ line
>> up for `into_*` in this case.
>
> No, ownership definitely doesn't line up: `Delta` is not `Copy` and
> there is no conceptual ownership transfer. While it says "owned ->
> owned", not being `Copy` is quite important here: the guidelines
> clarify in an example for a `Copy` type that if the input is not
> consumed then it should not be `into_*`.

OK, that makes sense. And you are right, `T: Copy` does not line up, I
must have read too fast.

>
> Sure, "Variable" cost means anything could go there, but that doesn't
> tell us much, i.e. if it was completely free, we could just as well
> pick `as_`, which would actually provide some information since you
> know it needs to be cheap.
>
> So the whole argument for `into_*` is... "it says 'Variable' cost so
> it lines up"?

You are right, there is no argument outside of "variable cost", thanks
for clarifying.

> Now, what I argued is that we may just as well define our own rules,
> since that table is confusing and doesn't cover all cases. If we do
> that, then you could propose things like "all owned->owned methods are
> `into_*`", which I think is what you are essentially implying here.

I would actually prefer that the rust-lang guidelines were clarified so
that we could just defer to those.

>
>> I do not think that is settled.
>
> If you think so, then the patch shouldn't be applied.

I understand.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> On Tue, 10 Jun 2025 22:28:18 +0900, FUJITA Tomonori wrote:
>> Convert hrtimer to use `Instant` and `Delta`; remove the use of
>> `Ktime` from the hrtimer code, which was originally introduced as a
>> temporary workaround.
>> 
>> hrtimer uses either an `Instant` or a `Delta` as its expiration value,
>> depending on the mode specified at creation time. This patchset
>> replaces `HrTimerMode` enum with a trait-based abstraction and
>> associates each mode with either an `Instant` or a `Delta`. By
>> leveraging Rust's type system, this change enables `HrTimer` to be
>> statically associated with a specific `HrTimerMode`, the corresponding
>> `Instant` or `Delta`, and a `ClockSource`.
>> 
>> [...]
>
> Applied, thanks!
>
> [1/5] rust: time: Rename Delta's methods from as_* to into_*
>       commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37

I fixed up the application of this patch in timekeeping-next.


Best regards,
Andreas Hindborg