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

[Boqun Feng <boqun.feng@gmail.com>]

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