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.

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

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 4a7c6037c256..38a70dc98083 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 = core::ffi::c_ulong;
 
@@ -81,3 +87,71 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of nanoseconds.
+    #[inline]
+    pub fn from_nanos(nanos: i64) -> Self {
+        Self { nanos }
+    }
+
+    /// Create a new `Delta` from a number of microseconds.
+    #[inline]
+    pub 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 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 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.nanos == 0
+    }
+
+    /// Return the number of nanoseconds in the `Delta`.
+    #[inline]
+    pub fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the number of microseconds in the `Delta`.
+    #[inline]
+    pub fn as_micros(self) -> i64 {
+        self.nanos / NSEC_PER_USEC
+    }
+
+    /// Return the number of milliseconds in the `Delta`.
+    #[inline]
+    pub fn as_millis(self) -> i64 {
+        self.nanos / NSEC_PER_MSEC
+    }
+
+    /// Return the number of seconds in the `Delta`.
+    #[inline]
+    pub fn as_secs(self) -> i64 {
+        self.nanos / NSEC_PER_SEC
+    }
+}
-- 
2.43.0

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

Wasn't there a comment that the code should always round up? Delaying
for 0 uS is probably not what the user wants.

    Andrew

On Fri, Oct 18, 2024 at 3:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Wasn't there a comment that the code should always round up? Delaying
> for 0 uS is probably not what the user wants.

This is not delaying, though, so I don't see why a method with this
name should return a rounded up value.

Moreover, `ktime_to_us` doesn't round up. The standard library one
doesn't, either.

Cheers,
Miguel

On Fri, Oct 18, 2024 at 04:31:34PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 18, 2024 at 3:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Wasn't there a comment that the code should always round up? Delaying
> > for 0 uS is probably not what the user wants.
> 
> This is not delaying, though, so I don't see why a method with this
> name should return a rounded up value.
> 
> Moreover, `ktime_to_us` doesn't round up. The standard library one
> doesn't, either.

Did you see my other comment, about not actually using these helpers?
I _guess_ it was not used because it does not in fact round up. So at
least for this patchset, the helpers are useless. Should we be adding
useless helpers? Or should we be adding useful helpers? Maybe these
helpers need a different name, and do actually round up?

	Andrew

On Fri, Oct 18, 2024 at 6:55 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Did you see my other comment, about not actually using these helpers?
> I _guess_ it was not used because it does not in fact round up. So at
> least for this patchset, the helpers are useless. Should we be adding
> useless helpers? Or should we be adding useful helpers? Maybe these
> helpers need a different name, and do actually round up?

Yeah, I saw that -- if I understand you correctly, you were asking why
`as_nanos()` is used and not `as_secs()` directly (did you mean
`as_micros()`?) by adding rounding on `Delta`'s `as_*()` methods.

So my point here was that a method with a name like `as_*()` has
nothing to do with rounding, so I wouldn't add rounding here (it would
be misleading).

Now, we can of course have rounding methods in `Delta` for convenience
if that is something commonly needed by `Delta`'s consumers like
`fsleep()`, that is fine, but those would need to be called
differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
(following e.g. `to_radians`) and + `_ceil` to follow `div_ceil` from
the `int_roundings` feature (and shorter than something like
`_rounded_up`).

In other words, I think you see these small `as_*()` functions as
"helpers" to do something else, and thus why you say we should provide
those that we need (only) and make them do what we need (the
rounding). That is fair.

However, another way of viewing these is as typical conversion methods
of `Delta`, i.e. the very basic interface for a type like this. Thus
Tomonori implemented the very basic interface, and since there was no
rounding, then he added it in `fsleep()`.

I agree having rounding methods here could be useful, but I am
ambivalent as to whether following the "no unused code" rule that far
as to remove very basic APIs for a type like this.

Cheers,
Miguel

