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

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: <87eczludtp.fsf@kernel.org>
Date: Wed, 26 Feb 2025 12:48:18 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Tamir Duberstein" <tamird@...il.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "Frederic Weisbecker" <frederic@...nel.org>,
  "Thomas Gleixner" <tglx@...utronix.de>,  "Danilo Krummrich"
 <dakr@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno Lossin" <benno.lossin@...ton.me>,
  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Lyude Paul" <lyude@...hat.com>,  "Guangbo Cui" <2407018371@...com>,
  "Dirk Behme" <dirk.behme@...il.com>,  "Daniel Almeida"
 <daniel.almeida@...labora.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 01/13] rust: hrtimer: introduce hrtimer support

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

> On Tue, Feb 25, 2025 at 2:12 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@...il.com> writes:
>>
>> > On Tue, Feb 25, 2025 at 3:52 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>> >>
>> >> "Tamir Duberstein" <tamird@...il.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