Received-SPF: pass (google.com: domain of linux-kernel+bounces-119641-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Feedback-ID: iad51458e:Fastmail
Date: Tue, 26 Mar 2024 11:00:52 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Benno Lossin <benno.lossin@proton.me>
Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Alice Ryhl <aliceryhl@google.com>, Miguel Ojeda <ojeda@kernel.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	John Stultz <jstultz@google.com>, Stephen Boyd <sboyd@kernel.org>,
	Valentin Obst <kernel@valentinobst.de>,
	Heghedus Razvan <heghedus.razvan@protonmail.com>,
	Asahi Lina <lina@asahilina.net>
Subject: Re: [PATCH 5/5] rust: time: Add Instant::elapsed() for monotonic
 clocks
Message-ID: <ZgMNVNOjoLH4S4Fb@boqun-archlinux>
References: <20240324223339.971934-1-boqun.feng@gmail.com>
 <20240324223339.971934-6-boqun.feng@gmail.com>
 <kZmwObXVcyEJFVR3I05Nab0WdjcccDVARoOm6U9EmXhd89fxYRjvNmEAUZsueWTUlBlHp8jpU3i2YfgN9aWCLZameta0tPT_OgQdBOWUIko=@proton.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <kZmwObXVcyEJFVR3I05Nab0WdjcccDVARoOm6U9EmXhd89fxYRjvNmEAUZsueWTUlBlHp8jpU3i2YfgN9aWCLZameta0tPT_OgQdBOWUIko=@proton.me>

On Tue, Mar 26, 2024 at 05:13:38PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:
> > This is a convenient way to do:
> > 
> > 	t1 = Clock::now();
> > 	...
> > 	delta =  Clock::now() - t1;
> > 
> > Hence add it.
> > 
> > Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> > Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/time.rs | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 5cd669cbea01..cd1e45169517 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -114,6 +114,31 @@ fn sub(self, other: Self) -> Self::Output {
> >      }
> >  }
> > 
> > +impl<T: Clock + Monotonic> Instant<T> {
> > +    /// Returns the time elapsed since this [`Instant`].
> > +    ///
> > +    /// This provides a convenient way to calculate time elapsed since a previous [`Clock::now`].
> > +    /// Note even though the function only exists for monotonic clocks, it could still return
> > +    /// negative [`Duration`] if the current time is earlier than the time of `&self`, and this
> > +    /// could happen if `&self` is a timestamp generated by a [`Instant`] + [`Duration`].
> 
> But there currently is no way to add an `Instant<T>` to a `Duration`.
> 

This is kinda the disadvantages of "upstreaming the bits you only need",
we know for sure there will be a way to generate an `Instant` with an
addition of a `Duration`. I can of course provide that function in this
series. But let's settle down on "negative durations" first.

> > +    ///
> > +    /// But for typical usages, it should always return non-negative [`Duration`]:
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::time::{Clock, clock::KernelTime};
> > +    ///
> > +    /// let ts = KernelTime::now();
> > +    ///
> > +    /// // `KernelTime` is monotonic.
> > +    /// assert!(ts.elapsed().to_ns() >= 0);
> 
> Now that I thought a bit more about the design, I think allowing
> negative durations is a bad idea.
> Do you disagree?
> 

So yes, I don't think allowing negative duration is really good design.
But as I mentioned in the cover letter, I hope to support cases where:

	d = ts2 - ts1;
	ts = ts3 + d;

	(where ts1, ts2, ts3 is Instant, and d is of course Duration)

without any branch instruction in the asm code. It's useful in the case
where ts1 is a old time base, and ts3 is the new one, and you want to
"remain" the delta between ts2 and t1 and apply that on ts3. To me there
are three options to achieve that: 1) allow negative durations (this
also mirrors what `ktime_t` represents for timedelta AKAIU), 2) have
a timedelta type that differs from Duration, and it can be negative, 3)
provide a function to do the above calculation for `Instant`. I choose
the first one because it's quick and simple (also easy to map to
`ktime_t`). But I don't have my own preference on these three options.

Regards,
Boqun

> If there is a case where you have a non-monotonic clock, or you are not
> sure if two timestamps are in the correct relation, we could have a
> function that returns a `Option<Duration>` or `Result<Duration>`.
> 
> -- 
> Cheers,
> Benno
> 
> > +    /// ```
> > +    pub fn elapsed(&self) -> Duration {
> > +        T::now() - *self
> > +    }
> > +}
> > +
> >  /// Contains the various clock source types available to the kernel.
> >  pub mod clock {
> >      use super::*;
> > --
> > 2.44.0
> >