Re: [PATCH v13 3/5] rust: time: Introduce Instant type

From: FUJITA Tomonori
Date: Tue Apr 15 2025 - 23:46:57 EST

Next message: Jakub Kicinski: "Re: [PATCH 4/5] net: ch9200: add missing error handling in ch9200_bind()"
Previous message: Jiri Slaby: "Re: [PATCH v2 03/13] vt: properly support zero-width Unicode code points"
In reply to: Boqun Feng: "Re: [PATCH v13 3/5] rust: time: Introduce Instant type"
Next in thread: FUJITA Tomonori: "[PATCH v13 4/5] rust: time: Add wrapper for fsleep() function"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 15 Apr 2025 11:01:30 -0700
Boqun Feng <boqun.feng@xxxxxxxxx> wrote:

> 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@xxxxxxxxxx> wrote:
>>
>> > "Boqun Feng" <boqun.feng@xxxxxxxxx> 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@xxxxxxxxx>
>> >>
>> >> I probably need to drop my Reviewed-by because of something below:
>> >>
>> >>> Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
>> >>> Reviewed-by: Fiona Behrens <me@xxxxxxxxxx>
>> >>> Tested-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
>> >>> Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
>> >>> ---
>> >> [...]
>> >>> 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?

Either is fine with me. I'll leave it to Andreas' judgment.

Andreas, if you like Boqun's approach, I'll replace the third patch
with the following one and send v14.

I added Ktime struct to hrtimer.rs so the well-reviewed changes to
time.rs remain unchanged.

---
rust: time: Introduce Instant type

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

Note that hrtimer still uses the Ktime type for now. It will be
replaced with Instant and Delta types later.

Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
Reviewed-by: Fiona Behrens <me@xxxxxxxxxx>
Tested-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
---
rust/kernel/time.rs | 77 +++++++++++++++--------------
rust/kernel/time/hrtimer.rs | 17 ++++++-
rust/kernel/time/hrtimer/arc.rs | 2 +-
rust/kernel/time/hrtimer/pin.rs | 2 +-
rust/kernel/time/hrtimer/pin_mut.rs | 4 +-
rust/kernel/time/hrtimer/tbox.rs | 2 +-
6 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index e00b9a853e6a..a8089a98da9e 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -5,6 +5,22 @@
//! This module contains the kernel APIs related to time and timers that
//! have been ported or wrapped for usage by Rust code in the kernel.
//!
+//! There are two types in this module:
+//!
+//! - The [`Instant`] type represents a specific point in time.
+//! - The [`Delta`] type represents a span of time.
+//!
+//! Note that the C side uses `ktime_t` type to represent both. However, timestamp
+//! and timedelta are different. To avoid confusion, we use two different types.
+//!
+//! A [`Instant`] object can be created by calling the [`Instant::now()`] function.
+//! It represents a point in time at which the object was created.
+//! By calling the [`Instant::elapsed()`] method, a [`Delta`] object representing
+//! the elapsed time can be created. The [`Delta`] object can also be created
+//! by subtracting two [`Instant`] objects.
+//!
+//! A [`Delta`] type supports methods to retrieve the duration in various units.
+//!
//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).

@@ -33,59 +49,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
unsafe { bindings::__msecs_to_jiffies(msecs) }
}

-/// A Rust wrapper around a `ktime_t`.
+/// A specific point in time.
+///
+/// # Invariants
+///
+/// The `inner` value is in the range from 0 to `KTIME_MAX`.
#[repr(transparent)]
#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
+pub struct Instant {
inner: bindings::ktime_t,
}

