Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

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

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

> On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> [...]
>>
>> >> >> +pub unsafe trait HrTimerHandle {
>> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
>> >> >> +    /// till the handler has finished.
>> >> >> +    fn cancel(&mut self) -> bool;
>> >> >> +}
>> >> >> +
>> >> >> +/// Implemented by structs that contain timer nodes.
>> >> >> +///
>> >> >> +/// Clients of the timer API would usually safely implement this trait by using
>> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
>> >> >> +///
>> >> >> +/// # Safety
>> >> >> +///
>> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
>> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
>> >> >> +/// implemented according to their documentation.
>> >> >> +///
>> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
>> >> >> +pub unsafe trait HasHrTimer<T> {
>> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
>> >> >> +    const OFFSET: usize;
>> >> >
>> >> > Does this need to be part of the trait? As an alternative the provided
>> >> > methods could be generated in the macro below and reduce the
>> >> > opportunity to implement this trait incorrectly.
>> >>
>> >> There is no risk of implementing the trait wrong, because it is usually
>> >> derived by a macro.
>> >
>> > There's no risk when it's implemented by the macro, but you used the
>> > word usually,  which means there is a risk.
>> >
>> >> We need at least one of the methods to be able to have the type system
>> >> verify that the type for which we implement `HasHrTImer` actually has a
>> >> field with the name we specify, and that this field has the right type.
>> >> And to have that, we need the OFFSET.
>> >
>> > I don't follow this logic. OFFSET is calculated in the body of the
>> > macro. I'm suggesting that the macro generate the method
>> > implementations (which would no longer be provided). In effect I'm
>> > saying: keep OFFSET private.
>> >
>> > I'm also noticing now that the macro generates an implementation of
>> > raw_get_timer *in addition to* the provided implementation. Why are
>> > both needed?
>>
>> HasHrTimer is unsafe, because it would be unsound to implement, if the
>> type it is implemented on does not have a `Timer` at the specified
>> offset.
>>
>> To be able to implement it safely with a macro, the macro must verify
>> that the type we implement the trait on satisfies the safety
>> requirement. That is, we have to have the macro verify that the type
>> indeed has a field of type `Timer` with the given name. If that is the
>> case, the macro can calculate OFFSET.
>>
>> The way we achieve this is we re-implement on of the trait methods in
>> such a way that it only compiles if the type we reimplement trait
>> on actually have the field of the right type.
>>
>> I want to generate as little code as possible in the macro, and I would
>> rather rely on the default implementations given in the trait, than have
>> the macro generate implementations for all the methods. Generated code
>> are more difficult to reason about.
>
> Again, I don't follow. The provided implementation of raw_get_timer is
> either not used (in the presence of the macro) or it relies on the
> implementer correctly setting OFFSET, which the compiler cannot check
> and which can break at a distance.
>
> Wouldn't it be simpler to just generate both functions that rely on
> OFFSET? They're both one-liners that delegate to other existing
> macros.

No, I would rather generate as little code as possible. The only reason
I am generating `raw_get_timer` is to be able to type check that the
field name given to the macro has the right type.


Best regards,
Andreas Hindborg