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

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

"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.


Best regards,
Andreas Hindborg