Re: [PATCH v3 0/5] rust: time: Convert hrtimer to use Instant and Delta

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

"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