Received-SPF: pass (google.com: domain of linux-kernel+bounces-120168-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 18:09:31 -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: <ZgNxy70__7-x0c-8@Boquns-Mac-mini.home>
References: <20240324223339.971934-1-boqun.feng@gmail.com>
 <20240324223339.971934-6-boqun.feng@gmail.com>
 <kZmwObXVcyEJFVR3I05Nab0WdjcccDVARoOm6U9EmXhd89fxYRjvNmEAUZsueWTUlBlHp8jpU3i2YfgN9aWCLZameta0tPT_OgQdBOWUIko=@proton.me>
 <ZgMNVNOjoLH4S4Fb@boqun-archlinux>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZgMNVNOjoLH4S4Fb@boqun-archlinux>

On Tue, Mar 26, 2024 at 11:00:52AM -0700, Boqun Feng wrote:
> 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.
> 

Hmm... I'd like to propose a change here. After some thoughts, I think
we should have two timestamp types: `Instant` and `KTime`, where
`Instant` represents solely a reading of a clocksource, and `KTime` is
just the normal timestamp. This means the only way to get an `Instant`
is via `Clock::now()`, and you cannot get an `Instant` by `Instant` +
`Duration` (this could return a `KTime`). And a `Instant` can always
`into_ktime()` return a `KTime` which support add/sub a duration. But
again you cannot get an `Instant` from a `KTime`.

Having this setup means for the same monotonic clocksource,
`Clock::now()` is always later than any `Instant`, since any `Instant`
must be created by a previous `Clock::now()`. And this makes a lot of
sense. Moreover, I could introduce `KTime` in a later patchset, since
`Instant` and `Duration` can fulfill the current requirement. We still
need two duration types though...

Regards,
Boqun

> > > +    ///
> > > +    /// 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
> > >