Message-ID: <9EAfAZHp8DflAFFp9hRs3vo2uuwLkwdGSRUVLQiIpCMHcKvjE95lT-VnO8DzJbL7u5Tw66qe-0_R56nwJsdcpeRGeS0794WzOPPexJZR9PA=@proton.me>
Date: Wed, 27 Mar 2024 11:31:10 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, John Stultz <jstultz@...gle.com>, Stephen Boyd <sboyd@...nel.org>, Valentin Obst <kernel@...entinobst.de>, Heghedus Razvan <heghedus.razvan@...tonmail.com>, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH 5/5] rust: time: Add Instant::elapsed() for monotonic clocks

On 27.03.24 02:09, Boqun Feng wrote:
> 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@...tonmail.com>
>>>> Signed-off-by: Heghedus Razvan <heghedus.razvan@...tonmail.com>
>>>> Co-developed-by: Asahi Lina <lina@...hilina.net>
>>>> Signed-off-by: Asahi Lina <lina@...hilina.net>
>>>> Signed-off-by: Boqun Feng <boqun.feng@...il.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...

I also wanted to suggest this. Another name for `KTime` could be
`Timestamp`. I think `Timedelta` is a good fit for a duration-like type
that allows negative values.

I don't remember exactly what binder needed the time stuff for, but
given that you cannot create negative durations with this design, I
think going for the `Duration` and `Instant` approach in this series
should be fine, right?
If then later you want to compute `Timedelta`s between `Timestamp`s we
can have another series.

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

I think that if the strongest motivation for allowing negative duration
is "it is more performant, since we don't need as many branches", then
the motivation for disallowing negative durations is stronger.

I think having multiple types is the solution.

-- 
Cheers,
Benno

Powered by blists - more mailing lists