Re: [PATCH v13 3/5] rust: time: Introduce Instant type

[Andreas Hindborg <a.hindborg@kernel.org>]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Tue, 15 Apr 2025 11:01:30 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
>
>> On Mon, Apr 14, 2025 at 08:59:54PM +0900, FUJITA Tomonori wrote:
>>> On Mon, 14 Apr 2025 09:04:14 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>> > "Boqun Feng" <boqun.feng@gmail.com> writes:
>>> >
>>> >> On Sun, Apr 13, 2025 at 07:43:08PM +0900, FUJITA Tomonori wrote:
>>> >>> Introduce a type representing a specific point in time. We could use
>>> >>> the Ktime type but C's ktime_t is used for both timestamp and
>>> >>> timedelta. To avoid confusion, introduce a new Instant type for
>>> >>> timestamp.
>>> >>>
>>> >>> Rename Ktime to Instant and modify their methods for timestamp.
>>> >>>
>>> >>> Implement the subtraction operator for Instant:
>>> >>>
>>> >>> Delta = Instant A - Instant B
>>> >>>
>>> >>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>>> >>
>>> >> I probably need to drop my Reviewed-by because of something below:
>>> >>
>>> >>> Reviewed-by: Gary Guo <gary@garyguo.net>
>>> >>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>>> >>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> >>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>> >>> ---
>>> >> [...]
>>> >>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> >>> index ce53f8579d18..27243eaaf8ed 100644
>>> >>> --- a/rust/kernel/time/hrtimer.rs
>>> >>> +++ b/rust/kernel/time/hrtimer.rs
>>> >>> @@ -68,7 +68,7 @@
>>> >>>  //! `start` operation.
>>> >>>
>>> >>>  use super::ClockId;
>>> >>> -use crate::{prelude::*, time::Ktime, types::Opaque};
>>> >>> +use crate::{prelude::*, time::Instant, types::Opaque};
>>> >>>  use core::marker::PhantomData;
>>> >>>  use pin_init::PinInit;
>>> >>>
>>> >>> @@ -189,7 +189,7 @@ pub trait HrTimerPointer: Sync + Sized {
>>> >>>
>>> >>>      /// Start the timer with expiry after `expires` time units. If the timer was
>>> >>>      /// already running, it is restarted with the new expiry time.
>>> >>> -    fn start(self, expires: Ktime) -> Self::TimerHandle;
>>> >>> +    fn start(self, expires: Instant) -> Self::TimerHandle;
>>> >>
>>> >> We should be able to use what I suggested:
>>> >>
>>> >> 	https://lore.kernel.org/rust-for-linux/Z_ALZsnwN53ZPBrB@boqun-archlinux/
>>> >>
>>> >> to make different timer modes (rel or abs) choose different expire type.
>>> >>
>>> >> I don't think we can merge this patch as it is, unfortunately, because
>>> >> it doesn't make sense for a relative timer to take an Instant as expires
>>> >> value.
>>> >
>>> > I told Tomo he could use `Instant` in this location and either he or I
>>> > would fix it up later [1].
>>> >
>>
>> I saw that, however, I don't think we can put `Instant` as the parameter
>> for HrTimerPointer::start() because we don't yet know how long would the
>> fix-it-up-later take. And it would confuse users if they need a put an
>> Instant for relative time.
>>
>>> > I don't want to block the series on this since the new API is not worse
>>> > than the old one where Ktime is overloaded for both uses.
>>
>> How about we keep Ktime? That is HrTimerPointer::start() still uses
>> Ktime, until we totally finish the refactoring as Tomo show below?
>> `Ktime` is much better here because it at least matches C API behavior,
>> we can remove `Ktime` once the dust is settled. Thoughts?
>
> Either is fine with me. I'll leave it to Andreas' judgment.
>
> Andreas, if you like Boqun's approach, I'll replace the third patch
> with the following one and send v14.
>
> I added Ktime struct to hrtimer.rs so the well-reviewed changes to
> time.rs remain unchanged.

OK, Let's keep Ktime for hrtimer for now. I am OK with you putting
`Ktime` in `hrtimer.rs` but you could also put it in time.rs. If you
don't want to modify the patches that already has reviews, you can add
it back in a separate patch.

Either way we should add a `// NOTE: Ktime is going to be removed when
hrtimer is converted to Instant/Duration`.


Best regards,
Andreas Hindborg