Feedback-ID: iad51458e:Fastmail
Date:   Tue, 21 Feb 2023 11:49:46 -0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Asahi Lina <lina@asahilina.net>, Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        John Stultz <jstultz@google.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-kernel@vger.kernel.org,
        rust-for-linux@vger.kernel.org, asahi@lists.linux.dev
Subject: Re: [PATCH] rust: time: New module for timekeeping functions
Message-ID: <Y/UgWqgRz2QxUT+t@boqun-archlinux>
References: <20230221-gpu-up-time-v1-1-bf8fe74b7f55@asahilina.net>
 <87v8jvnqq4.ffs@tglx>
 <Y/TP6as7qqwfcI42@boqun-archlinux>
 <6aa15295-219b-225c-607d-e87e3d51d048@asahilina.net>
 <87bklmonbv.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87bklmonbv.ffs@tglx>
Precedence: bulk

On Tue, Feb 21, 2023 at 08:00:52PM +0100, Thomas Gleixner wrote:
[...]
> > I figured we might get a better idea for what to do once a
> > second user comes along. For example, do we want straight methods like
> > that or std::ops trait impls? And do we make everything fallible or
> > panic on overflow or just wrap?
> >
> > It also means we basically have to reimplement all of
> > core::time::Duration if we want to offer an equally ergonomic API with a
> > 64-bit type (for reference, Duration is a struct with u64 secs and u32
> > nanoseconds).
> 
> As you said yourself: The kernel can't use Rust std lib. So you better
> implement sensible interfaces which are straight forward and simple to
> use in the context you are aiming for.
> 

Agreed!

Lina, my suggestion is just to go ahead and add the minimal timestamp
abstraction, surely you may make some bad decisions about APIs (e.g.
panic vs returning a Result), but kernel doesn't have a stable internal
API, so we can always fix things later.

Myself actually do something like below based on your patch, because
nothing like `now()` ;-)

I'm sure you can do better because as you said, you're the first user
;-)

Regards,
Boqun

---------->8
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 02844db47d34..3398388de0e1 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -6,16 +6,61 @@
 //! C header: [`include/linux/timekeeping.h`](../../../../include/linux/timekeeping.h)
 
 use crate::bindings;
-use core::time::Duration;
+// Re-exports [`Duration`], so that it's easy to provide kernel's own implemention in the future.
+pub use core::time::Duration;
+
+/// A timestamp
+pub trait TimeStamp: Copy {
+    /// Gets the current stamp.
+    fn now() -> Self;
+
+    /// Calculates the passed duration since `another`.
+    fn duration_since(&self, another: Self) -> Duration;
+
+
+    /// Return the duration passed since this stamp was created.
+    fn elapsed(&self) -> Duration {
+        let created = self.clone();
+        self.duration_since(created)
+    }
+}
+
+/// CLOCK_MONOTONIC timestamps.
+#[derive(Clone, Copy)]
+pub struct MonoTime(Duration);
+
+impl TimeStamp for MonoTime {
+    fn now() -> Self {
+        Self(ktime_get())
+    }
+
+    fn duration_since(&self, another: Self) -> Duration {
+        ktime_get() - another.0
+    }
+}
+
+/// CLOCK_BOOTTIME timestamps.
+#[derive(Clone, Copy)]
+pub struct BootTime(Duration);
+
+impl TimeStamp for BootTime {
+    fn now() -> Self {
+        Self(ktime_get_boottime())
+    }
+
+    fn duration_since(&self, another: Self) -> Duration {
+        ktime_get_boottime() - another.0
+    }
+}
 
 /// Returns the kernel time elapsed since boot, excluding time spent sleeping, as a [`Duration`].
-pub fn ktime_get() -> Duration {
+fn ktime_get() -> Duration {
     // SAFETY: Function has no side effects and no inputs.
     Duration::from_nanos(unsafe { bindings::ktime_get() }.try_into().unwrap())
 }
 
 /// Returns the kernel time elapsed since boot, including time spent sleeping, as a [`Duration`].
-pub fn ktime_get_boottime() -> Duration {
+fn ktime_get_boottime() -> Duration {
     Duration::from_nanos(
         // SAFETY: Function has no side effects and no variable inputs.
         unsafe { bindings::ktime_get_with_offset(bindings::tk_offsets_TK_OFFS_BOOT) }