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
The operation never overflows (Instant ranges from 0 to
`KTIME_MAX`).
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/kernel/time.rs | 48 +++++++++++++++------------------------------
1 file changed, 16 insertions(+), 32 deletions(-)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 55a365af85a3..da54a70f8f1f 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -31,59 +31,43 @@ 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.
#[repr(transparent)]
#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
+pub struct Instant {
+ // Range from 0 to `KTIME_MAX`.
inner: bindings::ktime_t,
}
-impl Ktime {
- /// Create a `Ktime` from a raw `ktime_t`.
+impl Instant {
+ /// Create a `Instant` from a raw `ktime_t`.
#[inline]
- pub fn from_raw(inner: bindings::ktime_t) -> Self {
+ fn from_raw(inner: bindings::ktime_t) -> Self {
Self { inner }
}
/// Get the current time using `CLOCK_MONOTONIC`.
#[inline]
- pub fn ktime_get() -> Self {
+ pub fn now() -> 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
- }
-
- /// Returns the number of milliseconds.
- #[inline]
- pub fn to_ms(self) -> i64 {
- self.divns_constant::<NSEC_PER_MSEC>()
+ /// Return the amount of time elapsed since the `Instant`.
+ 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;
+ // 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,
}
}
}
--
2.43.0On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> -/// A Rust wrapper around a `ktime_t`.
> +/// A specific point in time.
> #[repr(transparent)]
> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
> -pub struct Ktime {
> +pub struct Instant {
> + // Range from 0 to `KTIME_MAX`.
On top of what Tom mentioned: is this intended as an invariant? If
yes, then please document it publicly in the `Instant` docs in a `#
Invariants` section. Otherwise, I would clarify this comment somehow,
since it seems ambiguous.
Thanks!
Cheers,
Miguel
On Thu, 16 Jan 2025 13:37:42 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> -/// A Rust wrapper around a `ktime_t`.
>> +/// A specific point in time.
>> #[repr(transparent)]
>> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
>> -pub struct Ktime {
>> +pub struct Instant {
>> + // Range from 0 to `KTIME_MAX`.
>
> On top of what Tom mentioned: is this intended as an invariant? If
> yes, then please document it publicly in the `Instant` docs in a `#
> Invariants` section. Otherwise, I would clarify this comment somehow,
> since it seems ambiguous.
As I wrote to Tom, that's the kernel's assumption. Do we need to make
it an invariant too?
Or improving the above "Range from 0 to `KTIME_MAX.`" is enough?
The kernel assumes that the range of the ktime_t type is from 0 to
KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t.
On Fri, Jan 17, 2025 at 12:31 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > As I wrote to Tom, that's the kernel's assumption. Do we need to make > it an invariant too? > > Or improving the above "Range from 0 to `KTIME_MAX.`" is enough? > > The kernel assumes that the range of the ktime_t type is from 0 to > KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t. It depends on what is best for users, i.e. if there are no use cases where this needs to be negative, then why wouldn't we have the invariant documented? Or do we want to make it completely opaque? Generally speaking, I think we should pick whatever makes the most sense for the future, since we have a chance to "do the right thing", even if the C side is a bit different (we already use a different name, anyway). Thomas et al. probably know what makes the most sense here. Cheers, Miguel
On Sat, 18 Jan 2025 13:15:42 +0100 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Fri, Jan 17, 2025 at 12:31 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> As I wrote to Tom, that's the kernel's assumption. Do we need to make >> it an invariant too? >> >> Or improving the above "Range from 0 to `KTIME_MAX.`" is enough? >> >> The kernel assumes that the range of the ktime_t type is from 0 to >> KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t. > > It depends on what is best for users, i.e. if there are no use cases > where this needs to be negative, then why wouldn't we have the > invariant documented? Or do we want to make it completely opaque? Instant object is always created via ktime_get() so it shouldn't be negative. ktime_t is opaque for users. However, we support creating a Delta object from the difference between two Instance objects: Delta = Instant1 - Instant2 It's a subtraction of two s64 types so to prevent overflow, the range of ktime_t needs to be limited. I'll add the invariant doc. I'm not sure if an invariant document is the best choice, but in any case, the above information should be documented.
On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> 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
>
> The operation never overflows (Instant ranges from 0 to
> `KTIME_MAX`).
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/kernel/time.rs | 48 +++++++++++++++------------------------------
> 1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 55a365af85a3..da54a70f8f1f 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -31,59 +31,43 @@ 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.
> #[repr(transparent)]
> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
> -pub struct Ktime {
> +pub struct Instant {
> + // Range from 0 to `KTIME_MAX`.
> inner: bindings::ktime_t,
> }
>
> -impl Ktime {
> - /// Create a `Ktime` from a raw `ktime_t`.
> +impl Instant {
> + /// Create a `Instant` from a raw `ktime_t`.
> #[inline]
> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
> + fn from_raw(inner: bindings::ktime_t) -> Self {
> Self { inner }
> }
Please keep this function public.
> /// Get the current time using `CLOCK_MONOTONIC`.
> #[inline]
> - pub fn ktime_get() -> Self {
> + pub fn now() -> 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
> - }
> -
> - /// Returns the number of milliseconds.
> - #[inline]
> - pub fn to_ms(self) -> i64 {
> - self.divns_constant::<NSEC_PER_MSEC>()
> + /// Return the amount of time elapsed since the `Instant`.
> + pub fn elapsed(&self) -> Delta {
Nit: This places the #[inline] marker before the documentation. Please
move it after to be consistent.
Alice
On Thu, 16 Jan 2025 10:32:45 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
>> -impl Ktime {
>> - /// Create a `Ktime` from a raw `ktime_t`.
>> +impl Instant {
>> + /// Create a `Instant` from a raw `ktime_t`.
>> #[inline]
>> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> + fn from_raw(inner: bindings::ktime_t) -> Self {
>> Self { inner }
>> }
>
> Please keep this function public.
Surely, your driver uses from_raw()?
>> /// Get the current time using `CLOCK_MONOTONIC`.
>> #[inline]
>> - pub fn ktime_get() -> Self {
>> + pub fn now() -> 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
>> - }
>> -
>> - /// Returns the number of milliseconds.
>> - #[inline]
>> - pub fn to_ms(self) -> i64 {
>> - self.divns_constant::<NSEC_PER_MSEC>()
>> + /// Return the amount of time elapsed since the `Instant`.
>> + pub fn elapsed(&self) -> Delta {
>
> Nit: This places the #[inline] marker before the documentation. Please
> move it after to be consistent.
Oops, I'll fix.
On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> On Thu, 16 Jan 2025 10:32:45 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>>> -impl Ktime {
>>> - /// Create a `Ktime` from a raw `ktime_t`.
>>> +impl Instant {
>>> + /// Create a `Instant` from a raw `ktime_t`.
>>> #[inline]
>>> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
>>> + fn from_raw(inner: bindings::ktime_t) -> Self {
>>> Self { inner }
>>> }
>>
>> Please keep this function public.
>
> Surely, your driver uses from_raw()?
I checked out the C version of Binder driver and it doesn't seem like
the driver needs from_raw function. The Rust version [1] also doesn't
seem to need the function. Do you have a different use case?
https://r.android.com/3004103 [1]
On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Thu, 16 Jan 2025 10:32:45 +0100
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >>> -impl Ktime {
> >>> - /// Create a `Ktime` from a raw `ktime_t`.
> >>> +impl Instant {
> >>> + /// Create a `Instant` from a raw `ktime_t`.
> >>> #[inline]
> >>> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
> >>> + fn from_raw(inner: bindings::ktime_t) -> Self {
> >>> Self { inner }
> >>> }
> >>
> >> Please keep this function public.
> >
> > Surely, your driver uses from_raw()?
>
> I checked out the C version of Binder driver and it doesn't seem like
> the driver needs from_raw function. The Rust version [1] also doesn't
> seem to need the function. Do you have a different use case?
Not for this particular function, but I've changed functions called
from_raw and similar from private to public so many times at this
point that I think it should be the default.
Alice
On Wed, 22 Jan 2025 13:51:21 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
> On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>> > On Thu, 16 Jan 2025 10:32:45 +0100
>> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >
>> >>> -impl Ktime {
>> >>> - /// Create a `Ktime` from a raw `ktime_t`.
>> >>> +impl Instant {
>> >>> + /// Create a `Instant` from a raw `ktime_t`.
>> >>> #[inline]
>> >>> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> >>> + fn from_raw(inner: bindings::ktime_t) -> Self {
>> >>> Self { inner }
>> >>> }
>> >>
>> >> Please keep this function public.
>> >
>> > Surely, your driver uses from_raw()?
>>
>> I checked out the C version of Binder driver and it doesn't seem like
>> the driver needs from_raw function. The Rust version [1] also doesn't
>> seem to need the function. Do you have a different use case?
>
> Not for this particular function, but I've changed functions called
> from_raw and similar from private to public so many times at this
> point that I think it should be the default.
Then can we remove from_raw()?
We don't use Instant to represent both a specific point in time and a
span of time (we add Delta) so a device driver don't need to create an
Instant from an arbitrary value, I think.
If we allow a device driver to create Instant via from_raw(), we need
to validate a value from the driver. If we create ktime_t only via
ktime_get(), we don't need the details of ktime like a valid range of
ktime_t.
On Wed, 22 Jan 2025 22:46:35 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> On Wed, 22 Jan 2025 13:51:21 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >>
> >> > On Thu, 16 Jan 2025 10:32:45 +0100
> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >
> >> >>> -impl Ktime {
> >> >>> - /// Create a `Ktime` from a raw `ktime_t`.
> >> >>> +impl Instant {
> >> >>> + /// Create a `Instant` from a raw `ktime_t`.
> >> >>> #[inline]
> >> >>> - pub fn from_raw(inner: bindings::ktime_t) -> Self {
> >> >>> + fn from_raw(inner: bindings::ktime_t) -> Self {
> >> >>> Self { inner }
> >> >>> }
> >> >>
> >> >> Please keep this function public.
> >> >
> >> > Surely, your driver uses from_raw()?
> >>
> >> I checked out the C version of Binder driver and it doesn't seem like
> >> the driver needs from_raw function. The Rust version [1] also doesn't
> >> seem to need the function. Do you have a different use case?
> >
> > Not for this particular function, but I've changed functions called
> > from_raw and similar from private to public so many times at this
> > point that I think it should be the default.
>
> Then can we remove from_raw()?
>
> We don't use Instant to represent both a specific point in time and a
> span of time (we add Delta) so a device driver don't need to create an
> Instant from an arbitrary value, I think.
>
> If we allow a device driver to create Instant via from_raw(), we need
> to validate a value from the driver. If we create ktime_t only via
> ktime_get(), we don't need the details of ktime like a valid range of
> ktime_t.
Yeah, I agree on this. If we specify the range 0..KTIME_MAX as
invariant then this either has to check or has to be unsafe. I don't
think that's worth adding unless there's a concrete need for it.
Best,
Gary
© 2016 - 2025 Red Hat, Inc.