[PATCH v8 2/7] rust: time: Introduce Delta type

FUJITA Tomonori posted 7 patches 11 months ago
There is a newer version of this series
[PATCH v8 2/7] rust: time: Introduce Delta type
Posted by FUJITA Tomonori 11 months ago
Introduce a type representing a span of time. Define our own type
because `core::time::Duration` is large and could panic during
creation.

time::Ktime could be also used for time duration but timestamp and
timedelta are different so better to use a new type.

i64 is used instead of u64 to represent a span of time; some C drivers
uses negative Deltas and i64 is more compatible with Ktime using i64
too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
object without type conversion.

i64 is used instead of bindings::ktime_t because when the ktime_t
type is used as timestamp, it represents values from 0 to
KTIME_MAX, which different from Delta.

Delta::from_[millis|secs] APIs take i64. When a span of time
overflows, i64::MAX is used.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 63 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 48b71e6641ce..55a365af85a3 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -8,9 +8,15 @@
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+/// The number of nanoseconds per microsecond.
+pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
+/// The number of nanoseconds per second.
+pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
+
 /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
 pub type Jiffies = crate::ffi::c_ulong;
 
@@ -81,3 +87,60 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of microseconds.
+    #[inline]
+    pub const fn from_micros(micros: i64) -> Self {
+        Self {
+            nanos: micros.saturating_mul(NSEC_PER_USEC),
+        }
+    }
+
+    /// Create a new `Delta` from a number of milliseconds.
+    #[inline]
+    pub const fn from_millis(millis: i64) -> Self {
+        Self {
+            nanos: millis.saturating_mul(NSEC_PER_MSEC),
+        }
+    }
+
+    /// Create a new `Delta` from a number of seconds.
+    #[inline]
+    pub const fn from_secs(secs: i64) -> Self {
+        Self {
+            nanos: secs.saturating_mul(NSEC_PER_SEC),
+        }
+    }
+
+    /// Return `true` if the `Detla` spans no time.
+    #[inline]
+    pub fn is_zero(self) -> bool {
+        self.as_nanos() == 0
+    }
+
+    /// Return `true` if the `Detla` spans a negative amount of time.
+    #[inline]
+    pub fn is_negative(self) -> bool {
+        self.as_nanos() < 0
+    }
+
+    /// Return the number of nanoseconds in the `Delta`.
+    #[inline]
+    pub fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the smallest number of microseconds greater than or equal
+    /// to the value in the `Delta`.
+    #[inline]
+    pub fn as_micros_ceil(self) -> i64 {
+        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+    }
+}
-- 
2.43.0
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by Miguel Ojeda 11 months ago
Hi Tomonori,

Since you started getting Reviewed-bys, I don't want to delay this
more (pun unintended :), but a couple quick notes...

I can create "good first issues" for these in our tracker if you
prefer, since these should be easy and can be done later (even if, in
general, I think we should require examples and good docs for new
abstractions).

On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> i64 is used instead of bindings::ktime_t because when the ktime_t
> type is used as timestamp, it represents values from 0 to
> KTIME_MAX, which different from Delta.

Typo: "is different ..."?

