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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87cyd4bjcp.fsf@kernel.org>
Date: Tue, 22 Apr 2025 12:07:02 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "FUJITA Tomonori" <fujita.tomonori@...il.com>
Cc: <boqun.feng@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <gary@...yguo.net>,  <me@...enk.dev>,  <daniel.almeida@...labora.com>,
  <linux-kernel@...r.kernel.org>,  <netdev@...r.kernel.org>,
  <andrew@...n.ch>,  <hkallweit1@...il.com>,  <tmgross@...ch.edu>,
  <ojeda@...nel.org>,  <alex.gaynor@...il.com>,
  <bjorn3_gh@...tonmail.com>,  <benno.lossin@...ton.me>,
  <a.hindborg@...sung.com>,  <aliceryhl@...gle.com>,
  <anna-maria@...utronix.de>,  <frederic@...nel.org>,
  <tglx@...utronix.de>,  <arnd@...db.de>,  <jstultz@...gle.com>,
  <sboyd@...nel.org>,  <mingo@...hat.com>,  <peterz@...radead.org>,
  <juri.lelli@...hat.com>,  <vincent.guittot@...aro.org>,
  <dietmar.eggemann@....com>,  <rostedt@...dmis.org>,
  <bsegall@...gle.com>,  <mgorman@...e.de>,  <vschneid@...hat.com>,
  <tgunders@...hat.com>,  <david.laight.linux@...il.com>
Subject: Re: [PATCH v13 3/5] rust: time: Introduce Instant type

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

> On Tue, 15 Apr 2025 11:01:30 -0700
> Boqun Feng <boqun.feng@...il.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@...nel.org> wrote:
>>>
>>> > "Boqun Feng" <boqun.feng@...il.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@...il.com>
>>> >>
>>> >> I probably need to drop my Reviewed-by because of something below:
>>> >>
>>> >>> Reviewed-by: Gary Guo <gary@...yguo.net>
>>> >>> Reviewed-by: Fiona Behrens <me@...enk.dev>
>>> >>> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
>>> >>> Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
>>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.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