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