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

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

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