Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

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