On Sat, Oct 19, 2024 at 02:21:45PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 18, 2024 at 6:55 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Did you see my other comment, about not actually using these helpers?
> > I _guess_ it was not used because it does not in fact round up. So at
> > least for this patchset, the helpers are useless. Should we be adding
> > useless helpers? Or should we be adding useful helpers? Maybe these
> > helpers need a different name, and do actually round up?
> 
> Yeah, I saw that -- if I understand you correctly, you were asking why
> `as_nanos()` is used and not `as_secs()` directly (did you mean
> `as_micros()`?) by adding rounding on `Delta`'s `as_*()` methods.
> 
> So my point here was that a method with a name like `as_*()` has
> nothing to do with rounding, so I wouldn't add rounding here (it would
> be misleading).
> 
> Now, we can of course have rounding methods in `Delta` for convenience
> if that is something commonly needed by `Delta`'s consumers like
> `fsleep()`, that is fine, but those would need to be called
> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
> (following e.g. `to_radians`) and + `_ceil` to follow `div_ceil` from
> the `int_roundings` feature (and shorter than something like
> `_rounded_up`).
> 
> In other words, I think you see these small `as_*()` functions as
> "helpers" to do something else, and thus why you say we should provide
> those that we need (only) and make them do what we need (the
> rounding). That is fair.
> 
> However, another way of viewing these is as typical conversion methods
> of `Delta`, i.e. the very basic interface for a type like this. Thus
> Tomonori implemented the very basic interface, and since there was no
> rounding, then he added it in `fsleep()`.
> 
> I agree having rounding methods here could be useful, but I am
> ambivalent as to whether following the "no unused code" rule that far
> as to remove very basic APIs for a type like this.

I would say ignoring the rule about an API always having a user has
led to badly designed code, which is actually going to lead to bugs.

It is clearly a philosophical point, what the base of the type is, and
what are helpers. For me, the base is a i64 representing nano seconds,
operations too add, subtract and compare, and a way to get the number
of nanoseconds in and out.

I see being able to create such a type from microseconds, millisecond,
seconds and decades as helpers on top of this base. Also, being able
to extract the number of nanoseconds from the type but expressed in
microseconds, milliseconds, seconds and months are lossy helpers on
top of the base.

So far, we only have one use case for this type, holding a duration to
be passed to fsleep(). Rounding down what you pass to fsleep() is
generally not what the user wants to do, and we should try to design
the code to avoid this. The current helpers actually encourage such
bugs, because they round down. Because of this they are currently
unused. But they are a trap waiting for somebody to fall into. What
the current users of this type really want is lossy helpers which
round up. And by reviewing the one user against the API, it is clear
the current API is wrong.

So i say, throw away the round down helpers until somebody really
needs them. That avoids a class of bugs, passing a too low value to
sleep. Add the one helper which is actually needed right now.

There is potentially a better option. Make the actual sleep operation
part of the type. All the rounding up then becomes part of the core,
and the developer gets core code which just works correctly, with an
API which is hard to make do the wrong thing.

	Andrew

On Sat, Oct 19, 2024 at 8:41 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> So far, we only have one use case for this type, holding a duration to
> be passed to fsleep(). Rounding down what you pass to fsleep() is
> generally not what the user wants to do, and we should try to design
> the code to avoid this. The current helpers actually encourage such
> bugs, because they round down. Because of this they are currently
> unused. But they are a trap waiting for somebody to fall into. What
> the current users of this type really want is lossy helpers which
> round up. And by reviewing the one user against the API, it is clear
> the current API is wrong.

If you are talking about Rust's `fsleep()`, then "users" are not the
ones calling the rounding up operation (the implementation of
`fsleep()` is -- just like in the C side).

If you are talking about C's `fsleep()`, then users are not supposed
to use `bindings::`.

> So i say, throw away the round down helpers until somebody really
> needs them. That avoids a class of bugs, passing a too low value to
> sleep. Add the one helper which is actually needed right now.

Eventually this type will likely get other methods for one reason or
another, including the non-rounding ones. Thus we should be careful
about the names we pick, which is why I was saying a method like
`as_micros()` should not be rounding up. That would be confusing, and
thus potentially end creating bugs, even if it is the only method you
add today.

Again, if you want to throw away all the unused methods and only have
the rounding up one, then that is reasonable, but please let's not add
misleading methods that could add more bugs than the ones you are
trying to avoid. Please use `as_micros_ceil()` or similar.

> There is potentially a better option. Make the actual sleep operation
> part of the type. All the rounding up then becomes part of the core,
> and the developer gets core code which just works correctly, with an
> API which is hard to make do the wrong thing.

That is orthogonal: whether the sleeping function is in `Delta` or
not, users would not be the ones calling the rounding up operation
(please see above).

