| Message ID | 20241016035214.2229-5-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: > > Implement Add<Delta> for Ktime to support the operation: > > Ktime = Ktime + Delta > > This is typically used to calculate the future time when the timeout > will occur. > > timeout Ktime = current Ktime (via ktime_get()) + Delta; > // do something > if (ktime_get() > timeout Ktime) { > // timed out > } > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> > rust/kernel/time.rs | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs > index 8c00854db58c..9b0537b63cf7 100644 > --- a/rust/kernel/time.rs > +++ b/rust/kernel/time.rs > @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 { > self.nanos / NSEC_PER_SEC > } > } > + > +impl core::ops::Add<Delta> for Ktime { > + type Output = Ktime; > + > + #[inline] > + fn add(self, delta: Delta) -> Ktime { > + Ktime { > + inner: self.inner + delta.as_nanos(), You don't want to do `delta.inner` here? Alice
On Wed, Oct 16, 2024 at 12:52:09PM +0900, FUJITA Tomonori wrote: > Implement Add<Delta> for Ktime to support the operation: > > Ktime = Ktime + Delta > > This is typically used to calculate the future time when the timeout > will occur. > > timeout Ktime = current Ktime (via ktime_get()) + Delta; > // do something > if (ktime_get() > timeout Ktime) { > // timed out > } > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/time.rs | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs > index 8c00854db58c..9b0537b63cf7 100644 > --- a/rust/kernel/time.rs > +++ b/rust/kernel/time.rs > @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 { > self.nanos / NSEC_PER_SEC > } > } > + > +impl core::ops::Add<Delta> for Ktime { > + type Output = Ktime; > + > + #[inline] > + fn add(self, delta: Delta) -> Ktime { > + Ktime { > + inner: self.inner + delta.as_nanos(), What if overflow happens in this addition? Is the expectation that user should avoid overflows? I asked because we have ktime_add_safe() which saturate at KTIME_SEC_MAX. Regards, Boqun > + } > + } > +} > -- > 2.43.0 > >
On Wed, 16 Oct 2024 12:54:07 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: >> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs >> index 8c00854db58c..9b0537b63cf7 100644 >> --- a/rust/kernel/time.rs >> +++ b/rust/kernel/time.rs >> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 { >> self.nanos / NSEC_PER_SEC >> } >> } >> + >> +impl core::ops::Add<Delta> for Ktime { >> + type Output = Ktime; >> + >> + #[inline] >> + fn add(self, delta: Delta) -> Ktime { >> + Ktime { >> + inner: self.inner + delta.as_nanos(), > > What if overflow happens in this addition? Is the expectation that user > should avoid overflows? Yes, I'll add a comment. > I asked because we have ktime_add_safe() which saturate at > KTIME_SEC_MAX. We could add the Rust version of add_safe method. But looks like ktime_add_safe() is used by only some core systems so we don't need to add it now?
On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Wed, 16 Oct 2024 12:54:07 -0700 > Boqun Feng <boqun.feng@gmail.com> wrote: > > >> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs > >> index 8c00854db58c..9b0537b63cf7 100644 > >> --- a/rust/kernel/time.rs > >> +++ b/rust/kernel/time.rs > >> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 { > >> self.nanos / NSEC_PER_SEC > >> } > >> } > >> + > >> +impl core::ops::Add<Delta> for Ktime { > >> + type Output = Ktime; > >> + > >> + #[inline] > >> + fn add(self, delta: Delta) -> Ktime { > >> + Ktime { > >> + inner: self.inner + delta.as_nanos(), > > > > What if overflow happens in this addition? Is the expectation that user > > should avoid overflows? > > Yes, I'll add a comment. > > > I asked because we have ktime_add_safe() which saturate at > > KTIME_SEC_MAX. > > We could add the Rust version of add_safe method. But looks like > ktime_add_safe() is used by only some core systems so we don't need to > add it now? I think it makes sense to follow the standard Rust addition conventions here. Rust normally treats + as addition that BUGs on overflow (with the appropriate configs set), and then there's a saturating_add function for when you want it to saturate. Alice
On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > We could add the Rust version of add_safe method. But looks like > ktime_add_safe() is used by only some core systems so we don't need to > add it now? There was some discussion in the past about this -- I wrote there a summary of the `add` variants: https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/ I think this is a case where following the naming of the C side would be worse, i.e. where it is worth not applying our usual guideline. Calling something `_safe`/`_unsafe` like the C macros would be quite confusing for Rust. Personally, I would prefer that we stay consistent, which will help when dealing with more code. That is (from the message above): - No suffix: not supposed to wrap. So, in Rust, map it to operators. - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods. - `_safe()`: saturates. So, in Rust, map it to `saturating` methods. (assuming I read the C code correctly back then.) And if there are any others that are Rust-unsafe, then map it to `unchecked` methods, of course. Cheers, Miguel
On Thu, Oct 17, 2024 at 06:33:23PM +0200, Miguel Ojeda wrote: > On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > We could add the Rust version of add_safe method. But looks like > > ktime_add_safe() is used by only some core systems so we don't need to > > add it now? > > There was some discussion in the past about this -- I wrote there a > summary of the `add` variants: > > https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/ > > I think this is a case where following the naming of the C side would > be worse, i.e. where it is worth not applying our usual guideline. > Calling something `_safe`/`_unsafe` like the C macros would be quite > confusing for Rust. > > Personally, I would prefer that we stay consistent, which will help > when dealing with more code. That is (from the message above): > > - No suffix: not supposed to wrap. So, in Rust, map it to operators. > - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods. > - `_safe()`: saturates. So, in Rust, map it to `saturating` methods. > > (assuming I read the C code correctly back then.) > > And if there are any others that are Rust-unsafe, then map it to > `unchecked` methods, of course. > The point I tried to make is that `+` operator of Ktime can cause overflow because of *user inputs*, unlike the `-` operator of Ktime, which cannot cause overflow as long as Ktime is implemented correctly (as a timestamp). Because the overflow possiblity is exposed to users, then we need to 1) document it and 2) provide saturating_add() (maybe also checked_add() and overflowing_add()) so that users won't need to do the saturating themselves: let mut kt = Ktime::ktime_get(); let d: Delta = <maybe a userspace input>; // kt + d may overflow, so checking if let Some(_) = kt.as_ns().checked_add(d.as_nanos()) { // not overflow, can add kt = kt + d; } else { // set kt to KTIME_SEC_MAX } instead, they can do: let kt = Ktime::ktime_get(); let d: Delta = <maybe a userspace input>; kt = kt.saturating_add(d); but one thing I'm not sure is since it looks like saturating to KTIME_SEC_MAX is the current C choice, if we want to do the same, should we use the name `add_safe()` instead of `saturating_add()`? FWIW, it seems harmless to saturate at KTIME_MAX to me. So personally, I like what Alice suggested. Hope these make sense. Regards, Boqun > Cheers, > Miguel
On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > The point I tried to make is that `+` operator of Ktime can cause > overflow because of *user inputs*, unlike the `-` operator of Ktime, > which cannot cause overflow as long as Ktime is implemented correctly > (as a timestamp). Because the overflow possiblity is exposed to users, > then we need to 1) document it and 2) provide saturating_add() (maybe > also checked_add() and overflowing_add()) so that users won't need to do > the saturating themselves: Generally, operators are expected to possibly wrap/panic, so that would be fine, no? It may be surprising to `ktime_t` users, and that is bad. It is one more reason to avoid using the same name for the type, too. My only concern is that people may expect this sort of thing (timing related) to "usually/just work" and not expect panics. That is bad, but if we remain consistent, it may be best to pay the cost now. In other words, when someone sees an operator, it is saying it is never supposed to wrap, and that is easy to remember. Perhaps some types could avoid providing the operators if it is too dangerous to use them. The other option is be inconsistent in our use of operators, and instead give operators the "best" semantics for each type. That can definitely provide better ergonomics in some cases, but there is a high risk of forgetting what each operator does for each type -- operator overloading can be quite risky. So that is why I think it may be best/easier to generally say "we don't do operator overloading in general unless the operator really behaves like a core one", especially early on. > kt = kt.saturating_add(d); Yeah, that is what we want (I may be missing something in your argument though). > but one thing I'm not sure is since it looks like saturating to > KTIME_SEC_MAX is the current C choice, if we want to do the same, should > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it > seems harmless to saturate at KTIME_MAX to me. So personally, I like > what Alice suggested. Do you mean it would be confusing to not saturate to the highest the lower/inner level type can hold? I mean, nothing says we need to saturate to the highest -- we could have a type invariant. (One more reason to avoid the same name). Worst case, we can have two saturating methods, or two types, if really needed. Thomas can probably tell us what are the most important use cases needed, and whether it is a good opportunity to clean/redesign this in Rust differently. Cheers, Miguel
On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > but one thing I'm not sure is since it looks like saturating to > KTIME_SEC_MAX is the current C choice, if we want to do the same, should > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it > seems harmless to saturate at KTIME_MAX to me. So personally, I like Wait -- `ktime_add_safe()` calls `ktime_set(KTIME_SEC_MAX, 0)` which goes into the conditional that returns `KTIME_MAX`, not `KTIME_SEC_MAX * NSEC_PER_SEC` (which is what I guess you were saying). So I am confused -- it doesn't saturate to `KTIME_SEC_MAX` (scaled) anyway. Which is confusing in itself. In fact, it means that `ktime_add_safe()` allows you to get any value whatsoever as long as you don't overflow, but `ktime_set` does not allow you to -- unless you use enough nanoseconds to get you there (i.e. over a second in nanoseconds). Cheers, Miguel
On Thu, Oct 17, 2024 at 08:10:17PM +0200, Miguel Ojeda wrote: > On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > The point I tried to make is that `+` operator of Ktime can cause > > overflow because of *user inputs*, unlike the `-` operator of Ktime, > > which cannot cause overflow as long as Ktime is implemented correctly > > (as a timestamp). Because the overflow possiblity is exposed to users, > > then we need to 1) document it and 2) provide saturating_add() (maybe > > also checked_add() and overflowing_add()) so that users won't need to do > > the saturating themselves: > > Generally, operators are expected to possibly wrap/panic, so that > would be fine, no? > Yes, but I guess I need to make it clear: the current `+` operator implemention is fine for me. What I'm trying to say is: since we have a `+` that expects users to not provide inputs that causes overflow, then it makes sense to provide a saturating_add() at the same time for the API completeness. > It may be surprising to `ktime_t` users, and that is bad. It is one > more reason to avoid using the same name for the type, too. > > My only concern is that people may expect this sort of thing (timing > related) to "usually/just work" and not expect panics. That is bad, > but if we remain consistent, it may be best to pay the cost now. In > other words, when someone sees an operator, it is saying it is never > supposed to wrap, and that is easy to remember. Perhaps some types > could avoid providing the operators if it is too dangerous to use > them. > Sounds reasonable to me. > The other option is be inconsistent in our use of operators, and > instead give operators the "best" semantics for each type. That can > definitely provide better ergonomics in some cases, but there is a > high risk of forgetting what each operator does for each type -- > operator overloading can be quite risky. > Agreed. > So that is why I think it may be best/easier to generally say "we > don't do operator overloading in general unless the operator really > behaves like a core one", especially early on. > > > kt = kt.saturating_add(d); > > Yeah, that is what we want (I may be missing something in your argument though). > > > but one thing I'm not sure is since it looks like saturating to > > KTIME_SEC_MAX is the current C choice, if we want to do the same, should > > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it > > seems harmless to saturate at KTIME_MAX to me. So personally, I like > > what Alice suggested. > > Do you mean it would be confusing to not saturate to the highest the > lower/inner level type can hold? > Yes. > I mean, nothing says we need to saturate to the highest -- we could > have a type invariant. (One more reason to avoid the same name). > > Worst case, we can have two saturating methods, or two types, if really needed. > > Thomas can probably tell us what are the most important use cases > needed, and whether it is a good opportunity to clean/redesign this in > Rust differently. > Works for me! Regards, Boqun > Cheers, > Miguel
On Thu, 17 Oct 2024 18:33:23 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> We could add the Rust version of add_safe method. But looks like >> ktime_add_safe() is used by only some core systems so we don't need to >> add it now? > > There was some discussion in the past about this -- I wrote there a > summary of the `add` variants: > > https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/ > > I think this is a case where following the naming of the C side would > be worse, i.e. where it is worth not applying our usual guideline. > Calling something `_safe`/`_unsafe` like the C macros would be quite > confusing for Rust. > > Personally, I would prefer that we stay consistent, which will help > when dealing with more code. That is (from the message above): > > - No suffix: not supposed to wrap. So, in Rust, map it to operators. > - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods. > - `_safe()`: saturates. So, in Rust, map it to `saturating` methods. Can we add the above to Documentation/rust/coding-guidelines.rst? I think that it's better than adding the similar comment to every function that performs arithmetic.
On Wed, Oct 23, 2024 at 8:51 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Can we add the above to Documentation/rust/coding-guidelines.rst? Sounds good to me -- I will send a patch. Just to confirm, do you mean the whole operators overloading guideline that I mentioned elsewhere and what semantics the arithmetic operators should have (i.e.to avoid having to repeatedly document why operator do "not supposed to wrap" and why we relegate saturating/wrapping/... to methods), or something else? Thanks! Cheers, Miguel
On Wed, 23 Oct 2024 12:59:03 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Wed, Oct 23, 2024 at 8:51 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> Can we add the above to Documentation/rust/coding-guidelines.rst? > > Sounds good to me -- I will send a patch. Great, thanks! > Just to confirm, do you mean the whole operators overloading guideline > that I mentioned elsewhere and what semantics the arithmetic operators > should have (i.e.to avoid having to repeatedly document why operator > do "not supposed to wrap" and why we relegate saturating/wrapping/... > to methods), or something else? I was only thinking about the guideline for naming (at least as a starter); your mail in this thread: https://lore.kernel.org/netdev/CANiq72mbWVVCA_EjV_7DtMYHH_RF9P9Br=sRdyLtPFkythST1w@mail.gmail.com/
On Thu, Oct 17, 2024 at 08:58:48PM +0200, Miguel Ojeda wrote: > On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > but one thing I'm not sure is since it looks like saturating to > > KTIME_SEC_MAX is the current C choice, if we want to do the same, should > > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it > > seems harmless to saturate at KTIME_MAX to me. So personally, I like > > Wait -- `ktime_add_safe()` calls `ktime_set(KTIME_SEC_MAX, 0)` which > goes into the conditional that returns `KTIME_MAX`, not `KTIME_SEC_MAX > * NSEC_PER_SEC` (which is what I guess you were saying). > Yeah.. this is very interesting ;-) I missed that. > So I am confused -- it doesn't saturate to `KTIME_SEC_MAX` (scaled) > anyway. Which is confusing in itself. > Then I think it suffices to say ktime_add_safe() is just a saturating_add() for i64? ;-) > In fact, it means that `ktime_add_safe()` allows you to get any value > whatsoever as long as you don't overflow, but `ktime_set` does not > allow you to -- unless you use enough nanoseconds to get you there > (i.e. over a second in nanoseconds). > That seems to the be case. Regards, Boqun > Cheers, > Miguel >
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs index 8c00854db58c..9b0537b63cf7 100644 --- a/rust/kernel/time.rs +++ b/rust/kernel/time.rs @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 { self.nanos / NSEC_PER_SEC } } + +impl core::ops::Add<Delta> for Ktime { + type Output = Ktime; + + #[inline] + fn add(self, delta: Delta) -> Ktime { + Ktime { + inner: self.inner + delta.as_nanos(), + } + } +}
Implement Add<Delta> for Ktime to support the operation: Ktime = Ktime + Delta This is typically used to calculate the future time when the timeout will occur. timeout Ktime = current Ktime (via ktime_get()) + Delta; // do something if (ktime_get() > timeout Ktime) { // timed out } Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/time.rs | 11 +++++++++++ 1 file changed, 11 insertions(+)