| Message ID | 20241005122531.20298-3-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
> +/// 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, 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
On 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
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, 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.
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 + } +}
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(+)