Hi Thomas,
This is an updated version of:
https://lore.kernel.org/rust-for-linux/Zf2kio8NYG5DEgyY@tardis/
where I'm trying to provide the same functionality of Alice's patch:
https://lore.kernel.org/rust-for-linux/[email protected]/
along with the lesson we learned from the previous discussion:
https://lore.kernel.org/rust-for-linux/[email protected]/
https://lore.kernel.org/rust-for-linux/[email protected]/
There are three important types or traits (the design was mostly brought
from previous patches):
* Instant type: it's a generic type for timestamps, whose backend is
just ktime_t, but it has a generic type to differentiate clocks.
* Clock trait: to read a clock, one must implement the `now` function,
in this series, only CLOCK_MONOTONIC support is added.
* Duration type: it's a signed 64 bit value which represents the
timedelta, I've considered making it unsigned, but I want the
following code:
d = ts2 - ts1;
ts = ts3 + d;
to work even when ts2 < ts1, but I'm also OK to define it as an
unsigned.
Let me know how you think about this.
Regards,
Boqun
Alice Ryhl (1):
rust: time: Introduce Duration type
Boqun Feng (4):
rust: time: doc: Add missing C header link to jiffies
rust: time: Introduce clock reading framework
rust: time: Support reading CLOCK_MONOTONIC
rust: time: Add Instant::elapsed() for monotonic clocks
rust/kernel/time.rs | 139 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 139 insertions(+)
--
2.44.0
The definitions related to jiffies are at linux/jiffies.h, and since
`kernel::time` provides the functionality dealing with jiffies, it makes
sense to add a link to it from Rust's time module.
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 25a896eed468..bbb666e64dd7 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -4,6 +4,8 @@
//!
//! 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).
/// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
pub type Jiffies = core::ffi::c_ulong;
--
2.44.0
From: Alice Ryhl <[email protected]>
Introduce a type representing time duration. Define our own type instead
of using `core::time::Duration` because in kernel C code, an i64
(ktime_t) is used for representing time durations, an i64 backed
duration type is more efficient when interacting with time APIs in C.
Signed-off-by: Alice Ryhl <[email protected]>
[boqun: Rename `Ktime` to `Duration`, and make it a type of durations]
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index bbb666e64dd7..b238b3a4e899 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -7,6 +7,9 @@
//!
//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
+/// 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;
@@ -20,3 +23,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
// matter what the argument is.
unsafe { bindings::__msecs_to_jiffies(msecs) }
}
+
+/// A time duration.
+///
+/// # Examples
+///
+/// ```
+/// let one_second = kernel::time::Duration::new(1000_000_000);
+///
+/// // 1 second is 1000 milliseconds.
+/// assert_eq!(one_second.to_ms(), 1000);
+/// ```
+#[repr(transparent)]
+#[derive(Copy, Clone, Debug)]
+pub struct Duration {
+ inner: i64,
+}
+
+impl Duration {
+ /// Creates a new duration of `ns` nanoseconds.
+ pub const fn new(ns: i64) -> Self {
+ Self { inner: ns }
+ }
+
+ /// Divides 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 milliseconds.
+ #[inline]
+ pub fn to_ms(self) -> i64 {
+ self.divns_constant::<NSEC_PER_MSEC>()
+ }
+
+ /// Returns the number of nanoseconds.
+ #[inline]
+ pub fn to_ns(self) -> i64 {
+ self.inner
+ }
+}
--
2.44.0
To make sure Rust code doesn't mix timestamps from different clocks, a
type safe clock reading framework is introduced. It includes:
* A `Clock` trait that represents different clocks, to read a particular
clock, one needs implement the `Clock::now()` function.
* A `Instant<Clock>` type that represents timestamps of different
clocks, whose implementation is just a `ktime_t`, so all the
calculation on `ktime_t` should apply to it as well.
Co-developed-by: Heghedus Razvan <[email protected]>
Signed-off-by: Heghedus Razvan <[email protected]>
Co-developed-by: Asahi Lina <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index b238b3a4e899..0f9f5605ed48 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -6,6 +6,9 @@
//! 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 core::marker::PhantomData;
/// The number of nanoseconds per millisecond.
pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
@@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
self.inner
}
}
+
+/// 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 {}
+
+/// A timestamp of a given [`Clock`]
+#[repr(transparent)]
+#[derive(Debug)]
+pub struct Instant<T: Clock> {
+ inner: bindings::ktime_t,
+ clock: 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: bindings::ktime_t) -> Self {
+ Self {
+ inner: ktime,
+ clock: PhantomData,
+ }
+ }
+}
+
+impl<T: Clock> core::ops::Sub for Instant<T> {
+ type Output = Duration;
+
+ /// Returns the difference of two timestamps.
+ #[inline]
+ fn sub(self, other: Self) -> Self::Output {
+ // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
+ // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
+ Duration::new(self.inner.wrapping_sub(other.inner))
+ }
+}
--
2.44.0
Rust Binder will need to read CLOCK_MONOTONIC 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].
Hence add the support for CLOCK_MONOTONIC read.
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]
Co-developed-by: Heghedus Razvan <[email protected]>
Signed-off-by: Heghedus Razvan <[email protected]>
Co-developed-by: Asahi Lina <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
Co-developed-by: Alice Ryhl <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
---
@Alice, I still put the link to the usage of Android binder here, if you
want to remove that, please let me know.
rust/kernel/time.rs | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 0f9f5605ed48..5cd669cbea01 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -113,3 +113,22 @@ fn sub(self, other: Self) -> Self::Output {
Duration::new(self.inner.wrapping_sub(other.inner))
}
}
+
+/// 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;
+
+ /// `CLOCK_MONOTONIC` is monotonic.
+ impl Monotonic for KernelTime {}
+
+ 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(unsafe { bindings::ktime_get() })
+ }
+ }
+}
--
2.44.0
This is a convenient way to do:
t1 = Clock::now();
...
delta = Clock::now() - t1;
Hence add it.
Co-developed-by: Heghedus Razvan <[email protected]>
Signed-off-by: Heghedus Razvan <[email protected]>
Co-developed-by: Asahi Lina <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 5cd669cbea01..cd1e45169517 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
}
}
+impl<T: Clock + Monotonic> Instant<T> {
+ /// Returns the time elapsed since this [`Instant`].
+ ///
+ /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
+ /// Note even though the function only exists for monotonic clocks, it could still return
+ /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
+ /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
+ ///
+ /// But for typical usages, it should always return non-negative [`Duration`]:
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::time::{Clock, clock::KernelTime};
+ ///
+ /// let ts = KernelTime::now();
+ ///
+ /// // `KernelTime` is monotonic.
+ /// assert!(ts.elapsed().to_ns() >= 0);
+ /// ```
+ pub fn elapsed(&self) -> Duration {
+ T::now() - *self
+ }
+}
+
/// Contains the various clock source types available to the kernel.
pub mod clock {
use super::*;
--
2.44.0
On 24.03.24 23:33, Boqun Feng wrote:> From: Alice Ryhl <[email protected]>
>
> Introduce a type representing time duration. Define our own type instead
> of using `core::time::Duration` because in kernel C code, an i64
> (ktime_t) is used for representing time durations, an i64 backed
> duration type is more efficient when interacting with time APIs in C.
>
> Signed-off-by: Alice Ryhl <[email protected]>
> [boqun: Rename `Ktime` to `Duration`, and make it a type of durations]
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> rust/kernel/time.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index bbb666e64dd7..b238b3a4e899 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -7,6 +7,9 @@
> //!
> //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>
> +/// 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;
>
> @@ -20,3 +23,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
> // matter what the argument is.
> unsafe { bindings::__msecs_to_jiffies(msecs) }
> }
> +
> +/// A time duration.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let one_second = kernel::time::Duration::new(1000_000_000);
> +///
> +/// // 1 second is 1000 milliseconds.
> +/// assert_eq!(one_second.to_ms(), 1000);
> +/// ```
> +#[repr(transparent)]
> +#[derive(Copy, Clone, Debug)]
> +pub struct Duration {
> + inner: i64,
Why not use the name `ns` or `nanos`?
> +}
> +
> +impl Duration {
> + /// Creates a new duration of `ns` nanoseconds.
> + pub const fn new(ns: i64) -> Self {
> + Self { inner: ns }
> + }
> +
> + /// Divides the number of nanoseconds by a compile-time constant.
> + #[inline]
> + fn divns_constant<const DIV: i64>(self) -> i64 {
> + self.to_ns() / DIV
> + }
I am a bit confused, why is this better than writing
`self.to_ns() / DIV` at the callsite?
--
Cheers,
Benno
> +
> + /// Returns the number of milliseconds.
> + #[inline]
> + pub fn to_ms(self) -> i64 {
> + self.divns_constant::<NSEC_PER_MSEC>()
> + }
> +
> + /// Returns the number of nanoseconds.
> + #[inline]
> + pub fn to_ns(self) -> i64 {
> + self.inner
> + }
> +}
> --
> 2.44.0
>
On 24.03.24 23:33, Boqun Feng wrote:
> To make sure Rust code doesn't mix timestamps from different clocks, a
> type safe clock reading framework is introduced. It includes:
>
> * A `Clock` trait that represents different clocks, to read a particular
> clock, one needs implement the `Clock::now()` function.
>
> * A `Instant<Clock>` type that represents timestamps of different
> clocks, whose implementation is just a `ktime_t`, so all the
> calculation on `ktime_t` should apply to it as well.
>
> Co-developed-by: Heghedus Razvan <[email protected]>
> Signed-off-by: Heghedus Razvan <[email protected]>
> Co-developed-by: Asahi Lina <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index b238b3a4e899..0f9f5605ed48 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -6,6 +6,9 @@
> //! 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 core::marker::PhantomData;
>
> /// The number of nanoseconds per millisecond.
> pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
> self.inner
> }
> }
> +
> +/// 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 {}
Why is this trait not an extension of `Clock`?
> +
> +/// A timestamp of a given [`Clock`]
Missing '.'.
> +#[repr(transparent)]
> +#[derive(Debug)]
> +pub struct Instant<T: Clock> {
> + inner: bindings::ktime_t,
> + clock: 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: bindings::ktime_t) -> Self {
When compiling, this function is marked as dead-code in this patch.
--
Cheers,
Benno
> + Self {
> + inner: ktime,
> + clock: PhantomData,
> + }
> + }
> +}
> +
> +impl<T: Clock> core::ops::Sub for Instant<T> {
> + type Output = Duration;
> +
> + /// Returns the difference of two timestamps.
> + #[inline]
> + fn sub(self, other: Self) -> Self::Output {
> + // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
> + // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
> + Duration::new(self.inner.wrapping_sub(other.inner))
> + }
> +}
> --
> 2.44.0
>
On 24.03.24 23:33, Boqun Feng wrote:
> Rust Binder will need to read CLOCK_MONOTONIC 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].
>
> Hence add the support for CLOCK_MONOTONIC read.
>
> 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]
> Co-developed-by: Heghedus Razvan <[email protected]>
> Signed-off-by: Heghedus Razvan <[email protected]>
> Co-developed-by: Asahi Lina <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> Co-developed-by: Alice Ryhl <[email protected]>
> Signed-off-by: Alice Ryhl <[email protected]>
> Signed-off-by: Boqun Feng <[email protected]>
With the nit fixed:
Reviewed-by: Benno Lossin <[email protected]>
> ---
> @Alice, I still put the link to the usage of Android binder here, if you
> want to remove that, please let me know.
>
> rust/kernel/time.rs | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 0f9f5605ed48..5cd669cbea01 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -113,3 +113,22 @@ fn sub(self, other: Self) -> Self::Output {
> Duration::new(self.inner.wrapping_sub(other.inner))
> }
> }
> +
> +/// 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;
> +
> + /// `CLOCK_MONOTONIC` is monotonic.
> + impl Monotonic for KernelTime {}
> +
> + 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(unsafe { bindings::ktime_get() })
I don't think the turbofish syntax (the `::<Self>` part) is needed here.
--
Cheers,
Benno
> + }
> + }
> +}
> --
> 2.44.0
>
On 24.03.24 23:33, Boqun Feng wrote:> The definitions related to jiffies are at linux/jiffies.h, and since
> `kernel::time` provides the functionality dealing with jiffies, it makes
> sense to add a link to it from Rust's time module.
>
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> rust/kernel/time.rs | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Benno Lossin <[email protected]>
--
Cheers,
Benno
On Tue, Mar 26, 2024 at 04:50:02PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:> From: Alice Ryhl <[email protected]>
> >
> > Introduce a type representing time duration. Define our own type instead
> > of using `core::time::Duration` because in kernel C code, an i64
> > (ktime_t) is used for representing time durations, an i64 backed
> > duration type is more efficient when interacting with time APIs in C.
> >
> > Signed-off-by: Alice Ryhl <[email protected]>
> > [boqun: Rename `Ktime` to `Duration`, and make it a type of durations]
> > Signed-off-by: Boqun Feng <[email protected]>
> > ---
> > rust/kernel/time.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index bbb666e64dd7..b238b3a4e899 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -7,6 +7,9 @@
> > //!
> > //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> >
> > +/// 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;
> >
> > @@ -20,3 +23,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
> > // matter what the argument is.
> > unsafe { bindings::__msecs_to_jiffies(msecs) }
> > }
> > +
> > +/// A time duration.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let one_second = kernel::time::Duration::new(1000_000_000);
> > +///
> > +/// // 1 second is 1000 milliseconds.
> > +/// assert_eq!(one_second.to_ms(), 1000);
> > +/// ```
> > +#[repr(transparent)]
> > +#[derive(Copy, Clone, Debug)]
> > +pub struct Duration {
> > + inner: i64,
>
> Why not use the name `ns` or `nanos`?
>
Good point, I will rename it in next version.
> > +}
> > +
> > +impl Duration {
> > + /// Creates a new duration of `ns` nanoseconds.
> > + pub const fn new(ns: i64) -> Self {
> > + Self { inner: ns }
> > + }
> > +
> > + /// Divides the number of nanoseconds by a compile-time constant.
> > + #[inline]
> > + fn divns_constant<const DIV: i64>(self) -> i64 {
> > + self.to_ns() / DIV
> > + }
>
> I am a bit confused, why is this better than writing
> `self.to_ns() / DIV` at the callsite?
>
Hmm.. you're right, there should be no difference I think. If there is
nothing I'm missing from Alice, I will drop this function in the next
version.
Regards,
Boqun
> --
> Cheers,
> Benno
>
> > +
> > + /// Returns the number of milliseconds.
> > + #[inline]
> > + pub fn to_ms(self) -> i64 {
> > + self.divns_constant::<NSEC_PER_MSEC>()
> > + }
> > +
> > + /// Returns the number of nanoseconds.
> > + #[inline]
> > + pub fn to_ns(self) -> i64 {
> > + self.inner
> > + }
> > +}
> > --
> > 2.44.0
> >
On 24.03.24 23:33, Boqun Feng wrote:
> This is a convenient way to do:
>
> t1 = Clock::now();
> ...
> delta = Clock::now() - t1;
>
> Hence add it.
>
> Co-developed-by: Heghedus Razvan <[email protected]>
> Signed-off-by: Heghedus Razvan <[email protected]>
> Co-developed-by: Asahi Lina <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> rust/kernel/time.rs | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 5cd669cbea01..cd1e45169517 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
> }
> }
>
> +impl<T: Clock + Monotonic> Instant<T> {
> + /// Returns the time elapsed since this [`Instant`].
> + ///
> + /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
> + /// Note even though the function only exists for monotonic clocks, it could still return
> + /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
> + /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
But there currently is no way to add an `Instant<T>` to a `Duration`.
> + ///
> + /// But for typical usages, it should always return non-negative [`Duration`]:
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::time::{Clock, clock::KernelTime};
> + ///
> + /// let ts = KernelTime::now();
> + ///
> + /// // `KernelTime` is monotonic.
> + /// assert!(ts.elapsed().to_ns() >= 0);
Now that I thought a bit more about the design, I think allowing
negative durations is a bad idea.
Do you disagree?
If there is a case where you have a non-monotonic clock, or you are not
sure if two timestamps are in the correct relation, we could have a
function that returns a `Option<Duration>` or `Result<Duration>`.
--
Cheers,
Benno
> + /// ```
> + pub fn elapsed(&self) -> Duration {
> + T::now() - *self
> + }
> +}
> +
> /// Contains the various clock source types available to the kernel.
> pub mod clock {
> use super::*;
> --
> 2.44.0
>
On Tue, Mar 26, 2024 at 10:11:07AM -0700, Boqun Feng wrote:
[...]
> > > +impl Duration {
> > > + /// Creates a new duration of `ns` nanoseconds.
> > > + pub const fn new(ns: i64) -> Self {
> > > + Self { inner: ns }
> > > + }
> > > +
> > > + /// Divides the number of nanoseconds by a compile-time constant.
> > > + #[inline]
> > > + fn divns_constant<const DIV: i64>(self) -> i64 {
> > > + self.to_ns() / DIV
> > > + }
> >
> > I am a bit confused, why is this better than writing
> > `self.to_ns() / DIV` at the callsite?
> >
>
> Hmm.. you're right, there should be no difference I think. If there is
> nothing I'm missing from Alice, I will drop this function in the next
> version.
>
On a second thought, I think this prevents accidentally divide a
non-const value, in other words, if you use this function, you're
guaranteed the divisor is a constant, and you have the compiler checking
that for you. So in that sense, I think it makes sense to remain it
here. Thoughts?
Regards,
Boqun
> Regards,
> Boqun
>
> > --
> > Cheers,
> > Benno
> >
> > > +
> > > + /// Returns the number of milliseconds.
> > > + #[inline]
> > > + pub fn to_ms(self) -> i64 {
> > > + self.divns_constant::<NSEC_PER_MSEC>()
> > > + }
> > > +
> > > + /// Returns the number of nanoseconds.
> > > + #[inline]
> > > + pub fn to_ns(self) -> i64 {
> > > + self.inner
> > > + }
> > > +}
> > > --
> > > 2.44.0
> > >
On Tue, Mar 26, 2024 at 05:13:38PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:
> > This is a convenient way to do:
> >
> > t1 = Clock::now();
> > ...
> > delta = Clock::now() - t1;
> >
> > Hence add it.
> >
> > Co-developed-by: Heghedus Razvan <[email protected]>
> > Signed-off-by: Heghedus Razvan <[email protected]>
> > Co-developed-by: Asahi Lina <[email protected]>
> > Signed-off-by: Asahi Lina <[email protected]>
> > Signed-off-by: Boqun Feng <[email protected]>
> > ---
> > rust/kernel/time.rs | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 5cd669cbea01..cd1e45169517 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
> > }
> > }
> >
> > +impl<T: Clock + Monotonic> Instant<T> {
> > + /// Returns the time elapsed since this [`Instant`].
> > + ///
> > + /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
> > + /// Note even though the function only exists for monotonic clocks, it could still return
> > + /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
> > + /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
>
> But there currently is no way to add an `Instant<T>` to a `Duration`.
>
This is kinda the disadvantages of "upstreaming the bits you only need",
we know for sure there will be a way to generate an `Instant` with an
addition of a `Duration`. I can of course provide that function in this
series. But let's settle down on "negative durations" first.
> > + ///
> > + /// But for typical usages, it should always return non-negative [`Duration`]:
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// use kernel::time::{Clock, clock::KernelTime};
> > + ///
> > + /// let ts = KernelTime::now();
> > + ///
> > + /// // `KernelTime` is monotonic.
> > + /// assert!(ts.elapsed().to_ns() >= 0);
>
> Now that I thought a bit more about the design, I think allowing
> negative durations is a bad idea.
> Do you disagree?
>
So yes, I don't think allowing negative duration is really good design.
But as I mentioned in the cover letter, I hope to support cases where:
d = ts2 - ts1;
ts = ts3 + d;
(where ts1, ts2, ts3 is Instant, and d is of course Duration)
without any branch instruction in the asm code. It's useful in the case
where ts1 is a old time base, and ts3 is the new one, and you want to
"remain" the delta between ts2 and t1 and apply that on ts3. To me there
are three options to achieve that: 1) allow negative durations (this
also mirrors what `ktime_t` represents for timedelta AKAIU), 2) have
a timedelta type that differs from Duration, and it can be negative, 3)
provide a function to do the above calculation for `Instant`. I choose
the first one because it's quick and simple (also easy to map to
`ktime_t`). But I don't have my own preference on these three options.
Regards,
Boqun
> If there is a case where you have a non-monotonic clock, or you are not
> sure if two timestamps are in the correct relation, we could have a
> function that returns a `Option<Duration>` or `Result<Duration>`.
>
> --
> Cheers,
> Benno
>
> > + /// ```
> > + pub fn elapsed(&self) -> Duration {
> > + T::now() - *self
> > + }
> > +}
> > +
> > /// Contains the various clock source types available to the kernel.
> > pub mod clock {
> > use super::*;
> > --
> > 2.44.0
> >
On Tue, Mar 26, 2024 at 11:00:52AM -0700, Boqun Feng wrote:
[...]
> >
> > Now that I thought a bit more about the design, I think allowing
> > negative durations is a bad idea.
> > Do you disagree?
> >
>
> So yes, I don't think allowing negative duration is really good design.
> But as I mentioned in the cover letter, I hope to support cases where:
>
> d = ts2 - ts1;
> ts = ts3 + d;
>
> (where ts1, ts2, ts3 is Instant, and d is of course Duration)
>
> without any branch instruction in the asm code. It's useful in the case
To be accurate, the "ts = ts3 + d" can have (and should have) a branch
to check overflows (and reset to KTIME_MAX if so), but that's the only
branch.
Regards,
Boqun
> where ts1 is a old time base, and ts3 is the new one, and you want to
> "remain" the delta between ts2 and t1 and apply that on ts3. To me there
> are three options to achieve that: 1) allow negative durations (this
> also mirrors what `ktime_t` represents for timedelta AKAIU), 2) have
> a timedelta type that differs from Duration, and it can be negative, 3)
> provide a function to do the above calculation for `Instant`. I choose
> the first one because it's quick and simple (also easy to map to
> `ktime_t`). But I don't have my own preference on these three options.
>
> Regards,
> Boqun
>
> > If there is a case where you have a non-monotonic clock, or you are not
> > sure if two timestamps are in the correct relation, we could have a
> > function that returns a `Option<Duration>` or `Result<Duration>`.
> >
> > --
> > Cheers,
> > Benno
> >
> > > + /// ```
> > > + pub fn elapsed(&self) -> Duration {
> > > + T::now() - *self
> > > + }
> > > +}
> > > +
> > > /// Contains the various clock source types available to the kernel.
> > > pub mod clock {
> > > use super::*;
> > > --
> > > 2.44.0
> > >
On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:
> > To make sure Rust code doesn't mix timestamps from different clocks, a
> > type safe clock reading framework is introduced. It includes:
> >
> > * A `Clock` trait that represents different clocks, to read a particular
> > clock, one needs implement the `Clock::now()` function.
> >
> > * A `Instant<Clock>` type that represents timestamps of different
> > clocks, whose implementation is just a `ktime_t`, so all the
> > calculation on `ktime_t` should apply to it as well.
> >
> > Co-developed-by: Heghedus Razvan <[email protected]>
> > Signed-off-by: Heghedus Razvan <[email protected]>
> > Co-developed-by: Asahi Lina <[email protected]>
> > Signed-off-by: Asahi Lina <[email protected]>
> > Signed-off-by: Boqun Feng <[email protected]>
> > ---
> > rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index b238b3a4e899..0f9f5605ed48 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -6,6 +6,9 @@
> > //! 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 core::marker::PhantomData;
> >
> > /// The number of nanoseconds per millisecond.
> > pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> > @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
> > self.inner
> > }
> > }
> > +
> > +/// 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 {}
>
> Why is this trait not an extension of `Clock`?
>
This was carried over from the old version, for myself, it doesn't make
much difference between:
trait A { .. }
trait B { .. }
impl<T: A + B> ...
vs
trait A { .. }
trait B: A { .. }
impl<T: B> ...
hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
change it in the next version.
> > +
> > +/// A timestamp of a given [`Clock`]
>
> Missing '.'.
>
Fixed locally.
> > +#[repr(transparent)]
> > +#[derive(Debug)]
> > +pub struct Instant<T: Clock> {
> > + inner: bindings::ktime_t,
> > + clock: 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: bindings::ktime_t) -> Self {
>
> When compiling, this function is marked as dead-code in this patch.
>
Hmm... I cannot trigger any compile errors with this patch. If you see
an error, could you share the build command? It's not a `pub` function
though.
Regards,
Boqun
> --
> Cheers,
> Benno
>
> > + Self {
> > + inner: ktime,
> > + clock: PhantomData,
> > + }
> > + }
> > +}
> > +
> > +impl<T: Clock> core::ops::Sub for Instant<T> {
> > + type Output = Duration;
> > +
> > + /// Returns the difference of two timestamps.
> > + #[inline]
> > + fn sub(self, other: Self) -> Self::Output {
> > + // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
> > + // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
> > + Duration::new(self.inner.wrapping_sub(other.inner))
> > + }
> > +}
> > --
> > 2.44.0
> >
On Tue, Mar 26, 2024 at 05:03:41PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:
> > Rust Binder will need to read CLOCK_MONOTONIC 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].
> >
> > Hence add the support for CLOCK_MONOTONIC read.
> >
> > 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]
> > Co-developed-by: Heghedus Razvan <[email protected]>
> > Signed-off-by: Heghedus Razvan <[email protected]>
> > Co-developed-by: Asahi Lina <[email protected]>
> > Signed-off-by: Asahi Lina <[email protected]>
> > Co-developed-by: Alice Ryhl <[email protected]>
> > Signed-off-by: Alice Ryhl <[email protected]>
> > Signed-off-by: Boqun Feng <[email protected]>
>
> With the nit fixed:
>
> Reviewed-by: Benno Lossin <[email protected]>
>
Thanks!
> > ---
> > @Alice, I still put the link to the usage of Android binder here, if you
> > want to remove that, please let me know.
> >
> > rust/kernel/time.rs | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 0f9f5605ed48..5cd669cbea01 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -113,3 +113,22 @@ fn sub(self, other: Self) -> Self::Output {
> > Duration::new(self.inner.wrapping_sub(other.inner))
> > }
> > }
> > +
> > +/// 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;
> > +
> > + /// `CLOCK_MONOTONIC` is monotonic.
> > + impl Monotonic for KernelTime {}
> > +
> > + 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(unsafe { bindings::ktime_get() })
>
> I don't think the turbofish syntax (the `::<Self>` part) is needed here.
>
Fixed locally.
Regards,
Boqun
> --
> Cheers,
> Benno
>
> > + }
> > + }
> > +}
> > --
> > 2.44.0
> >
On Tue, Mar 26, 2024 at 11:00:52AM -0700, Boqun Feng wrote:
> On Tue, Mar 26, 2024 at 05:13:38PM +0000, Benno Lossin wrote:
> > On 24.03.24 23:33, Boqun Feng wrote:
> > > This is a convenient way to do:
> > >
> > > t1 = Clock::now();
> > > ...
> > > delta = Clock::now() - t1;
> > >
> > > Hence add it.
> > >
> > > Co-developed-by: Heghedus Razvan <[email protected]>
> > > Signed-off-by: Heghedus Razvan <[email protected]>
> > > Co-developed-by: Asahi Lina <[email protected]>
> > > Signed-off-by: Asahi Lina <[email protected]>
> > > Signed-off-by: Boqun Feng <[email protected]>
> > > ---
> > > rust/kernel/time.rs | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > > index 5cd669cbea01..cd1e45169517 100644
> > > --- a/rust/kernel/time.rs
> > > +++ b/rust/kernel/time.rs
> > > @@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
> > > }
> > > }
> > >
> > > +impl<T: Clock + Monotonic> Instant<T> {
> > > + /// Returns the time elapsed since this [`Instant`].
> > > + ///
> > > + /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
> > > + /// Note even though the function only exists for monotonic clocks, it could still return
> > > + /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
> > > + /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
> >
> > But there currently is no way to add an `Instant<T>` to a `Duration`.
> >
>
> This is kinda the disadvantages of "upstreaming the bits you only need",
> we know for sure there will be a way to generate an `Instant` with an
> addition of a `Duration`. I can of course provide that function in this
> series. But let's settle down on "negative durations" first.
>
Hmm... I'd like to propose a change here. After some thoughts, I think
we should have two timestamp types: `Instant` and `KTime`, where
`Instant` represents solely a reading of a clocksource, and `KTime` is
just the normal timestamp. This means the only way to get an `Instant`
is via `Clock::now()`, and you cannot get an `Instant` by `Instant` +
`Duration` (this could return a `KTime`). And a `Instant` can always
`into_ktime()` return a `KTime` which support add/sub a duration. But
again you cannot get an `Instant` from a `KTime`.
Having this setup means for the same monotonic clocksource,
`Clock::now()` is always later than any `Instant`, since any `Instant`
must be created by a previous `Clock::now()`. And this makes a lot of
sense. Moreover, I could introduce `KTime` in a later patchset, since
`Instant` and `Duration` can fulfill the current requirement. We still
need two duration types though...
Regards,
Boqun
> > > + ///
> > > + /// But for typical usages, it should always return non-negative [`Duration`]:
> > > + ///
> > > + /// # Examples
> > > + ///
> > > + /// ```
> > > + /// use kernel::time::{Clock, clock::KernelTime};
> > > + ///
> > > + /// let ts = KernelTime::now();
> > > + ///
> > > + /// // `KernelTime` is monotonic.
> > > + /// assert!(ts.elapsed().to_ns() >= 0);
> >
> > Now that I thought a bit more about the design, I think allowing
> > negative durations is a bad idea.
> > Do you disagree?
> >
>
> So yes, I don't think allowing negative duration is really good design.
> But as I mentioned in the cover letter, I hope to support cases where:
>
> d = ts2 - ts1;
> ts = ts3 + d;
>
> (where ts1, ts2, ts3 is Instant, and d is of course Duration)
>
> without any branch instruction in the asm code. It's useful in the case
> where ts1 is a old time base, and ts3 is the new one, and you want to
> "remain" the delta between ts2 and t1 and apply that on ts3. To me there
> are three options to achieve that: 1) allow negative durations (this
> also mirrors what `ktime_t` represents for timedelta AKAIU), 2) have
> a timedelta type that differs from Duration, and it can be negative, 3)
> provide a function to do the above calculation for `Instant`. I choose
> the first one because it's quick and simple (also easy to map to
> `ktime_t`). But I don't have my own preference on these three options.
>
> Regards,
> Boqun
>
> > If there is a case where you have a non-monotonic clock, or you are not
> > sure if two timestamps are in the correct relation, we could have a
> > function that returns a `Option<Duration>` or `Result<Duration>`.
> >
> > --
> > Cheers,
> > Benno
> >
> > > + /// ```
> > > + pub fn elapsed(&self) -> Duration {
> > > + T::now() - *self
> > > + }
> > > +}
> > > +
> > > /// Contains the various clock source types available to the kernel.
> > > pub mod clock {
> > > use super::*;
> > > --
> > > 2.44.0
> > >
On 26.03.24 18:17, Boqun Feng wrote:
> On Tue, Mar 26, 2024 at 10:11:07AM -0700, Boqun Feng wrote:
> [...]
>>>> +impl Duration {
>>>> + /// Creates a new duration of `ns` nanoseconds.
>>>> + pub const fn new(ns: i64) -> Self {
>>>> + Self { inner: ns }
>>>> + }
>>>> +
>>>> + /// Divides the number of nanoseconds by a compile-time constant.
>>>> + #[inline]
>>>> + fn divns_constant<const DIV: i64>(self) -> i64 {
>>>> + self.to_ns() / DIV
>>>> + }
>>>
>>> I am a bit confused, why is this better than writing
>>> `self.to_ns() / DIV` at the callsite?
>>>
>>
>> Hmm.. you're right, there should be no difference I think. If there is
>> nothing I'm missing from Alice, I will drop this function in the next
>> version.
>>
>
> On a second thought, I think this prevents accidentally divide a
> non-const value, in other words, if you use this function, you're
> guaranteed the divisor is a constant, and you have the compiler checking
> that for you. So in that sense, I think it makes sense to remain it
> here. Thoughts?
I don't see the value in that. It does not prevent me from just doing
`self.to_ns() / DIV` now. I imagine that 99% of the time users will want
to get milliseconds or microseconds and we should have methods for that
(when we have users). But for the last 1% I don't think we need this
method.
--
Cheers,
Benno
On 27.03.24 02:09, Boqun Feng wrote:
> On Tue, Mar 26, 2024 at 11:00:52AM -0700, Boqun Feng wrote:
>> On Tue, Mar 26, 2024 at 05:13:38PM +0000, Benno Lossin wrote:
>>> On 24.03.24 23:33, Boqun Feng wrote:
>>>> This is a convenient way to do:
>>>>
>>>> t1 = Clock::now();
>>>> ...
>>>> delta = Clock::now() - t1;
>>>>
>>>> Hence add it.
>>>>
>>>> Co-developed-by: Heghedus Razvan <[email protected]>
>>>> Signed-off-by: Heghedus Razvan <[email protected]>
>>>> Co-developed-by: Asahi Lina <[email protected]>
>>>> Signed-off-by: Asahi Lina <[email protected]>
>>>> Signed-off-by: Boqun Feng <[email protected]>
>>>> ---
>>>> rust/kernel/time.rs | 25 +++++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>>>> index 5cd669cbea01..cd1e45169517 100644
>>>> --- a/rust/kernel/time.rs
>>>> +++ b/rust/kernel/time.rs
>>>> @@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
>>>> }
>>>> }
>>>>
>>>> +impl<T: Clock + Monotonic> Instant<T> {
>>>> + /// Returns the time elapsed since this [`Instant`].
>>>> + ///
>>>> + /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
>>>> + /// Note even though the function only exists for monotonic clocks, it could still return
>>>> + /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
>>>> + /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
>>>
>>> But there currently is no way to add an `Instant<T>` to a `Duration`.
>>>
>>
>> This is kinda the disadvantages of "upstreaming the bits you only need",
>> we know for sure there will be a way to generate an `Instant` with an
>> addition of a `Duration`. I can of course provide that function in this
>> series. But let's settle down on "negative durations" first.
>>
>
> Hmm... I'd like to propose a change here. After some thoughts, I think
> we should have two timestamp types: `Instant` and `KTime`, where
> `Instant` represents solely a reading of a clocksource, and `KTime` is
> just the normal timestamp. This means the only way to get an `Instant`
> is via `Clock::now()`, and you cannot get an `Instant` by `Instant` +
> `Duration` (this could return a `KTime`). And a `Instant` can always
> `into_ktime()` return a `KTime` which support add/sub a duration. But
> again you cannot get an `Instant` from a `KTime`.
>
> Having this setup means for the same monotonic clocksource,
> `Clock::now()` is always later than any `Instant`, since any `Instant`
> must be created by a previous `Clock::now()`. And this makes a lot of
> sense. Moreover, I could introduce `KTime` in a later patchset, since
> `Instant` and `Duration` can fulfill the current requirement. We still
> need two duration types though...
I also wanted to suggest this. Another name for `KTime` could be
`Timestamp`. I think `Timedelta` is a good fit for a duration-like type
that allows negative values.
I don't remember exactly what binder needed the time stuff for, but
given that you cannot create negative durations with this design, I
think going for the `Duration` and `Instant` approach in this series
should be fine, right?
If then later you want to compute `Timedelta`s between `Timestamp`s we
can have another series.
>
> Regards,
> Boqun
>
>>>> + ///
>>>> + /// But for typical usages, it should always return non-negative [`Duration`]:
>>>> + ///
>>>> + /// # Examples
>>>> + ///
>>>> + /// ```
>>>> + /// use kernel::time::{Clock, clock::KernelTime};
>>>> + ///
>>>> + /// let ts = KernelTime::now();
>>>> + ///
>>>> + /// // `KernelTime` is monotonic.
>>>> + /// assert!(ts.elapsed().to_ns() >= 0);
>>>
>>> Now that I thought a bit more about the design, I think allowing
>>> negative durations is a bad idea.
>>> Do you disagree?
>>>
>>
>> So yes, I don't think allowing negative duration is really good design.
>> But as I mentioned in the cover letter, I hope to support cases where:
>>
>> d = ts2 - ts1;
>> ts = ts3 + d;
>>
>> (where ts1, ts2, ts3 is Instant, and d is of course Duration)
>>
>> without any branch instruction in the asm code. It's useful in the case
>> where ts1 is a old time base, and ts3 is the new one, and you want to
>> "remain" the delta between ts2 and t1 and apply that on ts3. To me there
>> are three options to achieve that: 1) allow negative durations (this
>> also mirrors what `ktime_t` represents for timedelta AKAIU), 2) have
>> a timedelta type that differs from Duration, and it can be negative, 3)
>> provide a function to do the above calculation for `Instant`. I choose
>> the first one because it's quick and simple (also easy to map to
>> `ktime_t`). But I don't have my own preference on these three options.
I think that if the strongest motivation for allowing negative duration
is "it is more performant, since we don't need as many branches", then
the motivation for disallowing negative durations is stronger.
I think having multiple types is the solution.
--
Cheers,
Benno
On 26.03.24 20:19, Boqun Feng wrote:
> On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
>> On 24.03.24 23:33, Boqun Feng wrote:
>>> To make sure Rust code doesn't mix timestamps from different clocks, a
>>> type safe clock reading framework is introduced. It includes:
>>>
>>> * A `Clock` trait that represents different clocks, to read a particular
>>> clock, one needs implement the `Clock::now()` function.
>>>
>>> * A `Instant<Clock>` type that represents timestamps of different
>>> clocks, whose implementation is just a `ktime_t`, so all the
>>> calculation on `ktime_t` should apply to it as well.
>>>
>>> Co-developed-by: Heghedus Razvan <[email protected]>
>>> Signed-off-by: Heghedus Razvan <[email protected]>
>>> Co-developed-by: Asahi Lina <[email protected]>
>>> Signed-off-by: Asahi Lina <[email protected]>
>>> Signed-off-by: Boqun Feng <[email protected]>
>>> ---
>>> rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 49 insertions(+)
>>>
>>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>>> index b238b3a4e899..0f9f5605ed48 100644
>>> --- a/rust/kernel/time.rs
>>> +++ b/rust/kernel/time.rs
>>> @@ -6,6 +6,9 @@
>>> //! 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 core::marker::PhantomData;
>>>
>>> /// The number of nanoseconds per millisecond.
>>> pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>>> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
>>> self.inner
>>> }
>>> }
>>> +
>>> +/// 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 {}
>>
>> Why is this trait not an extension of `Clock`?
>>
>
> This was carried over from the old version, for myself, it doesn't make
> much difference between:
>
> trait A { .. }
> trait B { .. }
>
> impl<T: A + B> ...
>
> vs
>
> trait A { .. }
> trait B: A { .. }
>
> impl<T: B> ...
>
> hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
> change it in the next version.
I think it would also make sense to rename it to `MonotonicClock`.
>
>>> +
>>> +/// A timestamp of a given [`Clock`]
>>
>> Missing '.'.
>>
>
> Fixed locally.
>
>>> +#[repr(transparent)]
>>> +#[derive(Debug)]
>>> +pub struct Instant<T: Clock> {
>>> + inner: bindings::ktime_t,
>>> + clock: 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: bindings::ktime_t) -> Self {
>>
>> When compiling, this function is marked as dead-code in this patch.
>>
>
> Hmm... I cannot trigger any compile errors with this patch. If you see
> an error, could you share the build command? It's not a `pub` function
> though.
I created a fresh config using `menuconfig` and enabled CONFIG_RUST.
Then I get this error:
error: associated function `new` is never used
--> rust/kernel/time.rs:97:8
|
96 | impl<T: Clock> Instant<T> {
| ------------------------- associated function in this implementation
97 | fn new(ktime: bindings::ktime_t) -> Self {
| ^^^
|
= note: `-D dead-code` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(dead_code)]`
error: aborting due to 1 previous error
Note that the error vanishes in the next patch, since there you do have
a user for `new`.
--
Cheers,
Benno
On Wed, Mar 27, 2024 at 12:50:50PM +0000, Benno Lossin wrote:
> On 26.03.24 20:19, Boqun Feng wrote:
> > On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
> >> On 24.03.24 23:33, Boqun Feng wrote:
> >>> To make sure Rust code doesn't mix timestamps from different clocks, a
> >>> type safe clock reading framework is introduced. It includes:
> >>>
> >>> * A `Clock` trait that represents different clocks, to read a particular
> >>> clock, one needs implement the `Clock::now()` function.
> >>>
> >>> * A `Instant<Clock>` type that represents timestamps of different
> >>> clocks, whose implementation is just a `ktime_t`, so all the
> >>> calculation on `ktime_t` should apply to it as well.
> >>>
> >>> Co-developed-by: Heghedus Razvan <[email protected]>
> >>> Signed-off-by: Heghedus Razvan <[email protected]>
> >>> Co-developed-by: Asahi Lina <[email protected]>
> >>> Signed-off-by: Asahi Lina <[email protected]>
> >>> Signed-off-by: Boqun Feng <[email protected]>
> >>> ---
> >>> rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 49 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> >>> index b238b3a4e899..0f9f5605ed48 100644
> >>> --- a/rust/kernel/time.rs
> >>> +++ b/rust/kernel/time.rs
> >>> @@ -6,6 +6,9 @@
> >>> //! 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 core::marker::PhantomData;
> >>>
> >>> /// The number of nanoseconds per millisecond.
> >>> pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> >>> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
> >>> self.inner
> >>> }
> >>> }
> >>> +
> >>> +/// 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 {}
> >>
> >> Why is this trait not an extension of `Clock`?
> >>
> >
> > This was carried over from the old version, for myself, it doesn't make
> > much difference between:
> >
> > trait A { .. }
> > trait B { .. }
> >
> > impl<T: A + B> ...
> >
> > vs
> >
> > trait A { .. }
> > trait B: A { .. }
> >
> > impl<T: B> ...
> >
> > hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
> > change it in the next version.
>
> I think it would also make sense to rename it to `MonotonicClock`.
>
> >
> >>> +
> >>> +/// A timestamp of a given [`Clock`]
> >>
> >> Missing '.'.
> >>
> >
> > Fixed locally.
> >
> >>> +#[repr(transparent)]
> >>> +#[derive(Debug)]
> >>> +pub struct Instant<T: Clock> {
> >>> + inner: bindings::ktime_t,
> >>> + clock: 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: bindings::ktime_t) -> Self {
> >>
> >> When compiling, this function is marked as dead-code in this patch.
> >>
> >
> > Hmm... I cannot trigger any compile errors with this patch. If you see
> > an error, could you share the build command? It's not a `pub` function
> > though.
>
> I created a fresh config using `menuconfig` and enabled CONFIG_RUST.
> Then I get this error:
>
> error: associated function `new` is never used
> --> rust/kernel/time.rs:97:8
> |
> 96 | impl<T: Clock> Instant<T> {
> | ------------------------- associated function in this implementation
> 97 | fn new(ktime: bindings::ktime_t) -> Self {
> | ^^^
> |
> = note: `-D dead-code` implied by `-D warnings`
> = help: to override `-D warnings` add `#[allow(dead_code)]`
>
> error: aborting due to 1 previous error
>
> Note that the error vanishes in the next patch, since there you do have
> a user for `new`.
>
Seems I didn't select CONFIG_WERROR=y, so I missed it as a warning.
and I can hit it now with that config. Will fix it in the next version.
Thanks!
Regards
> --
> Cheers,
> Benno