2024-03-20 10:09:12

by Alice Ryhl

[permalink] [raw]
Subject: [PATCH] rust: time: add Ktime

Introduce a wrapper around `ktime_t` with a few different useful
methods.

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, see [2].

Link: https://lore.kernel.org/lkml/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com/ [1]
Link: https://r.android.com/3004103
Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/time.rs | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 25a896eed468..ac8b35f662af 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -5,6 +5,9 @@
//! 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.

+/// 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 +21,60 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
// matter what the argument is.
unsafe { bindings::__msecs_to_jiffies(msecs) }
}
+
+/// A Rust wrapper around a `ktime_t`.
+#[repr(transparent)]
+#[derive(Copy, Clone)]
+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 }
+ }
+
+ /// Get the current time using `CLOCK_MONOTONIC`.
+ #[inline]
+ pub fn ktime_get() -> Self {
+ // SAFETY: It is always safe to call `ktime_get`.
+ 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
+ }
+
+ /// Returns the number of milliseconds.
+ #[inline]
+ pub fn to_ms(self) -> i64 {
+ self.divns_constant::<NSEC_PER_MSEC>()
+ }
+}
+
+/// 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;
+
+ #[inline]
+ fn sub(self, other: Ktime) -> Ktime {
+ Self {
+ inner: self.inner - other.inner,
+ }
+ }
+}

---
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
change-id: 20240320-rust-ktime_ms_delta-74b00c9ab872

Best regards,
--
Alice Ryhl <[email protected]>



2024-03-20 13:19:04

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] rust: time: add Ktime

On Wed, Mar 20 2024 at 10:08, Alice Ryhl wrote:
> +impl Ktime {
> + /// Create a `Ktime` from a raw `ktime_t`.
> + #[inline]
> + pub fn from_raw(inner: bindings::ktime_t) -> Self {
> + Self { inner }
> + }
> +
> + /// Get the current time using `CLOCK_MONOTONIC`.
> + #[inline]
> + pub fn ktime_get() -> Self {
> + // SAFETY: It is always safe to call `ktime_get`.

That's not entirely correct. ktime_get() cannot be safely invoked from
NMI context. That won't matter for driver writers obviously.

Looks sensible otherwise.

Thanks,

tglx

2024-03-20 15:30:24

by Boqun Feng

[permalink] [raw]
Subject: Re: [PATCH] rust: time: add Ktime

Hi,

On Wed, Mar 20, 2024 at 10:08:45AM +0000, Alice Ryhl wrote:
> Introduce a wrapper around `ktime_t` with a few different useful
> methods.
>
> 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].
>

I wonder whether Lina's previous patch also works for your case?

https://lore.kernel.org/rust-for-linux/[email protected]/

You don't need to implement all the `Clock`s since you only need
MONOTONIC time. But maybe `Duration::as_nanos` and `Duration::as_millis`
return `u128` is problematic?

Regards,
Boqun

> For a usage example, see [2].
>
> Link: https://lore.kernel.org/lkml/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com/ [1]
> Link: https://r.android.com/3004103
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/time.rs | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 25a896eed468..ac8b35f662af 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -5,6 +5,9 @@
> //! 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.
>
> +/// 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 +21,60 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
> // matter what the argument is.
> unsafe { bindings::__msecs_to_jiffies(msecs) }
> }
> +
> +/// A Rust wrapper around a `ktime_t`.
> +#[repr(transparent)]
> +#[derive(Copy, Clone)]
> +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 }
> + }
> +
> + /// Get the current time using `CLOCK_MONOTONIC`.
> + #[inline]
> + pub fn ktime_get() -> Self {
> + // SAFETY: It is always safe to call `ktime_get`.
> + 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
> + }
> +
> + /// Returns the number of milliseconds.
> + #[inline]
> + pub fn to_ms(self) -> i64 {
> + self.divns_constant::<NSEC_PER_MSEC>()
> + }
> +}
> +
> +/// 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;
> +
> + #[inline]
> + fn sub(self, other: Ktime) -> Ktime {
> + Self {
> + inner: self.inner - other.inner,
> + }
> + }
> +}
>
> ---
> base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
> change-id: 20240320-rust-ktime_ms_delta-74b00c9ab872
>
> Best regards,
> --
> Alice Ryhl <[email protected]>
>

