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 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>> > "Tamir Duberstein" <tamird@gmail.com> writes:
>> >
>> >> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>>
>> >>> "Tamir Duberstein" <tamird@gmail.com> writes:
>> >>>
>> >>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> [...]
>>
>> >>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> >>> >> +    ///
>> >>> >> +    /// # Safety
>> >>> >> +    ///
>> >>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>> >>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> >>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> >>> >> +        // because the caller of this function promises that `ptr` points to an
>> >>> >> +        // allocation of at least the size of `Self`.
>> >>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> >>> >> +    }
>> >>> >
>> >>> > Can you help me understand why the various functions here operate on
>> >>> > *const Self? I understand the need to obtain a C pointer to interact
>> >>> > with bindings, but I don't understand why we're dealing in raw
>> >>> > pointers to the abstraction rather than references.
>> >>>
>> >>> We cannot reference the `bindings::hrtimer` without wrapping it in
>> >>> `Opaque`. This would be the primary reason. At other times, we cannot
>> >>> produce references because we might not be able to prove that we satisfy
>> >>> the safety requirements for turning a pointer into a reference. If we
>> >>> are just doing offset arithmetic anyway, we don't need a reference.
>> >>
>> >> Why do we have a pointer, rather than a reference, to Self in the
>> >> first place? I think this is the key thing I don't understand.
>> >
>> > Perhaps it makes more sense if you look at the context. One of the entry
>> > points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
>> > HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
>> > handle contains an arc to a type that contains a `Timer` and implements
>> > `HasHrTImer`. To get to the timer, we need to do pointer manipulation.
>> > We only know how to get the `Timer` field via the `OFFSET`. The natural
>> > return value from the offset operation is a raw pointer. Rather than
>> > convert back to a reference, we stay in pointer land when we call
>> > `HrTimer::raw_cancel`, because we need a pointer to the
>> > `bindings::hrtimer` anyway, not a reference.
>>
>> I changed `HasHrTimer::start` to take a reference, and I think that
>> makes sense 👍 Taking an `impl AsRef` does not work out when `Self` is
>> `Pin<&T>`. I'll go over the whole thing and see of other places could
>> benefit.
>
> Can you elaborate please? Pin<&T>::get_ref returns &T, which gets you
> to the AsRef you need, no?

Right, that works. But if I pass in &self it's not really needed. You
can see for yourself in next version.

Hopefully I did not break any provenance rules.


Best regards,
Andreas Hindborg