Re: [PATCH v9 01/13] rust: hrtimer: introduce hrtimer support

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

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Tue, Feb 25, 2025 at 2:12 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Tue, Feb 25, 2025 at 3:52 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> "Tamir Duberstein" <tamird@gmail.com> writes:
>> >>
>> >> > Hi Andreas, mostly grammar and prose clarity comments below.
>> >> >
>> >> > I still think HasHrTimer::OFFSET is less clear and more fragile than
>> >> > just generating compiler-checked implementations in the macro (you're
>> >> > already generating OFFSET and one method implementation rather than
>> >> > generating 2 method implementations).
>> >>
>> >> I don't agree with you assessment. My argument is that I would rather
>> >> generate as little code as possible in the macro, and the trait would in
>> >> practice never be implemented by hand.
>> >
>> > In the current patch, the trait:
>> > - provides raw_get_timer
>> > - provides timer_container_of
>> > and the macro:
>> > - defines OFFSET
>> > - defines raw_get_timer
>> >
>> > The justification for the redundancy is that without defining
>> > raw_get_timer in the macro the user might invoke the macro
>> > incorrectly.
>>
>> It's not that they might invoke the macro incorrectly, it's that we
>> would not be able to make the macro safe. The way it is implemented now,
>> it will only compile if it is safe.
>>
>> > But why is that better than defining both methods in the
>> > macro?
>>
>> Because it is generating less code. I would rather write the library code than
>> have the macro generate the code for us on every invocation.
>
> How is it less code? It's the same amount, just harder to reason about
> because you're doing pointer arithmetic rather than relying on
> existing macros like container_of.
>
>>
>> > Either way the macro provides 2 items. The key benefit of
>> > defining both methods in the macro is that there's no dead-code
>> > implementation of raw_get_pointer in the trait. It also reduces the
>> > surface of the trait, which is always a benefit due to Hyrum's law.
>>
>> When you say that the surface would be smaller, you mean that by
>> dropping OFFSET entirely, the trait would have fewer items?
>
> Yes.
>
>
>> I'm not familiar with Hyrum's law.
>
> TL;DR is that anything that can become load bearing will. So even if
> the intent is that OFFSET is an implementation detail, there's no way
> to enforce that, and so someone will misuse it.

I don't fully agree with your assessment, but either way is fine for me.
So I shall implement your suggestion.

[...]

>> > I noticed below I had suggested talking about the handler as
>> > "returning" rather than "finishing execution"; please consider that
>> > throughout.
>>
>> I do not prefer one over the other. Do you care strongly about this one?
>
> I prefer return since it's more obvious but don't feel strongly about
> the choice, only that the usage is consistent.

Ok, let's do that then.

[...]

>> >> >> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
>> >
>> > This comment says "embed a [`HrTimer`]" but in `trait HrTimer` the
>> > wording is "Implemented by structs that contain timer nodes."
>>
>> I don't follow. There is no `trait HrTimer`, there is a `struct
>> HrTimer`, but it has no such wording.
>>
>> > Is the difference significant?
>>
>> No, I would say they are semantically the same. Whether a struct
>> contains a field of a type or it embeds another struct - I would say
>> that is the same.
>
> Can we use the same wording in both places then?

OK.

>> > Also the naming of the two traits feels inconsistent; one contains
>> > "Has" and the other doesn't.
>>
>> One is not a trait, not sure if you are looking on another item than
>> `struct HrTimer`?
>
> Sorry, I meant HasHrTimer and HrTimerPointer rather than HrTimer and
> HrTimerPointer.

`HasHrTimer` is named so because it is meant to be implemented by types
that contain a field of type `HrTimer`.

`HrTimerPointer` is meant to be implemented by pointer types that point
to types that implement `HasHrTimer`.

They are different, and the naming reflect that.

I will not rename `HasHrTimer` to `ContainsHrTimer`, because the rest of
the rust kernel uses the `HasFoo` naming scheme.


Best regards,
Andreas Hindborg