2024-03-22 07:50:57

by Alice Ryhl

[permalink] [raw]
Subject: Re: [PATCH] rust: time: add Ktime

On Wed, Mar 20, 2024 at 4:30 PM Boqun Feng <[email protected]> wrote:
>
> Hi,
>
> On Wed, Mar 20, 2024 at 10:08:45AM +0000, Alice Ryhl wrote:
> > Introduce a wrapper around `ktime_t` with a few different useful
> > methods.
> >
> > 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].
> >
>
> I wonder whether Lina's previous patch also works for your case?
>
> https://lore.kernel.org/rust-for-linux/[email protected]/
>
> You don't need to implement all the `Clock`s since you only need
> MONOTONIC time. But maybe `Duration::as_nanos` and `Duration::as_millis`
> return `u128` is problematic?

It would work for me too, but adds more than what I need. As for
`u128`, I don't really know what the status on that is. I need to be
able to print it as a base-10 integer.

Adding an API with separate types that distinguish between Instant and
Duration and different clock sources so that you don't mix them up is
reasonable, but a bit overkill for my needs. I am spending enough API
design cycles on my uaccess, file, and linked list patchsets. Someone
else can submit patches to add a more well-typed time API in the
future.

Alice

2024-03-22 07:51:55

by Alice Ryhl

[permalink] [raw]
Subject: Re: [PATCH] rust: time: add Ktime

On Wed, Mar 20, 2024 at 2:18 PM Thomas Gleixner <[email protected]> wrote:
>
> On Wed, Mar 20 2024 at 10:08, Alice Ryhl wrote:
> > +impl Ktime {
> > + /// Create a `Ktime` from a raw `ktime_t`.
> > + #[inline]
> > + pub fn from_raw(inner: bindings::ktime_t) -> Self {
> > + Self { inner }
> > + }
> > +
> > + /// Get the current time using `CLOCK_MONOTONIC`.
> > + #[inline]
> > + pub fn ktime_get() -> Self {
> > + // SAFETY: It is always safe to call `ktime_get`.
>
> That's not entirely correct. ktime_get() cannot be safely invoked from
> NMI context. That won't matter for driver writers obviously.
>
> Looks sensible otherwise.

Thanks for pointing that out. I will send a v2 with a reworded comment soon.

Alice

2024-03-22 15:25:58

by Boqun Feng

[permalink] [raw]
Subject: Re: [PATCH] rust: time: add Ktime

On Fri, Mar 22, 2024 at 08:50:18AM +0100, Alice Ryhl wrote:
> On Wed, Mar 20, 2024 at 4:30 PM Boqun Feng <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 20, 2024 at 10:08:45AM +0000, Alice Ryhl wrote:
> > > Introduce a wrapper around `ktime_t` with a few different useful
> > > methods.
> > >
> > > 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].
> > >
> >
> > I wonder whether Lina's previous patch also works for your case?
> >
> > https://lore.kernel.org/rust-for-linux/[email protected]/
> >
> > You don't need to implement all the `Clock`s since you only need
> > MONOTONIC time. But maybe `Duration::as_nanos` and `Duration::as_millis`
> > return `u128` is problematic?
>
> It would work for me too, but adds more than what I need. As for
> `u128`, I don't really know what the status on that is. I need to be
> able to print it as a base-10 integer.
>
> Adding an API with separate types that distinguish between Instant and
> Duration and different clock sources so that you don't mix them up is
> reasonable, but a bit overkill for my needs. I am spending enough API
> design cycles on my uaccess, file, and linked list patchsets. Someone

I totally understand that! But the requirement was brought up a while
ago by Thomas:

https://lore.kernel.org/rust-for-linux/87h6vfnh0f.ffs@tglx/

> else can submit patches to add a more well-typed time API in the
> future.
>

Would it work, if I create a new patch based on yours? The work for me
won't be too much since you, Lina and Heghedus already did the hard work
along with some feedback from Thomas.

Regards,
Boqun

> Alice