> Delta::from_[millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.

This behavior should be part of the docs of the methods below.

> +/// A span of time.
> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> +pub struct Delta {
> +    nanos: i64,
> +}

Some more docs here in `Delta` would be good, e.g. some questions
readers may have could be: What range of values can it hold? Can they
be negative?

Also some module-level docs would be nice relating all the types (you
mention some of that in the commit message for `Instant`, but it would
be nice to put it as docs, rather than in the commit message).

> +    /// Create a new `Delta` from a number of microseconds.
> +    #[inline]
> +    pub const fn from_micros(micros: i64) -> Self {
> +        Self {
> +            nanos: micros.saturating_mul(NSEC_PER_USEC),
> +        }
> +    }

For each of these, I would mention that they saturate and I would
mention the range of input values that would be kept as-is without
loss.

And it would be nice to add some examples, which you can take the
chance to show how it saturates, and they would double as tests, too.

Thanks!

Cheers,
Miguel
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by FUJITA Tomonori 10 months, 3 weeks ago
On Sat, 18 Jan 2025 13:19:21 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Since you started getting Reviewed-bys, I don't want to delay this
> more (pun unintended :), but a couple quick notes...
> 
> I can create "good first issues" for these in our tracker if you
> prefer, since these should be easy and can be done later (even if, in
> general, I think we should require examples and good docs for new
> abstractions).

Yes, please create such. I'll add more docs but I'm sure that there
will be room for improvement.

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> i64 is used instead of bindings::ktime_t because when the ktime_t
>> type is used as timestamp, it represents values from 0 to
>> KTIME_MAX, which different from Delta.
> 
> Typo: "is different ..."?

Oops, will fix.

>> Delta::from_[millis|secs] APIs take i64. When a span of time
>> overflows, i64::MAX is used.
>
> This behavior should be part of the docs of the methods below.

You want to add the above explanation to all the
Delta::from_[millis|micro|secs], right?

>> +/// A span of time.
>> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> +pub struct Delta {
>> +    nanos: i64,
>> +}
> 
> Some more docs here in `Delta` would be good, e.g. some questions
> readers may have could be: What range of values can it hold? Can they
> be negative?

Okay, I'll add.

> Also some module-level docs would be nice relating all the types (you
> mention some of that in the commit message for `Instant`, but it would
> be nice to put it as docs, rather than in the commit message).

Is there any existing source code I can refer to? I'm not sure
how "module-level docs" should be written.

>> +    /// Create a new `Delta` from a number of microseconds.
>> +    #[inline]
>> +    pub const fn from_micros(micros: i64) -> Self {
>> +        Self {
>> +            nanos: micros.saturating_mul(NSEC_PER_USEC),
>> +        }
>> +    }
> 
> For each of these, I would mention that they saturate and I would
> mention the range of input values that would be kept as-is without
> loss.
> 
> And it would be nice to add some examples, which you can take the
> chance to show how it saturates, and they would double as tests, too.

I'll try to improve.
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by Miguel Ojeda 10 months, 3 weeks ago
On Wed, Jan 22, 2025 at 8:37 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Yes, please create such. I'll add more docs but I'm sure that there
> will be room for improvement.

Sounds good, will do!

> You want to add the above explanation to all the
> Delta::from_[millis|micro|secs], right?

Yeah, I meant to add the saturating note to each of them.

> Is there any existing source code I can refer to? I'm not sure
> how "module-level docs" should be written.

You can see e.g.

    rust/kernel/block/mq.rs
    rust/kernel/init.rs
    rust/kernel/workqueue.rs

(grep `^//!` for others).

In general, you can use the module-level docs to talk about how things
relate between items of that module. For instance, when I saw in your
commit message this note:

    Implement the subtraction operator for Instant:

    Delta = Instant A - Instant B

I thought something like: "It would be nice to explain how `Delta` and
`Instant` fit together / how they are related / how all fits together
in the `time` module".

> I'll try to improve.

Thanks a lot! (really -- I personally appreciate good docs)

Cheers,
Miguel
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by Miguel Ojeda 11 months ago
On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +    /// Return `true` if the `Detla` spans no time.

Typo: `Delta` (here and another one).

By the way, please try to use intra-doc links (i.e. unless they don't
work for some reason).

Thanks!

Cheers,
Miguel
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by FUJITA Tomonori 11 months ago
On Thu, 16 Jan 2025 13:43:36 +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:
>>
>> +    /// Return `true` if the `Detla` spans no time.
> 
> Typo: `Delta` (here and another one).

Oops, sorry. I'll fix.

> By the way, please try to use intra-doc links (i.e. unless they don't
> work for some reason).

Surely, I'll try.

Thanks,
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by Alice Ryhl 11 months ago
On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
>
> time::Ktime could be also used for time duration but timestamp and
> timedelta are different so better to use a new type.
>
> i64 is used instead of u64 to represent a span of time; some C drivers
> uses negative Deltas and i64 is more compatible with Ktime using i64
> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
> object without type conversion.
>
> i64 is used instead of bindings::ktime_t because when the ktime_t
> type is used as timestamp, it represents values from 0 to
> KTIME_MAX, which different from Delta.
>
> Delta::from_[millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

One nit below, otherwise LGTM

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +    /// Return the number of nanoseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_nanos(self) -> i64 {
> +        self.nanos
> +    }

I added the ktime_ms_delta() function because I was going to use it.
Can you add an `as_millis()` function too? That way I can use
start_time.elapsed().as_millis() for my use-case.

Alice
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by FUJITA Tomonori 11 months ago
On Thu, 16 Jan 2025 10:36:07 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Introduce a type representing a span of time. Define our own type
>> because `core::time::Duration` is large and could panic during
>> creation.
>>
>> time::Ktime could be also used for time duration but timestamp and
>> timedelta are different so better to use a new type.
>>
>> i64 is used instead of u64 to represent a span of time; some C drivers
>> uses negative Deltas and i64 is more compatible with Ktime using i64
>> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
>> object without type conversion.
>>
>> i64 is used instead of bindings::ktime_t because when the ktime_t
>> type is used as timestamp, it represents values from 0 to
>> KTIME_MAX, which different from Delta.
>>
>> Delta::from_[millis|secs] APIs take i64. When a span of time
>> overflows, i64::MAX is used.
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> One nit below, otherwise LGTM
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Thanks!

>> +    /// Return the number of nanoseconds in the `Delta`.
>> +    #[inline]
>> +    pub fn as_nanos(self) -> i64 {
>> +        self.nanos
>> +    }
> 
> I added the ktime_ms_delta() function because I was going to use it.
> Can you add an `as_millis()` function too? That way I can use
> start_time.elapsed().as_millis() for my use-case.

Surely, I'll in the next version.

I dropped as_millis() method in v4 because I followed the rule, don't
add an API that may not be used.
Re: [PATCH v8 2/7] rust: time: Introduce Delta type
Posted by Miguel Ojeda 11 months ago
On Thu, Jan 16, 2025 at 1:00 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I dropped as_millis() method in v4 because I followed the rule, don't
> add an API that may not be used.

Yeah, please mention the intended/expected use case in the commit
message so that it is clear, and then it should be OK (at least for
something simple like `as_millis()` here).

Thanks!

Cheers,
Miguel