Anyway, by moving more things into a type, you are increasing its
complexity. And, depending how modules are organized, you could be
also giving visibility into the internals of that type, thus
increasing the possibility of "implementation-side bugs" compared to
the other way around (it does not really matter here, though).

If you want it as a method for user ergonomics though, that is a
different topic.

Cheers,
Miguel

On Sun, 20 Oct 2024 15:05:36 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Again, if you want to throw away all the unused methods and only have
> the rounding up one, then that is reasonable, but please let's not add
> misleading methods that could add more bugs than the ones you are
> trying to avoid. Please use `as_micros_ceil()` or similar.

as_micros_ceil() looks good. I'll add it in the next version.

I'll drop the unused methods in the next version. I think that adding
a type with the unused, very basic interface isn't bad though; it
could give more info about the type.

On Sat, Oct 19, 2024 at 2:21 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"

Well, it is sufficiently free, I guess, considering other methods. I
noticed `as_*()` in this patch take `self` rather than `&self`, though.

Cheers,
Miguel

On Sat, 19 Oct 2024 14:41:07 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Oct 19, 2024 at 2:21 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
> 
> Well, it is sufficiently free, I guess, considering other methods. I
> noticed `as_*()` in this patch take `self` rather than `&self`, though.

Should use &self for as_*() instead?

On Wed, Oct 23, 2024 at 1:53 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Should use &self for as_*() instead?

I don't think there is a hard rule, so taking `self` is fine even if uncommon.

But probably we should discuss eventually if we want more concrete
guidelines here (i.e. more concrete than Rust usual ones).

Thanks!

Cheers,
Miguel

On 16 Oct 2024, at 5:52, FUJITA Tomonori 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.
>
> Delta::from_[micro|millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 4a7c6037c256..38a70dc98083 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 = core::ffi::c_ulong;
>
> @@ -81,3 +87,71 @@ fn sub(self, other: Ktime) -> Ktime {
>          }
>      }
>  }
> +
> +/// A span of time.
> +#[derive(Copy, Clone)]

Could we also derive PartialEq and Eq (maybe also PartialOrd and Ord)? Would need that to compare deltas in my LED driver.

> +pub struct Delta {
> +    nanos: i64,
> +}
> +

I think all this functions could be const (need from_millis as const for LED, but when at it we could probably make all those const?)

 - Fiona

> +impl Delta {
> +    /// Create a new `Delta` from a number of nanoseconds.
> +    #[inline]
> +    pub fn from_nanos(nanos: i64) -> Self {
> +        Self { nanos }
> +    }
> +
> +    /// Create a new `Delta` from a number of microseconds.
> +    #[inline]
> +    pub 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 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 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.nanos == 0
> +    }
> +
> +    /// Return the number of nanoseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_nanos(self) -> i64 {
> +        self.nanos
> +    }
> +
> +    /// Return the number of microseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_micros(self) -> i64 {
> +        self.nanos / NSEC_PER_USEC
> +    }
> +
> +    /// Return the number of milliseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_millis(self) -> i64 {
> +        self.nanos / NSEC_PER_MSEC
> +    }
> +
> +    /// Return the number of seconds in the `Delta`.
> +    #[inline]
> +    pub fn as_secs(self) -> i64 {
> +        self.nanos / NSEC_PER_SEC
> +    }
> +}
> -- 
> 2.43.0

On Thu, 17 Oct 2024 12:17:18 +0200
Fiona Behrens <me@kloenk.dev> wrote:

>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>
> Could we also derive PartialEq and Eq (maybe also PartialOrd and
> Ord)? Would need that to compare deltas in my LED driver.

Sure, I'll add.

>> +pub struct Delta {
>> +    nanos: i64,
>> +}
>> +
>
> I think all this functions could be const (need from_millis as const
> for LED, but when at it we could probably make all those const?)

I think that making all the from_* methods const fn make sense. I'll
do.

On Wed, Oct 16, 2024 at 5:53 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.

Is there a reason that Delta wraps i64 directly instead of wrapping
the Ktime type? I'd like to see the reason in the commit message.

Alice

On Wed, 16 Oct 2024 10:23:49 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 16, 2024 at 5:53 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.
> 
> Is there a reason that Delta wraps i64 directly instead of wrapping
> the Ktime type? I'd like to see the reason in the commit message.

You meant that bindings::ktime_t is used instead of i64 like:

pub struct Delta {
    nanos: bindings::ktime_t,
}

? I've not considered but might make sense.