Messages in this thread | | | Date | Fri, 12 Apr 2024 18:30:34 -0700 | From | Boqun Feng <> | Subject | Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub() |
| |
On Fri, Apr 12, 2024 at 04:41:26PM +0200, Miguel Ojeda wrote: > On Fri, Apr 12, 2024 at 3:34 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > That works for me, although I would prefer `Ktime::sub()` is wrapping > > sub and we have another function doing a safe version of sub. > > Why? It goes against the "normal" case in integers. It is also not > what `ktime_sub()` does, which is the "normal" case here, vs.
Seems we have a different reading of `ktime_sub()` ;-)
Based on your reply to Philipp, I take it that CONFIG_RUST_CHECK_OVERFLOWS can be enabled in a production kernel, right? IOW, it's not a debug-only feature like UBSAN (or maybe I'm way wrong, that UBSAN is also a feature that production kernel can or already use?). If so, then the current `Ktime::sub()` has a different behavior compared to `ktime_sub()`: it will perform overflow checks and panic (which is BUG()) in production kernels.
Now I wasn't trying to say substraction overflows shouldn't be checked (by default), the thing is that `Ktime` is just a `ktime_t` wrapper, so it's natural that it provides as least difference as possible. If it was a standalone abstraction, then by all means let's add different APIs for different purpose.
If you look at ktime API, ktime_sub() is the only one doing substraction between two ktime_t, there is no raw or unsafe or safe API, So as a minimal abstraction, it's natural for a user to expect `Ktime::sub()` behaves like `ktime_sub()`.
That's my reasoning, but it depends one a few "if"s and what time subsystem wants to do.
> `_unsafe()` and `_safe()` ones. > > > Exactly, ktime_add_safe() doesn't panic if overflow happens, right? > > I think that's pretty clear on how time subsystem wants to handle > > overflow (saturating it, or zeroing it instead of panicing). > > There are three variants in C (for addition) that I can see: > > - No suffix: not supposed to wrap. > - `_unsafe()`: wraps. > - `_safe()`: saturates. > > The first one, in normal C, would be UB. In kernel C, it wraps but may > be detected by UBSAN (this is what Kees is re-introducing very > recently with 557f8c582a9b ("ubsan: Reintroduce signed overflow > sanitizer")). > > So, in Rust terms, the three options above would map to: > > - Raw operators. > - `wrapping_`. > - `saturating_`. > > Because the raw operators are what we use for arithmetic that is "not > supposed to wrap" too. That is, they wrap, but may be checked by the > Kconfig option. Of course, it may be worth having an intermediate > option that does not actually go for a full-blown Rust-panic for that, > but the point is that the current "not supposed to wrap" methods are > the raw operators. > > All three, in fact, are "safe" in Rust terms, since none can actually > trigger UB (in kernel C at least -- it would be different in normal C: > the first one would map to an unsafe Rust method, i.e. `unchecked_`). > > Instead, in the C side, `_unsafe()` seems to be used to mean instead > "you should be checking for overflow if needed, because it will never > be reported by UBSAN unlike the raw one". Again, this is based on my > reading of that commit and the docs on `_unsafe()`. It may be wrong, > or maybe the subtraction is supposed to be different. It should > probably be clarified in the C side anyway. > > And, relatedly, I see that when the `union` was removed in commit > 2456e8553544 ("ktime: Get rid of the union"), `ktime_add_unsafe()` > stopped returning a `ktime_t` even when both inputs are `ktime_t`s > themselves: > > static_assert(_Generic(ktime_add(a, b), ktime_t: true, default: > false)); // OK > static_assert(_Generic(ktime_add_unsafe(a, b), ktime_t: true, > default: false)); // Bad > > It returns an `u64` now, which could surprise users, and probably > should be fixed. The only user just puts the result into a `ktime_t`, > so there is no actual issue today. > > > I must defer this to Thomas. > > Yeah, the question on the C API was meant for Thomas et al. >
Maybe it's wise to just wait for them to reply, I don't think you and I have much disagree other than ktime_t API semantics ;-)
Regards, Boqun
> > Maybe, however neither of this function probably shouldn't have the > > panic-on-overflow behavior. So I agree that overflow checking is not a > > bad thing, but when to check and how to handle overflow should be > > controlled by the users, and making the default behavior > > panic-on-overflow doesn't look reasonable to me. > > Yes, it should be controlled by callers, but the point above is that, > from the looks of it, these interfaces are not meant to overflow to > begin with. > > Cheers, > Miguel
| |