Received-SPF: pass (google.com: domain of linux-kernel+bounces-112845-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: iad51458e:Fastmail
Date: Sun, 24 Mar 2024 13:52:46 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Valentin Obst <kernel@valentinobst.de>
Cc: aliceryhl@google.com, a.hindborg@samsung.com, alex.gaynor@gmail.com,
	benno.lossin@proton.me, bjorn3_gh@protonmail.com, gary@garyguo.net,
	heghedus.razvan@protonmail.com, jstultz@google.com,
	lina@asahilina.net, linux-kernel@vger.kernel.org, ojeda@kernel.org,
	rust-for-linux@vger.kernel.org, sboyd@kernel.org,
	tglx@linutronix.de, wedsonaf@gmail.com
Subject: Re: [PATCH v2] rust: time: add Ktime
Message-ID: <ZgCSnoSQCYiOlYNU@boqun-archlinux>
References: <Zf2kio8NYG5DEgyY@tardis>
 <20240324094023.22317-1-kernel@valentinobst.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240324094023.22317-1-kernel@valentinobst.de>

On Sun, Mar 24, 2024 at 10:40:23AM +0100, Valentin Obst wrote:
> > Subject: [PATCH] rust: time: Add clock source reading functionality
> >
> > Introduce wrappers around `ktime_t` with a time duration type `KTime`
> > and a timestamp type `Instant`.
> >
> > Rust Binder will use these bindings to compute how many milliseconds a
> > transaction has been active for when dumping the current state of the
> > Binder driver. This replicates the logic in C Binder [1].
> >
> > For a usage example in Rust Binder, see [2].
> >
> > The `ktime_get` method cannot be safely called in NMI context. This
> > requirement is not checked by these abstractions, but it is intended
> > that klint [3] or a similar tool will be used to check it in the future.
> >
> > Link: https://lore.kernel.org/lkml/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com/ [1]
> > Link: https://r.android.com/3004103 [2]
> > Link: https://rust-for-linux.com/klint [3]
> > Originally-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> > Originally-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/time.rs | 158 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 25a896eed468..50cc063aa9b4 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -4,6 +4,15 @@
> >  //!
> >  //! 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.
> > +//!
> > +//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> > +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
> > +
> > +use crate::pr_err;
> > +use core::marker::PhantomData;
> > +
> > +/// The number of nanoseconds per millisecond.
> > +pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> >
> >  /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
> >  pub type Jiffies = core::ffi::c_ulong;
> > @@ -18,3 +27,152 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
> >      // matter what the argument is.
> >      unsafe { bindings::__msecs_to_jiffies(msecs) }
> >  }
> > +
> > +/// A kernel time duration.
> > +///
> > +/// This type basically wraps the `ktime_t` with one restriction: it should only be used for
> > +/// representing a time duration, in other words, it's not the type for timestamps.
> > +#[repr(transparent)]
> > +#[derive(Debug, Copy, Clone, PartialEq, PartialOrd)]
> > +pub struct KTime {
> > +    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 }
> > +    }
> 
> Eventually we might want to be able to create instances of types that
> represent durations in const contexts, e.g., for fixed thresholds or
> fixed offsets to relative timers. Would it make sense to '/fn/const fn/'
> for the `KTime` (or `Ktime`) methods that support it?
> 

Yeah, that makes sense. Besides, I'm going to rename `KTime` as
`Duration`, since it really represents a duration of time, and...

> [For that use case the naming/signature `from_raw(inner: bindings::ktime_t)`
> could maybe also be changed to something like `new(duration: i64)`, i.e.,
> make it sound less like an internal API.]
> 

..it makes more sense for a `Duration::new()` instead of a
`KTime::from_raw()`. I will send an updated version soon, since I also
find a few problems:

