rust/kernel/time.rs | 19 +- rust/kernel/time/hrtimer.rs | 281 ++++++++++++++++++---------- rust/kernel/time/hrtimer/arc.rs | 8 +- rust/kernel/time/hrtimer/pin.rs | 8 +- rust/kernel/time/hrtimer/pin_mut.rs | 8 +- rust/kernel/time/hrtimer/tbox.rs | 8 +- 6 files changed, 218 insertions(+), 114 deletions(-)
Convert hrtimer to use `Instant` and `Delta`; remove the use of
`Ktime` from the hrtimer code, which was originally introduced as a
temporary workaround.
hrtimer uses either an `Instant` or a `Delta` as its expiration value,
depending on the mode specified at creation time. This patchset
replaces `HrTimerMode` enum with a trait-based abstraction and
associates each mode with either an `Instant` or a `Delta`. By
leveraging Rust's type system, this change enables `HrTimer` to be
statically associated with a specific `HrTimerMode`, the corresponding
`Instant` or `Delta`, and a `ClockSource`.
The `impl_has_hr_timer` macro is extended to allow specifying the
`HrTimerMode`. In the following example, it guarantees that the
`start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
`Delta` or an `Instant` with a different clock source will result in a
compile-time error:
struct Foo {
#[pin]
timer: HrTimer<Self>,
}
impl_has_hr_timer! {
impl HasHrTimer<Self> for Foo {
mode : AbsoluteMode<Monotonic>,
field : self.timer
}
}
This design eliminates runtime mismatches between expires types and
clock sources, and enables stronger type-level guarantees throughout
hrtimer.
This patchset can be applied on top of the typed clock sources patchset (v4):
https://lore.kernel.org/lkml/20250610093258.3435874-1-fujita.tomonori@gmail.com/
v3
- allow optional trailing comma for the field entry in impl_has_hr_timer! macro
v2: https://lore.kernel.org/lkml/20250609102418.3345792-1-fujita.tomonori@gmail.com/
- rebased on 6.16-rc1
- change impl_has_hr_timer! macro format
- remove define_hrtimer_mode! macro
- drop patch to change Delta's methods to take &self instead of self
- add patch to rename Delta's methods from as_* to into_*
v1: https://lore.kernel.org/lkml/20250504045959.238068-1-fujita.tomonori@gmail.com/
FUJITA Tomonori (5):
rust: time: Rename Delta's methods from as_* to into_*
rust: time: Replace HrTimerMode enum with trait-based mode types
rust: time: Add HrTimerExpires trait
rust: time: Make HasHrTimer generic over HrTimerMode
rust: time: Remove Ktime in hrtimer
rust/kernel/time.rs | 19 +-
rust/kernel/time/hrtimer.rs | 281 ++++++++++++++++++----------
rust/kernel/time/hrtimer/arc.rs | 8 +-
rust/kernel/time/hrtimer/pin.rs | 8 +-
rust/kernel/time/hrtimer/pin_mut.rs | 8 +-
rust/kernel/time/hrtimer/tbox.rs | 8 +-
6 files changed, 218 insertions(+), 114 deletions(-)
base-commit: 8bffa361fb76742eb953ca024a9363c6e9357d65
--
2.43.0
On Tue, 10 Jun 2025 22:28:18 +0900, FUJITA Tomonori wrote:
> Convert hrtimer to use `Instant` and `Delta`; remove the use of
> `Ktime` from the hrtimer code, which was originally introduced as a
> temporary workaround.
>
> hrtimer uses either an `Instant` or a `Delta` as its expiration value,
> depending on the mode specified at creation time. This patchset
> replaces `HrTimerMode` enum with a trait-based abstraction and
> associates each mode with either an `Instant` or a `Delta`. By
> leveraging Rust's type system, this change enables `HrTimer` to be
> statically associated with a specific `HrTimerMode`, the corresponding
> `Instant` or `Delta`, and a `ClockSource`.
>
> [...]
Applied, thanks!
[1/5] rust: time: Rename Delta's methods from as_* to into_*
commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37
[2/5] rust: time: Replace HrTimerMode enum with trait-based mode types
commit: 1d1102d098879b5c8fcd9babeadd2930b0a19259
[3/5] rust: time: Add HrTimerExpires trait
commit: f7fe342fc72915f5eb2280d6ea38bc75d480bed0
[4/5] rust: time: Make HasHrTimer generic over HrTimerMode
commit: ab57261bb9dea0e552a5cf8440e0688e6967163d
[5/5] rust: time: Remove Ktime in hrtimer
commit: 994393295c89711531583f6de8f296a30b0d944a
Best regards,
--
Andreas Hindborg <a.hindborg@kernel.org>
Andreas Hindborg <a.hindborg@kernel.org> writes: > On Tue, 10 Jun 2025 22:28:18 +0900, FUJITA Tomonori wrote: >> Convert hrtimer to use `Instant` and `Delta`; remove the use of >> `Ktime` from the hrtimer code, which was originally introduced as a >> temporary workaround. >> >> hrtimer uses either an `Instant` or a `Delta` as its expiration value, >> depending on the mode specified at creation time. This patchset >> replaces `HrTimerMode` enum with a trait-based abstraction and >> associates each mode with either an `Instant` or a `Delta`. By >> leveraging Rust's type system, this change enables `HrTimer` to be >> statically associated with a specific `HrTimerMode`, the corresponding >> `Instant` or `Delta`, and a `ClockSource`. >> >> [...] > > Applied, thanks! > > [1/5] rust: time: Rename Delta's methods from as_* to into_* > commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37 I fixed up the application of this patch in timekeeping-next. Best regards, Andreas Hindborg
On Tue, Jun 17, 2025 at 12:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> [1/5] rust: time: Rename Delta's methods from as_* to into_*
> commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37
Do we want this given the (~ongoing) discussion at
https://lore.kernel.org/rust-for-linux/20250617144155.3903431-2-fujita.tomonori@gmail.com/
?
I noticed due to a conflict in linux-next today.
Cheers,
Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes: > On Tue, Jun 17, 2025 at 12:39 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> [1/5] rust: time: Rename Delta's methods from as_* to into_* >> commit: 2ed94606a0fea693e250e5b8fda11ff8fc240d37 > > Do we want this given the (~ongoing) discussion at > > https://lore.kernel.org/rust-for-linux/20250617144155.3903431-2-fujita.tomonori@gmail.com/ > > ? > My plan is to merge it and go with `into_*`. There are pros and cons for both `to_*` and `into_*`. If someone has objections, they can send a new patch with rationale and we can revisit. Sounds OK? Best regards, Andreas Hindborg
On Tue, Jun 24, 2025 at 1:14 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: > > My plan is to merge it and go with `into_*`. There are pros and cons for > both `to_*` and `into_*`. If someone has objections, they can send a new > patch with rationale and we can revisit. Sounds OK? I would just drop (or revert) the patch. The issue was under discussion, and anyway it seems clear that `into_*` is not the right choice from both the cost and ownership perspectives that we were discussing in the other thread. If this were not a rename and didn't had conflicts, then it wouldn't be a big deal. But given it is wrong and already introduces pain for others (and likely even more pain when we need to rename it back next cycle), it doesn't look like a good idea to keep it. It is early in the cycle anyway. Cheers, Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Tue, Jun 24, 2025 at 1:14 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> My plan is to merge it and go with `into_*`. There are pros and cons for >> both `to_*` and `into_*`. If someone has objections, they can send a new >> patch with rationale and we can revisit. Sounds OK? > > I would just drop (or revert) the patch. The issue was under > discussion, and anyway it seems clear that `into_*` is not the right > choice from both the cost and ownership perspectives that we were > discussing in the other thread. None of the options are the right choice. Cost and ownership _do_ line up for `into_*` in this case. The mismatch is `into_*` is reserved for `T: !Copy`. > > If this were not a rename and didn't had conflicts, then it wouldn't > be a big deal. But given it is wrong I do not think that is settled. > and already introduces pain for > others (and likely even more pain when we need to rename it back next > cycle), it doesn't look like a good idea to keep it. Ok, I'll drop it. Best regards, Andreas Hindborg
On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote: > > None of the options are the right choice. That is fine (it is also what I have been arguing in the other thread and in previous times), but that does not imply `into_*` is not a bad choice if we really want to follow upstream. > Cost and ownership _do_ line > up for `into_*` in this case. No, ownership definitely doesn't line up: `Delta` is not `Copy` and there is no conceptual ownership transfer. While it says "owned -> owned", not being `Copy` is quite important here: the guidelines clarify in an example for a `Copy` type that if the input is not consumed then it should not be `into_*`. Sure, "Variable" cost means anything could go there, but that doesn't tell us much, i.e. if it was completely free, we could just as well pick `as_`, which would actually provide some information since you know it needs to be cheap. So the whole argument for `into_*` is... "it says 'Variable' cost so it lines up"? Now, what I argued is that we may just as well define our own rules, since that table is confusing and doesn't cover all cases. If we do that, then you could propose things like "all owned->owned methods are `into_*`", which I think is what you are essentially implying here. > I do not think that is settled. If you think so, then the patch shouldn't be applied. Cheers, Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> None of the options are the right choice. > > That is fine (it is also what I have been arguing in the other thread > and in previous times), but that does not imply `into_*` is not a bad > choice if we really want to follow upstream. > >> Cost and ownership _do_ line >> up for `into_*` in this case. > > No, ownership definitely doesn't line up: `Delta` is not `Copy` and > there is no conceptual ownership transfer. While it says "owned -> > owned", not being `Copy` is quite important here: the guidelines > clarify in an example for a `Copy` type that if the input is not > consumed then it should not be `into_*`. OK, that makes sense. And you are right, `T: Copy` does not line up, I must have read too fast. > > Sure, "Variable" cost means anything could go there, but that doesn't > tell us much, i.e. if it was completely free, we could just as well > pick `as_`, which would actually provide some information since you > know it needs to be cheap. > > So the whole argument for `into_*` is... "it says 'Variable' cost so > it lines up"? You are right, there is no argument outside of "variable cost", thanks for clarifying. > Now, what I argued is that we may just as well define our own rules, > since that table is confusing and doesn't cover all cases. If we do > that, then you could propose things like "all owned->owned methods are > `into_*`", which I think is what you are essentially implying here. I would actually prefer that the rust-lang guidelines were clarified so that we could just defer to those. > >> I do not think that is settled. > > If you think so, then the patch shouldn't be applied. I understand. Best regards, Andreas Hindborg
On Tue, 24 Jun 2025 23:13:49 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Tue, 24 Jun 2025 at 15:11, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> None of the options are the right choice.
>
> That is fine (it is also what I have been arguing in the other thread
> and in previous times), but that does not imply `into_*` is not a bad
> choice if we really want to follow upstream.
>
>> Cost and ownership _do_ line
>> up for `into_*` in this case.
>
> No, ownership definitely doesn't line up: `Delta` is not `Copy` and
> there is no conceptual ownership transfer. While it says "owned ->
> owned", not being `Copy` is quite important here: the guidelines
Just to clarify, Delta implements Copy:
#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
pub struct Delta {
nanos: i64,
}
But it's just i64 so your point is that there is no conceptual
ownership transfer, right?
> clarify in an example for a `Copy` type that if the input is not
> consumed then it should not be `into_*`.
>
> Sure, "Variable" cost means anything could go there, but that doesn't
> tell us much, i.e. if it was completely free, we could just as well
> pick `as_`, which would actually provide some information since you
> know it needs to be cheap.
>
> So the whole argument for `into_*` is... "it says 'Variable' cost so
> it lines up"?
>
> Now, what I argued is that we may just as well define our own rules,
> since that table is confusing and doesn't cover all cases. If we do
> that, then you could propose things like "all owned->owned methods are
> `into_*`", which I think is what you are essentially implying here.
>
>> I do not think that is settled.
>
> If you think so, then the patch shouldn't be applied.
>
> Cheers,
> Miguel
>
On Wed, Jun 25, 2025 at 1:30 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Just to clarify, Delta implements Copy: Sorry, that was a typo: I meant it is `Copy` (the table says `into_*` is for non`-`Copy` types). Cheers, Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Wed, Jun 25, 2025 at 1:30 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> Just to clarify, Delta implements Copy: > > Sorry, that was a typo: I meant it is `Copy` (the table says `into_*` > is for non`-`Copy` types). The confusions will have no end. Best regards, Andreas Hindborg
On Tue, 24 Jun 2025 15:11:31 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote: >> and already introduces pain for >> others (and likely even more pain when we need to rename it back next >> cycle), it doesn't look like a good idea to keep it. > > Ok, I'll drop it. Do you want me to send the updated hrtimer conversion patchset (using as_* names)?
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > On Tue, 24 Jun 2025 15:11:31 +0200 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > >>> and already introduces pain for >>> others (and likely even more pain when we need to rename it back next >>> cycle), it doesn't look like a good idea to keep it. >> >> Ok, I'll drop it. > > Do you want me to send the updated hrtimer conversion patchset > (using as_* names)? No, I am just about finished fixing up the rest. You can check if it is OK when I push. Best regards, Andreas Hindborg
Andreas Hindborg <a.hindborg@kernel.org> writes: > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > >> On Tue, 24 Jun 2025 15:11:31 +0200 >> Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >>>> and already introduces pain for >>>> others (and likely even more pain when we need to rename it back next >>>> cycle), it doesn't look like a good idea to keep it. >>> >>> Ok, I'll drop it. >> >> Do you want me to send the updated hrtimer conversion patchset >> (using as_* names)? > > No, I am just about finished fixing up the rest. You can check if it is > OK when I push. I pushed it, please check. Best regards, Andreas Hindborg
On Tue, 24 Jun 2025 21:03:24 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>>> and already introduces pain for
>>>>> others (and likely even more pain when we need to rename it back next
>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>
>>>> Ok, I'll drop it.
>>>
>>> Do you want me to send the updated hrtimer conversion patchset
>>> (using as_* names)?
>>
>> No, I am just about finished fixing up the rest. You can check if it is
>> OK when I push.
>
> I pushed it, please check.
Thanks!
The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
to Instant structure:
+ #[inline]
+ pub(crate) fn as_nanos(&self) -> i64 {
+ self.inner
+ }
Would it be better to take self instead of &self?
pub(crate) fn as_nanos(self) -> i64 {
Because the as_nanos method on the Delta struct takes self, wouldn’t it
be better to keep it consistent? I think that my original patch adds
into_nanos() that takes self.
This commit also adds HrTimerExpire strait, which as_nanos() method
takes &self:
+/// Time representations that can be used as expiration values in [`HrTimer`].
+pub trait HrTimerExpires {
+ /// Converts the expiration time into a nanosecond representation.
+ ///
+ /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
+ /// timer functions. The interpretation (absolute vs relative) depends on the
+ /// associated [HrTimerMode] in use.
+ fn as_nanos(&self) -> i64;
+}
That's because as I reported, Clippy warns if as_* take self.
As Alice pointed out, Clippy doesn't warn if a type implements
Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
warn about as_nanos method that takes self:
+/// Time representations that can be used as expiration values in [`HrTimer`].
+pub trait HrTimerExpires: Copy {
+ /// Converts the expiration time into a nanosecond representation.
+ ///
+ /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
+ /// timer functions. The interpretation (absolute vs relative) depends on the
+ /// associated [HrTimerMode] in use.
+ fn as_nanos(self) -> i64;
+}
I'm fine with either (taking &self or Adding Copy).
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> On Tue, 24 Jun 2025 21:03:24 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>>>> and already introduces pain for
>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>
>>>>> Ok, I'll drop it.
>>>>
>>>> Do you want me to send the updated hrtimer conversion patchset
>>>> (using as_* names)?
>>>
>>> No, I am just about finished fixing up the rest. You can check if it is
>>> OK when I push.
>>
>> I pushed it, please check.
>
> Thanks!
>
> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
> to Instant structure:
>
> + #[inline]
> + pub(crate) fn as_nanos(&self) -> i64 {
> + self.inner
> + }
>
> Would it be better to take self instead of &self?
>
> pub(crate) fn as_nanos(self) -> i64 {
>
> Because the as_nanos method on the Delta struct takes self, wouldn’t it
> be better to keep it consistent? I think that my original patch adds
> into_nanos() that takes self.
>
> This commit also adds HrTimerExpire strait, which as_nanos() method
> takes &self:
>
> +/// Time representations that can be used as expiration values in [`HrTimer`].
> +pub trait HrTimerExpires {
> + /// Converts the expiration time into a nanosecond representation.
> + ///
> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
> + /// timer functions. The interpretation (absolute vs relative) depends on the
> + /// associated [HrTimerMode] in use.
> + fn as_nanos(&self) -> i64;
> +}
>
> That's because as I reported, Clippy warns if as_* take self.
>
> As Alice pointed out, Clippy doesn't warn if a type implements
> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
> warn about as_nanos method that takes self:
>
> +/// Time representations that can be used as expiration values in [`HrTimer`].
> +pub trait HrTimerExpires: Copy {
> + /// Converts the expiration time into a nanosecond representation.
> + ///
> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
> + /// timer functions. The interpretation (absolute vs relative) depends on the
> + /// associated [HrTimerMode] in use.
> + fn as_nanos(self) -> i64;
> +}
>
> I'm fine with either (taking &self or Adding Copy).
Let's wait for the whole naming discussion to resolve before we do
anything. I am honestly a bit confused as to what is the most idiomatic
resolution here.
I think taking `&self` vs `self` makes not difference in codegen if we
mark the function `#[inline(always)]`.
Best regards,
Andreas Hindborg
On Wed, 25 Jun 2025 10:19:59 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Tue, 24 Jun 2025 21:03:24 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>
>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>
>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>
>>>>>>> and already introduces pain for
>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>
>>>>>> Ok, I'll drop it.
>>>>>
>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>> (using as_* names)?
>>>>
>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>> OK when I push.
>>>
>>> I pushed it, please check.
>>
>> Thanks!
>>
>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>> to Instant structure:
>>
>> + #[inline]
>> + pub(crate) fn as_nanos(&self) -> i64 {
>> + self.inner
>> + }
>>
>> Would it be better to take self instead of &self?
>>
>> pub(crate) fn as_nanos(self) -> i64 {
>>
>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>> be better to keep it consistent? I think that my original patch adds
>> into_nanos() that takes self.
>>
>> This commit also adds HrTimerExpire strait, which as_nanos() method
>> takes &self:
>>
>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>> +pub trait HrTimerExpires {
>> + /// Converts the expiration time into a nanosecond representation.
>> + ///
>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>> + /// associated [HrTimerMode] in use.
>> + fn as_nanos(&self) -> i64;
>> +}
>>
>> That's because as I reported, Clippy warns if as_* take self.
>>
>> As Alice pointed out, Clippy doesn't warn if a type implements
>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>> warn about as_nanos method that takes self:
>>
>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>> +pub trait HrTimerExpires: Copy {
>> + /// Converts the expiration time into a nanosecond representation.
>> + ///
>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>> + /// associated [HrTimerMode] in use.
>> + fn as_nanos(self) -> i64;
>> +}
>>
>> I'm fine with either (taking &self or Adding Copy).
>
> Let's wait for the whole naming discussion to resolve before we do
> anything. I am honestly a bit confused as to what is the most idiomatic
> resolution here.
>
> I think taking `&self` vs `self` makes not difference in codegen if we
> mark the function `#[inline(always)]`.
I believe we've reached a consensus on the discussion about `&self` vs
`self`.
Miguel summarized nicely:
https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/
>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>> this thread that the compiler will be smart about this and just pass the
>> value. But it still feels wrong to me.
>
> If inlined/private, yes; but not always.
>
> So for small ("free") functions like this, it should generally not
> matter, since they should be inlined whether by manual marking or by
> the compiler.
> But, in general, it is not the same, and you can see cases where the
> compiler will still pass a pointer, and thus dereferences and writes
> to memory to take an address to pass it.
>
> Which means that, outside small things like `as_*`, one should still
> probably take by value. Which creates an inconsistency.
I think that another consensus from this discussion is that the table
in the API naming guidelines doesn't cover this particular case.
Therefore, it makes sense to keep the current function name and move
forward.
Delta already provides `fn as_nanos(self)` (and drm uses in
linux-next, as you know) so I believe that Instant should use the same
interface.
That table needs improvement, but reaching consensus will likely take
time, it makes sense to address it independently.
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> On Wed, 25 Jun 2025 10:19:59 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Tue, 24 Jun 2025 21:03:24 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>
>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>
>>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>
>>>>>>>> and already introduces pain for
>>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>>
>>>>>>> Ok, I'll drop it.
>>>>>>
>>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>>> (using as_* names)?
>>>>>
>>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>>> OK when I push.
>>>>
>>>> I pushed it, please check.
>>>
>>> Thanks!
>>>
>>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>>> to Instant structure:
>>>
>>> + #[inline]
>>> + pub(crate) fn as_nanos(&self) -> i64 {
>>> + self.inner
>>> + }
>>>
>>> Would it be better to take self instead of &self?
>>>
>>> pub(crate) fn as_nanos(self) -> i64 {
>>>
>>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>>> be better to keep it consistent? I think that my original patch adds
>>> into_nanos() that takes self.
>>>
>>> This commit also adds HrTimerExpire strait, which as_nanos() method
>>> takes &self:
>>>
>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>> +pub trait HrTimerExpires {
>>> + /// Converts the expiration time into a nanosecond representation.
>>> + ///
>>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>>> + /// associated [HrTimerMode] in use.
>>> + fn as_nanos(&self) -> i64;
>>> +}
>>>
>>> That's because as I reported, Clippy warns if as_* take self.
>>>
>>> As Alice pointed out, Clippy doesn't warn if a type implements
>>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>>> warn about as_nanos method that takes self:
>>>
>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>> +pub trait HrTimerExpires: Copy {
>>> + /// Converts the expiration time into a nanosecond representation.
>>> + ///
>>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>>> + /// associated [HrTimerMode] in use.
>>> + fn as_nanos(self) -> i64;
>>> +}
>>>
>>> I'm fine with either (taking &self or Adding Copy).
>>
>> Let's wait for the whole naming discussion to resolve before we do
>> anything. I am honestly a bit confused as to what is the most idiomatic
>> resolution here.
>>
>> I think taking `&self` vs `self` makes not difference in codegen if we
>> mark the function `#[inline(always)]`.
>
> I believe we've reached a consensus on the discussion about `&self` vs
> `self`.
But not on the function name, right?
>
> Miguel summarized nicely:
>
> https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/
>
>>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>>> this thread that the compiler will be smart about this and just pass the
>>> value. But it still feels wrong to me.
>>
>> If inlined/private, yes; but not always.
>>
>> So for small ("free") functions like this, it should generally not
>> matter, since they should be inlined whether by manual marking or by
>> the compiler.
>
>> But, in general, it is not the same, and you can see cases where the
>> compiler will still pass a pointer, and thus dereferences and writes
>> to memory to take an address to pass it.
>>
>> Which means that, outside small things like `as_*`, one should still
>> probably take by value. Which creates an inconsistency.
>
>
> I think that another consensus from this discussion is that the table
> in the API naming guidelines doesn't cover this particular case.
> Therefore, it makes sense to keep the current function name and move
> forward.
>
> Delta already provides `fn as_nanos(self)` (and drm uses in
> linux-next, as you know) so I believe that Instant should use the same
> interface.
>
>
> That table needs improvement, but reaching consensus will likely take
> time, it makes sense to address it independently.
I am still uncertain what guidelines to follow inside the kernel. Last
time I applied a patch in this situation, I had to remove it again. I
would rather not have to do that.
Perhaps the best way forward is if you send the patch with the naming
and argument type you think is best, and then we continue the discussion
on that patch?
Best regards,
Andreas Hindborg
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Wed, 25 Jun 2025 10:19:59 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Tue, 24 Jun 2025 21:03:24 +0200
>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>>
>>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>>
>>>>>>> On Tue, 24 Jun 2025 15:11:31 +0200
>>>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>>
>>>>>>>>> and already introduces pain for
>>>>>>>>> others (and likely even more pain when we need to rename it back next
>>>>>>>>> cycle), it doesn't look like a good idea to keep it.
>>>>>>>>
>>>>>>>> Ok, I'll drop it.
>>>>>>>
>>>>>>> Do you want me to send the updated hrtimer conversion patchset
>>>>>>> (using as_* names)?
>>>>>>
>>>>>> No, I am just about finished fixing up the rest. You can check if it is
>>>>>> OK when I push.
>>>>>
>>>>> I pushed it, please check.
>>>>
>>>> Thanks!
>>>>
>>>> The commit d9fc00dc7354 ("rust: time: Add HrTimerExpires trait") adds
>>>> to Instant structure:
>>>>
>>>> + #[inline]
>>>> + pub(crate) fn as_nanos(&self) -> i64 {
>>>> + self.inner
>>>> + }
>>>>
>>>> Would it be better to take self instead of &self?
>>>>
>>>> pub(crate) fn as_nanos(self) -> i64 {
>>>>
>>>> Because the as_nanos method on the Delta struct takes self, wouldn’t it
>>>> be better to keep it consistent? I think that my original patch adds
>>>> into_nanos() that takes self.
>>>>
>>>> This commit also adds HrTimerExpire strait, which as_nanos() method
>>>> takes &self:
>>>>
>>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>>> +pub trait HrTimerExpires {
>>>> + /// Converts the expiration time into a nanosecond representation.
>>>> + ///
>>>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>>>> + /// associated [HrTimerMode] in use.
>>>> + fn as_nanos(&self) -> i64;
>>>> +}
>>>>
>>>> That's because as I reported, Clippy warns if as_* take self.
>>>>
>>>> As Alice pointed out, Clippy doesn't warn if a type implements
>>>> Copy. So we can add Copy to HrTimerExpires trait, then Clippy doesn't
>>>> warn about as_nanos method that takes self:
>>>>
>>>> +/// Time representations that can be used as expiration values in [`HrTimer`].
>>>> +pub trait HrTimerExpires: Copy {
>>>> + /// Converts the expiration time into a nanosecond representation.
>>>> + ///
>>>> + /// This value corresponds to a raw ktime_t value, suitable for passing to kernel
>>>> + /// timer functions. The interpretation (absolute vs relative) depends on the
>>>> + /// associated [HrTimerMode] in use.
>>>> + fn as_nanos(self) -> i64;
>>>> +}
>>>>
>>>> I'm fine with either (taking &self or Adding Copy).
>>>
>>> Let's wait for the whole naming discussion to resolve before we do
>>> anything. I am honestly a bit confused as to what is the most idiomatic
>>> resolution here.
>>>
>>> I think taking `&self` vs `self` makes not difference in codegen if we
>>> mark the function `#[inline(always)]`.
>>
>> I believe we've reached a consensus on the discussion about `&self` vs
>> `self`.
>
> But not on the function name, right?
>
>>
>> Miguel summarized nicely:
>>
>> https://lore.kernel.org/lkml/CANiq72nd6m3eOxF+6kscXuVu7uLim4KgpONupgTsMcAF9TNhYQ@mail.gmail.com/
>>
>>>> Yes, I would prefer taking by value. I think Alice mentioned earlier in
>>>> this thread that the compiler will be smart about this and just pass the
>>>> value. But it still feels wrong to me.
>>>
>>> If inlined/private, yes; but not always.
>>>
>>> So for small ("free") functions like this, it should generally not
>>> matter, since they should be inlined whether by manual marking or by
>>> the compiler.
>>
>>> But, in general, it is not the same, and you can see cases where the
>>> compiler will still pass a pointer, and thus dereferences and writes
>>> to memory to take an address to pass it.
>>>
>>> Which means that, outside small things like `as_*`, one should still
>>> probably take by value. Which creates an inconsistency.
>>
>>
>> I think that another consensus from this discussion is that the table
>> in the API naming guidelines doesn't cover this particular case.
>> Therefore, it makes sense to keep the current function name and move
>> forward.
>>
>> Delta already provides `fn as_nanos(self)` (and drm uses in
>> linux-next, as you know) so I believe that Instant should use the same
>> interface.
>>
>>
>> That table needs improvement, but reaching consensus will likely take
>> time, it makes sense to address it independently.
>
> I am still uncertain what guidelines to follow inside the kernel. Last
> time I applied a patch in this situation, I had to remove it again. I
> would rather not have to do that.
>
> Perhaps the best way forward is if you send the patch with the naming
> and argument type you think is best, and then we continue the discussion
> on that patch?
This was discussed [1] and consensus was reached that `as_*` iwth pass
by value plus a `Copy` bound on the trait is the way to go for this
method.
Best regards,
Andreas Hindborg
[1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions
On Thu, 10 Jul 2025 13:59:17 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote: > This was discussed [1] and consensus was reached that `as_*` iwth pass > by value plus a `Copy` bound on the trait is the way to go for this > method. > > [1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions Great, thanks! Would you like me to send a patch for something ― for example, a patch that applies the above changes to the current timekeeping-next tree? Or would you prefer to reset timekeeping-next to an earlier commit for a cleaner history, and have me send a patchset based on that?
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > On Thu, 10 Jul 2025 13:59:17 +0200 > Andreas Hindborg <a.hindborg@kernel.org> wrote: > >> This was discussed [1] and consensus was reached that `as_*` iwth pass >> by value plus a `Copy` bound on the trait is the way to go for this >> method. >> >> [1] https://hackmd.io/ZXXSbxxQRpiWzX61sJFlcg?view#API-Naming-guidelines-for-conversion-functions > > Great, thanks! > > Would you like me to send a patch for something ― for example, a patch > that applies the above changes to the current timekeeping-next tree? > > Or would you prefer to reset timekeeping-next to an earlier commit for > a cleaner history, and have me send a patchset based on that? Yes, we should align the code, so if you can change all the `as_nanos` to take by value, that would be great. I already sent the PR for the 6.17 merge window, so please send a new patch, then we can get it in the next cycle. Best regards, Andreas Hindborg
On Tue, 10 Jun 2025 22:28:18 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> Convert hrtimer to use `Instant` and `Delta`; remove the use of
> `Ktime` from the hrtimer code, which was originally introduced as a
> temporary workaround.
>
> hrtimer uses either an `Instant` or a `Delta` as its expiration value,
> depending on the mode specified at creation time. This patchset
> replaces `HrTimerMode` enum with a trait-based abstraction and
> associates each mode with either an `Instant` or a `Delta`. By
> leveraging Rust's type system, this change enables `HrTimer` to be
> statically associated with a specific `HrTimerMode`, the corresponding
> `Instant` or `Delta`, and a `ClockSource`.
>
> The `impl_has_hr_timer` macro is extended to allow specifying the
> `HrTimerMode`. In the following example, it guarantees that the
> `start()` method for `Foo` only accepts `Instant<Monotonic>`. Using a
> `Delta` or an `Instant` with a different clock source will result in a
> compile-time error:
>
> struct Foo {
> #[pin]
> timer: HrTimer<Self>,
> }
>
> impl_has_hr_timer! {
> impl HasHrTimer<Self> for Foo {
> mode : AbsoluteMode<Monotonic>,
> field : self.timer
> }
> }
>
> This design eliminates runtime mismatches between expires types and
> clock sources, and enables stronger type-level guarantees throughout
> hrtimer.
>
> This patchset can be applied on top of the typed clock sources patchset (v4):
>
> https://lore.kernel.org/lkml/20250610093258.3435874-1-fujita.tomonori@gmail.com/
Andreas, whenever you get a chance, could you create a
timekeeping-next branch and merge the typed clock sources patchset and
this?
That would make it easier for me to work on top of them.
Thanks a lot for the review!
Hi Fujita, "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > Andreas, whenever you get a chance, could you create a > timekeeping-next branch and merge the typed clock sources patchset and > this? > > That would make it easier for me to work on top of them. > > Thanks a lot for the review! I will let you know when I publish timekeeping-next, probably this week. I applied the patches, but they are currently going through my own testing. Best regards, Andreas Hindborg
© 2016 - 2025 Red Hat, Inc.