Re: [PATCH v3 07/13] rust: hrtimer: implement `UnsafeTimerPointer` for `Pin<&T>`

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

"Lyude Paul" <lyude@redhat.com> writes:

> On Thu, 2024-10-17 at 15:04 +0200, Andreas Hindborg wrote:
>> Allow pinned references to structs that contain a `Timer` node to be
>> scheduled with the `hrtimer` subsystem.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/hrtimer.rs     |  1 +
>>  rust/kernel/hrtimer/pin.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 98 insertions(+)
>>
>> diff --git a/rust/kernel/hrtimer.rs b/rust/kernel/hrtimer.rs
>> index e97d7b8ec63ce6c9ac3fe9522192a28fba78b8ba..ceedf330a803ec2db7ff6c25713ae48e2fd1f4ca 100644
>> --- a/rust/kernel/hrtimer.rs
>> +++ b/rust/kernel/hrtimer.rs
>> @@ -362,3 +362,4 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
>>  }
>>
>>  mod arc;
>> +mod pin;
>> diff --git a/rust/kernel/hrtimer/pin.rs b/rust/kernel/hrtimer/pin.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..a2c1dbd5e48b668cc3dc540c5fd5514f5331d968
>> --- /dev/null
>> +++ b/rust/kernel/hrtimer/pin.rs
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +use super::HasTimer;
>> +use super::RawTimerCallback;
>> +use super::Timer;
>> +use super::TimerCallback;
>> +use super::TimerHandle;
>> +use super::UnsafeTimerPointer;
>> +use crate::time::Ktime;
>> +use core::pin::Pin;
>> +
>> +/// A handle for a `Pin<&HasTimer>`. When the handle exists, the timer might be
>> +/// running.
>> +pub struct PinTimerHandle<'a, U>
>> +where
>> +    U: HasTimer<U>,
>> +{
>> +    pub(crate) inner: Pin<&'a U>,
>> +}
>> +
>> +// SAFETY: We cancel the timer when the handle is dropped. The implementation of
>> +// the `cancel` method will block if the timer handler is running.
>> +unsafe impl<'a, U> TimerHandle for PinTimerHandle<'a, U>
>> +where
>> +    U: HasTimer<U>,
>> +{
>> +    fn cancel(&mut self) -> bool {
>> +        let self_ptr = self.inner.get_ref() as *const U;
>> +
>> +        // SAFETY: As we got `self_ptr` from a reference above, it must point to
>> +        // a valid `U`.
>> +        let timer_ptr = unsafe { <U as HasTimer<U>>::raw_get_timer(self_ptr) };
>> +
>> +        // SAFETY: As `timer_ptr` is derived from a reference, it must point to
>> +        // a valid and initialized `Timer`.
>> +        unsafe { Timer::<U>::raw_cancel(timer_ptr) }
>> +    }
>> +}
>> +
>> +impl<'a, U> Drop for PinTimerHandle<'a, U>
>> +where
>> +    U: HasTimer<U>,
>> +{
>> +    fn drop(&mut self) {
>> +        self.cancel();
>> +    }
>> +}
>> +
>> +// SAFETY: We capture the lifetime of `Self` when we create a `PinTimerHandle`,
>> +// so `Self` will outlive the handle.
>> +unsafe impl<'a, U> UnsafeTimerPointer for Pin<&'a U>
>> +where
>> +    U: Send + Sync,
>> +    U: HasTimer<U>,
>> +    U: TimerCallback<CallbackTarget<'a> = Self>,
>> +{
>> +    type TimerHandle = PinTimerHandle<'a, U>;
>> +
>> +    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
>> +        use core::ops::Deref;
>
> I'm sure this is valid but this seems like a strange place to put a module use
> (also - do we ever actually need to import Deref explicitly? It should always
> be imported)

`core::ops::Deref` is not in scope. So if we want to use
`Deref::deref()`, we must import the trait first.

My first intuition for writing this expression was:

 `(*self) as *const U;`

because `*self` should invoke `Deref::deref()`, right?

But the compiler does not do what I thought it would do. I am not sure
why it does not work. It thinks the result of `(*self)` is not a
reference, but a value expression:

> error[E0605]: non-primitive cast: `U` as `*const U`
>   --> /home/aeh/src/linux-rust/hrtimer-v4-wip/rust/kernel/hrtimer/pin.rs:62:24
>    |
> 62 |         let self_ptr = (*self) as *const U;
>    |                        ^^^^^^^^^^^^^^^^^^^ invalid cast
>    |
> help: consider borrowing the value
>    |
> 62 |         let self_ptr = &(*self) as *const U;
>    |                        +


Another option to consider is:

 `<Self as core::ops::Deref>::deref(&self) as *const U;`

That is also fine for me. Which one do you like better?


Best regards,
Andreas Hindborg