Re: [PATCH v2 3/8] rust: hrtimer: Add HrTimerCallbackContext and ::forward()

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

Lyude Paul <lyude@redhat.com> writes:

> With Linux's hrtimer API, there's a number of methods that can only be
> called in two situations:
>
> * When we have exclusive access to the hrtimer and it is not currently
>   active
> * When we're within the context of an hrtimer callback context
>
> This commit handles the second situation and implements hrtimer_forward()
> support in the context of a timer callback. We do this by introducing a
> HrTimerCallbackContext type which is provided to users during the
> RawHrTimerCallback::run() callback, and then add a forward() function to
> the type.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
>
> ---
>
> V2:
> * Improve SAFETY comments for HrTimerCallbackContext uses (I forgot to
>   mention that we're within RawHrTimerCallback::run()
> * Split forward into forward() and raw_forward() since we're going to have
>   two contexts that we can call forward() from now.
> * Clarify contexts in which certain hrtimer methods can be called.
> * Make sure that we use a mutable reference for forward() here - just in
>   case :).
> * Rename interval to duration
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  rust/kernel/time/hrtimer.rs         | 51 +++++++++++++++++++++++++++--
>  rust/kernel/time/hrtimer/arc.rs     |  9 ++++-
>  rust/kernel/time/hrtimer/pin.rs     |  9 ++++-
>  rust/kernel/time/hrtimer/pin_mut.rs | 11 +++++--
>  rust/kernel/time/hrtimer/tbox.rs    |  9 ++++-
>  5 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index aadae8666f7ea..507fff67f8ab2 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -73,7 +73,7 @@
>      time::{Delta, Instant},
>      types::Opaque,
>  };
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, ptr::NonNull};
>  use pin_init::PinInit;
>  
>  /// A timer backed by a C `struct hrtimer`.
> @@ -314,7 +314,10 @@ pub trait HrTimerCallback {
>      type Pointer<'a>: RawHrTimerCallback;
>  
>      /// Called by the timer logic when the timer fires.
> -    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>) -> HrTimerRestart
> +    fn run<T>(
> +        this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>,
> +        ctx: HrTimerCallbackContext<'_, T>,
> +    ) -> HrTimerRestart
>      where
>          Self: Sized;
>  }
> @@ -507,6 +510,50 @@ fn into_c(self) -> bindings::hrtimer_mode {
>      }
>  }
>  
> +/// Privileged smart-pointer for a [`HrTimer`] callback context.
> +///
> +/// Many [`HrTimer`] methods can only be called in two situations:
> +///
> +/// * When the caller has exclusive access to the `HrTimer` and the `HrTimer` is guaranteed not to
> +///   be running.
> +/// * From within the context of an `HrTimer`'s callback method.
> +///
> +/// This type provides access to said methods from within a timer callback context.
> +///
> +/// # Invariants
> +///
> +/// * The existence of this type means the caller is currently within the callback for an
> +///   [`HrTimer`].
> +/// * `self.0` always points to a live instance of [`HrTimer<T>`].
> +pub struct HrTimerCallbackContext<'a, T>(NonNull<HrTimer<T>>, PhantomData<&'a ()>);
> +
> +impl<'a, T> HrTimerCallbackContext<'a, T> {
> +    /// Create a new [`HrTimerCallbackContext`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function relies on the caller being within the context of a timer callback, so it must
> +    /// not be used anywhere except for within implementations of [`RawHrTimerCallback::run`]. The
> +    /// caller promises that `timer` points to a valid initialized instance of
> +    /// [`bindings::hrtimer`].
> +    pub(crate) unsafe fn from_raw(timer: *mut HrTimer<T>) -> Self {
> +        // SAFETY: The caller guarantees `timer` is a valid pointer to an initialized
> +        // `bindings::hrtimer`
> +        Self(unsafe { NonNull::new_unchecked(timer) }, PhantomData)
> +    }
> +
> +    /// Forward the timer expiry so it expires at `duration` after `now`.
> +    ///
> +    /// Note that this does not requeue the timer, it simply updates its expiry value. It returns
> +    /// the number of overruns that have occurred as a result of the expiry change.
> +    pub fn forward(&mut self, now: Instant, duration: Delta) -> u64 {
> +        // SAFETY:
> +        // - We are guaranteed to be within the context of a timer callback by our type invariants
> +        // - By our type invariants, `self.0` always points to a valid `HrTimer<T>`
> +        unsafe { HrTimer::<T>::raw_forward(self.0.as_ptr(), now, duration) }

Safety comment do not match requirements for `raw_forward`. We should
require that either we are in timer context, or the timer is stopped and
we have exclusive access.


Best regards,
Andreas Hindborg