> 	- Best Valentin
> 
> > +
> > +    /// 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
> > +    }
> > +
> > +    /// Returns the number of milliseconds.
> > +    #[inline]
> > +    pub fn to_ms(self) -> i64 {
> > +        self.divns_constant::<NSEC_PER_MSEC>()
> > +    }
> > +}
> > +
> > +impl core::ops::Sub for KTime {
> > +    type Output = KTime;
> > +
> > +    #[inline]
> > +    fn sub(self, other: KTime) -> KTime {
> > +        Self {
> > +            inner: self.inner - other.inner,

This doesn't handle the overflow cases.

> > +        }
> > +    }
> > +}
> > +
> > +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
> > +pub trait Clock: Sized {
> > +    /// Returns the current time for this clock.
> > +    fn now() -> Instant<Self>;
> > +}
> > +
> > +/// Marker trait for clock sources that are guaranteed to be monotonic.
> > +pub trait Monotonic {}
> > +
> > +/// An instant in time associated with a given clock source.
> > +#[derive(Debug)]
> > +pub struct Instant<T: Clock> {
> > +    ktime: KTime,
> > +    _type: PhantomData<T>,
> > +}
> > +
> > +impl<T: Clock> Clone for Instant<T> {
> > +    fn clone(&self) -> Self {
> > +        *self
> > +    }
> > +}
> > +
> > +impl<T: Clock> Copy for Instant<T> {}
> > +
> > +impl<T: Clock> Instant<T> {
> > +    fn new(ktime: KTime) -> Self {
> > +        Instant {
> > +            ktime,
> > +            _type: PhantomData,
> > +        }
> > +    }
> > +
> > +    /// Returns the time elapsed since an earlier [`Instant`], or None if the argument is a later
> > +    /// Instant.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::time::{Clock, clock::KernelTime};
> > +    ///
> > +    /// let a = KernelTime::now();
> > +    /// let b = KernelTime::now();
> > +    ///
> > +    /// // `KernelTime` is monotonic.
> > +    /// assert_eq!(a.since(b), None);
> > +    /// assert_eq!(b.since(a).map(|d| d.to_ns() >= 0), Some(true));
> > +    ///
> > +    /// ```
> > +    pub fn since(&self, earlier: Instant<T>) -> Option<KTime> {
> > +        if self.ktime < earlier.ktime {
> > +            None
> > +        } else {
> > +            Some(self.ktime - earlier.ktime)
> > +        }
> > +    }
> > +}
> > +
> > +impl<T: Clock + Monotonic> Instant<T> {
> > +    /// Returns the time elapsed since this [`Instant`].
> > +    ///
> > +    /// This is guaranteed to return a non-negative result, since it is only implemented for
> > +    /// monotonic clocks.
> > +    ///

And this is not quite right, since if we implement `Add` trait for
`Instant` (`Instant` + `Duration`, i.e. the "wrapper" for
ktime_add_safe()), we could have an `Instant` of monotonic clocks that
points to the future (i.e. now() is earlier than `self`). I will remove
the `pr_err()` below (since it's not an error anymore). But the bound of
`Monotonic` will still be remained, since it's mostly a convenient
interface for monotonic clocks.

> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::time::{Clock, clock::KernelTime};
> > +    ///
> > +    /// let a = KernelTime::now();
> > +    ///
> > +    /// // `KernelTime` is monotonic.
> > +    /// assert!(a.elapsed().to_ns() >= 0);
> > +    ///
> > +    /// ```
> > +    pub fn elapsed(&self) -> KTime {
> > +        self.since(T::now()).unwrap_or_else(|| {
> > +            pr_err!(
> > +                "Monotonic clock {} went backwards!",
> > +                core::any::type_name::<T>()
> > +            );
> > +            KTime::from_raw(0)
> > +        })
> > +    }
> > +}
> > +
> > +/// Contains the various clock source types available to the kernel.
> > +pub mod clock {
> > +    use super::*;
> > +
> > +    /// A clock representing the default kernel time source (`CLOCK_MONOTONIC`).
> > +    pub struct KernelTime;
> > +
> > +    impl Monotonic for KernelTime {}
> 
> nit: blank line missing
> 

Thanks, I will add it in a new version.

Regards,
Boqun

> > +    impl Clock for KernelTime {
> > +        #[inline]
> > +        fn now() -> Instant<Self> {
> > +            // SAFETY: It is always safe to call `ktime_get` outside of NMI context.
> > +            Instant::<Self>::new(KTime::from_raw(unsafe { bindings::ktime_get() }))
> > +        }
> > +    }
> > +}