Introduce a type representing a span of time. Define our own type
because `core::time::Duration` is large and could panic during
creation.
We could use time::Ktime for time duration but timestamp and timedelta
are different so better to use a new type.
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/kernel/time.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index c40105941a2c..6c5a1c50c5f1 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;
@@ -103,3 +109,61 @@ fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
}
}
}
+
+/// 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: u16) -> Self {
+ Self {
+ nanos: nanos.into(),
+ }
+ }
+
+ /// Create a new `Delta` from a number of microseconds.
+ #[inline]
+ pub fn from_micros(micros: u16) -> Self {
+ Self {
+ nanos: micros as i64 * NSEC_PER_USEC,
+ }
+ }
+
+ /// Create a new `Delta` from a number of milliseconds.
+ #[inline]
+ pub fn from_millis(millis: u16) -> Self {
+ Self {
+ nanos: millis as i64 * NSEC_PER_MSEC,
+ }
+ }
+
+ /// Create a new `Delta` from a number of seconds.
+ #[inline]
+ pub fn from_secs(secs: u16) -> Self {
+ Self {
+ nanos: secs as i64 * 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
+ }
+}
--
2.34.1On Sat, Oct 05, 2024 at 09:25:27PM +0900, 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.
>
> We could use time::Ktime for time duration but timestamp and timedelta
> are different so better to use a new type.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/kernel/time.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index c40105941a2c..6c5a1c50c5f1 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;
>
> @@ -103,3 +109,61 @@ fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
> }
> }
> }
> +
> +/// 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: u16) -> Self {
> + Self {
> + nanos: nanos.into(),
> + }
> + }
Just throwing out an idea:
How about we clamp delay to ~1 year, with a pr_warn() if it needs to
actually clamp. All the APIs take or return a u64.
> + /// Return the number of microseconds in the `Delta`.
> + #[inline]
> + pub fn as_micros(self) -> i64 {
> + self.nanos / NSEC_PER_USEC
> + }
Another dumb rust question. How does the Rust compiler implement 64
bit division on 32 bit systems? GCC with C calls out to a library to
do it, and the kernel does not have that library. So you need to use
the kernel div_u64() function.
Did you compiler this code for a 32 bit system?
Andrew
> +/// A span of time.
> +#[derive(Copy, Clone)]
> +pub struct Delta {
> + nanos: i64,
Is there are use case for negative Deltas ? Should this be u64?
A u64 would allow upto 500 years, if i did my back of an envelope
maths correct. So i suppose 250 years allowing negative delta would
also work.
> +}
> +
> +impl Delta {
> + /// Create a new `Delta` from a number of nanoseconds.
> + #[inline]
> + pub fn from_nanos(nanos: u16) -> Self {
So here you don't allow negative values.
But why limit it to u16, when the base value is a 63 bits? 65535 nS is
not very long.
> + Self {
> + nanos: nanos.into(),
> + }
> + }
> +
> + /// Create a new `Delta` from a number of microseconds.
> + #[inline]
> + pub fn from_micros(micros: u16) -> Self {
A u32 should not overflow when converted to nS in an i64.
Dumb question. What does Rust in the kernel do if there is an
overflow?
> + /// 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
> + }
> +}
So here we are back to signed values. And also you cannot create a
Delta from a Delta because the types are not transitive.
Andrew
On Sat, 5 Oct 2024 20:02:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:
>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>> +pub struct Delta {
>> + nanos: i64,
>
> Is there are use case for negative Deltas ? Should this be u64?
After investigating ktime_us_delta() and ktime_ms_delta() users, I
found that ten or so places which use nagative Deltas like:
timedout_ktime = ktime_add_us(ktime_get(), some_usecs);
// Do something that takes time
remaining = ktime_us_delta(timedout_ktime, ktime_get());
if (remaining > 0)
fsleep(remaining)
Looks straightforward. On second thought, I feel it would be better to
support nagative Deltas. We could use i64 everywhere.
And i64 is more compatible with Ktime Delta APIs; ktime_us_delta and
ktime_ms_delta returns s64; we create Delta without type conversion.
On Sat, 5 Oct 2024 20:02:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:
>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>> +pub struct Delta {
>> + nanos: i64,
>
> Is there are use case for negative Deltas ? Should this be u64?
I thought that logically Delta could be negative but considering Ktime
APIs like the following, I think that u64 is more appropriate now.
static inline ktime_t ktime_add_us(const ktime_t kt, const u64 usec)
{
return ktime_add_ns(kt, usec * NSEC_PER_USEC);
}
static inline ktime_t ktime_sub_us(const ktime_t kt, const u64 usec)
{
return ktime_sub_ns(kt, usec * NSEC_PER_USEC);
}
>> +}
>> +
>> +impl Delta {
>> + /// Create a new `Delta` from a number of nanoseconds.
>> + #[inline]
>> + pub fn from_nanos(nanos: u16) -> Self {
>
> So here you don't allow negative values.
>
> But why limit it to u16, when the base value is a 63 bits? 65535 nS is
> not very long.
I thought that from_secs(u16) gives long enough duration but
how about the following APIs?
pub fn from_nanos(nanos: u64)
pub fn from_micros(micros: u32)
pub fn from_millis(millis: u16)
You can create the maximum via from_nanos. from_micros and from_millis
don't cause wrapping.
> I thought that from_secs(u16) gives long enough duration but > how about the following APIs? > > pub fn from_nanos(nanos: u64) > pub fn from_micros(micros: u32) > pub fn from_millis(millis: u16) > > You can create the maximum via from_nanos. from_micros and from_millis > don't cause wrapping. When i talked about transitive types, i was meaning that to_nanos(), to_micros(), to_millis() should have the same type as from_nanos(), to_micros(), and to_millis(). It is clear these APIs cause discard. millis is a lot less accurate than nanos. Which is fine, the names make that obvious. But what about the range? Are there values i can create using from_nanos() which i cannot then use to_millis() on because it overflows the u16? And i guess the overflow point is different to to_micros()? This API feels inconsistent to me. This is why i suggested u64 is used everywhere. And we avoid the range issues, by artificially clamping to something which can be represented in all forms, so we have a uniform behaviour. But i have little experience of dealing with time in the kernel. I don't know what the real issues are here, what developers have been getting wrong for the last 30 years etc. Andrew
On Mon, 7 Oct 2024 15:33:13 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> I thought that from_secs(u16) gives long enough duration but >> how about the following APIs? >> >> pub fn from_nanos(nanos: u64) >> pub fn from_micros(micros: u32) >> pub fn from_millis(millis: u16) >> >> You can create the maximum via from_nanos. from_micros and from_millis >> don't cause wrapping. > > When i talked about transitive types, i was meaning that to_nanos(), > to_micros(), to_millis() should have the same type as from_nanos(), > to_micros(), and to_millis(). > > It is clear these APIs cause discard. millis is a lot less accurate > than nanos. Which is fine, the names make that obvious. But what about > the range? Are there values i can create using from_nanos() which i > cannot then use to_millis() on because it overflows the u16? And i > guess the overflow point is different to to_micros()? This API feels > inconsistent to me. This is why i suggested u64 is used > everywhere. And we avoid the range issues, by artificially clamping to > something which can be represented in all forms, so we have a uniform > behaviour. I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds for all the from_* functions.
On Wed, 09 Oct 2024 23:00:15 +0900 (JST) FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > On Mon, 7 Oct 2024 15:33:13 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > >> I thought that from_secs(u16) gives long enough duration but > >> how about the following APIs? > >> > >> pub fn from_nanos(nanos: u64) > >> pub fn from_micros(micros: u32) > >> pub fn from_millis(millis: u16) > >> > >> You can create the maximum via from_nanos. from_micros and from_millis > >> don't cause wrapping. > > > > When i talked about transitive types, i was meaning that to_nanos(), > > to_micros(), to_millis() should have the same type as from_nanos(), > > to_micros(), and to_millis(). > > > > It is clear these APIs cause discard. millis is a lot less accurate > > than nanos. Which is fine, the names make that obvious. But what about > > the range? Are there values i can create using from_nanos() which i > > cannot then use to_millis() on because it overflows the u16? And i > > guess the overflow point is different to to_micros()? This API feels > > inconsistent to me. This is why i suggested u64 is used > > everywhere. And we avoid the range issues, by artificially clamping to > > something which can be represented in all forms, so we have a uniform > > behaviour. > > I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds > for all the from_* functions. If you do, I'd recommend to call it `Duration` rather than `Delta`. `Delta` sounds to me that it can represent a negative delta, where `Duration` makes sense to be non-negative. And it also makes sense that `kernel::time::Duration` is the replacement of `core::time::Duration`. Thanks, Gary
On Sat, 12 Oct 2024 19:56:52 +0100 Gary Guo <gary@garyguo.net> wrote: >> I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds >> for all the from_* functions. > > If you do, I'd recommend to call it `Duration` rather than `Delta`. > `Delta` sounds to me that it can represent a negative delta, where > `Duration` makes sense to be non-negative. > > And it also makes sense that `kernel::time::Duration` is the replacement > of `core::time::Duration`. Ok, `Duration` also works for me. I had kinda impression that it's better not to use `Duration`.
On Sat, Oct 5, 2024 at 8:03 PM Andrew Lunn <andrew@lunn.ch> wrote: > > But why limit it to u16, when the base value is a 63 bits? 65535 nS is > not very long. Agreed, for these constructors we should use (at the very least) the biggest possible type that fits without possible wrapping. > Dumb question. What does Rust in the kernel do if there is an > overflow? It is controlled by `CONFIG_RUST_OVERFLOW_CHECKS` -- it defaults to a Rust panic, but otherwise it wraps. Either way, it is not UB and regardless of the setting, it is considered a bug to be fixed. (I requested upstream Rust a third mode that would allow us to report the issue (e.g. map it to `WARN_ONCE`) and then continue with wrap.) Cheers, Miguel
© 2016 - 2024 Red Hat, Inc.