-impl Ktime {
- /// Create a `Ktime` from a raw `ktime_t`.
- #[inline]
- pub fn from_raw(inner: bindings::ktime_t) -> Self {
- Self { inner }
- }
-
+impl Instant {
/// Get the current time using `CLOCK_MONOTONIC`.
#[inline]
- pub fn ktime_get() -> Self {
- // SAFETY: It is always safe to call `ktime_get` outside of NMI context.
- Self::from_raw(unsafe { bindings::ktime_get() })
- }
-
- /// Divide the number of nanoseconds by a compile-time constant.
- #[inline]
- fn divns_constant<const DIV: i64>(self) -> i64 {
- self.to_ns() / DIV
- }
-
- /// Returns the number of nanoseconds.
- #[inline]
- pub fn to_ns(self) -> i64 {
- self.inner
+ pub fn now() -> Self {
+ // INVARIANT: The `ktime_get()` function returns a value in the range
+ // from 0 to `KTIME_MAX`.
+ Self {
+ // SAFETY: It is always safe to call `ktime_get()` outside of NMI context.
+ inner: unsafe { bindings::ktime_get() },
+ }
}

- /// Returns the number of milliseconds.
+ /// Return the amount of time elapsed since the [`Instant`].
#[inline]
- pub fn to_ms(self) -> i64 {
- self.divns_constant::<NSEC_PER_MSEC>()
+ pub fn elapsed(&self) -> Delta {
+ Self::now() - *self
}
}

-/// Returns the number of milliseconds between two ktimes.
-#[inline]
-pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
- (later - earlier).to_ms()
-}
-
-impl core::ops::Sub for Ktime {
- type Output = Ktime;
+impl core::ops::Sub for Instant {
+ type Output = Delta;

+ // By the type invariant, it never overflows.
#[inline]
- fn sub(self, other: Ktime) -> Ktime {
- Self {
- inner: self.inner - other.inner,
+ fn sub(self, other: Instant) -> Delta {
+ Delta {
+ nanos: self.inner - other.inner,
}
}
}
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index ce53f8579d18..f46b41d3c31e 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -68,10 +68,25 @@
//! `start` operation.

use super::ClockId;
-use crate::{prelude::*, time::Ktime, types::Opaque};
+use crate::{prelude::*, types::Opaque};
use core::marker::PhantomData;
use pin_init::PinInit;

+/// A Rust wrapper around a `ktime_t`.
+#[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
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 4a984d85b4a1..ccf1e66e5b2d 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -5,10 +5,10 @@
use super::HrTimerCallback;
use super::HrTimerHandle;
use super::HrTimerPointer;
+use super::Ktime;
use super::RawHrTimerCallback;
use crate::sync::Arc;
use crate::sync::ArcBorrow;
-use crate::time::Ktime;

/// A handle for an `Arc<HasHrTimer<T>>` returned by a call to
/// [`HrTimerPointer::start`].
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
index f760db265c7b..293ca9cf058c 100644
--- a/rust/kernel/time/hrtimer/pin.rs
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -4,9 +4,9 @@
use super::HrTimer;
use super::HrTimerCallback;
use super::HrTimerHandle;
+use super::Ktime;
use super::RawHrTimerCallback;
use super::UnsafeHrTimerPointer;
-use crate::time::Ktime;
use core::pin::Pin;

/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
index 90c0351d62e4..6033572d35ad 100644
--- a/rust/kernel/time/hrtimer/pin_mut.rs
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -1,9 +1,9 @@
// SPDX-License-Identifier: GPL-2.0

use super::{
- HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, RawHrTimerCallback, UnsafeHrTimerPointer,
+ HasHrTimer, HrTimer, HrTimerCallback, HrTimerHandle, Ktime, RawHrTimerCallback,
+ UnsafeHrTimerPointer,
};
-use crate::time::Ktime;
use core::{marker::PhantomData, pin::Pin, ptr::NonNull};

/// A handle for a `Pin<&mut HasHrTimer>`. When the handle exists, the timer might
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
index 2071cae07234..29526a5da203 100644
--- a/rust/kernel/time/hrtimer/tbox.rs
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -5,9 +5,9 @@
use super::HrTimerCallback;
use super::HrTimerHandle;
use super::HrTimerPointer;
+use super::Ktime;
use super::RawHrTimerCallback;
use crate::prelude::*;
-use crate::time::Ktime;
use core::ptr::NonNull;

/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
--
2.43.0

Next message: Jakub Kicinski: "Re: [PATCH 4/5] net: ch9200: add missing error handling in ch9200_bind()"
Previous message: Jiri Slaby: "Re: [PATCH v2 03/13] vt: properly support zero-width Unicode code points"
In reply to: Boqun Feng: "Re: [PATCH v13 3/5] rust: time: Introduce Instant type"
Next in thread: FUJITA Tomonori: "[PATCH v13 4/5] rust: time: Add wrapper for fsleep() function"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]