Received-SPF: pass (google.com: domain of linux-kernel+bounces-142738-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: iad51458e:Fastmail
Date: Fri, 12 Apr 2024 06:34:10 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Miguel Ojeda <ojeda@kernel.org>,
	John Stultz <jstultz@google.com>, Stephen Boyd <sboyd@kernel.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Gary Guo <gary@garyguo.net>, bjorn3_gh@protonmail.com,
	Benno Lossin <benno.lossin@proton.me>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	Alice Ryhl <aliceryhl@google.com>, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()
Message-ID: <Zhk4Uqc4LImR2n_r@Boquns-Mac-mini.home>
References: <20240411230801.1504496-1-boqun.feng@gmail.com>
 <20240411230801.1504496-3-boqun.feng@gmail.com>
 <CANiq72nSSAVkynVaAq7bQKoL6N8K2JUXp8AOVvu7vN+siAhk-Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANiq72nSSAVkynVaAq7bQKoL6N8K2JUXp8AOVvu7vN+siAhk-Q@mail.gmail.com>

On Fri, Apr 12, 2024 at 09:14:03AM +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> 
> Nit: it is enabled by default, but configurable (`CONFIG_RUST_OVERFLOW_CHECKS`).
> 

Ok, I will change it accordingly.

> > although overflow detection is nice to have, however this makes
> > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> > clear that the overflow checking is helpful, since for example, the
> > current binder usage[1] doesn't have the checking.
> >
> > Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> > overflow behaves like 2s-complement wrapping sub.
> 
> If `ktime_sub()`'s callers rely on wrapping in some cases, then an
> alternative we should consider is having a method for explicitly
> wrapping, like the integers. This would allow callers to decide and it

That works for me, although I would prefer `Ktime::sub()` is wrapping
sub and we have another function doing a safe version of sub.

> would make the expected semantics clear since the beginning (which is
> the easiest time to add this kind of thing) for Rust code.
> 
> Otherwise, I agree we should at least document the preconditions clearly.
> 
> Having said that, I see a `ktime_add_unsafe()` too, which was added
> due to a UBSAN report for `ktime_add()` in commit 979515c56458 ("time:
> Avoid undefined behaviour in ktime_add_safe()"). There is also a
> private `ktime_add_safe()` too, which is a saturating one.
> 

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).

> So, given that, can callers actually rely on wrapping for these
> functions, or not? The documentation on the C side could perhaps be
> clarified here (including the mention of UB in `ktime_add_unsafe()` --
> we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C
> functions too.
> 

I must defer this to Thomas.

> In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`,
> right? In that case the arguments are called `later` and `earlier`,
> perhaps those have a different expectation even if `ktime_sub()` is
> allowed to overflow and thus it would make sense to check in that
> function only instead? (and document accordingly)
> 

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.

Regards,
Boqun

> Thanks!
> 
> Cheers,
> Miguel