Re: [PATCH v2 2/8] rust: hrtimer: Add HrTimer::raw_forward() and forward()

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

Lyude Paul <lyude@redhat.com> writes:

> Within the hrtimer API there are quite a number of functions that can only
> be safely called from one of two contexts:
>
> * When we have exclusive access to the hrtimer and the timer is not active.
> * When we're within the hrtimer's callback context as it is being executed.
>
> This commit adds bindings for hrtimer_forward() for the first such context,
> along with HrTimer::raw_forward() for later use in implementing the
> hrtimer_forward() in the latter context.
>
> Since we can only retrieve a &mut reference to an HrTimer<T> in contexts
> where it is not possible for the timer to be accessed by others or
> currently executing (e.g. a UniqueArc), a &mut is actually enough of a
> guarantee to safely fulfill the C API requirements here.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  rust/kernel/time/hrtimer.rs | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index bfe0e25f5abd0..aadae8666f7ea 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -68,7 +68,11 @@
>  //! `start` operation.
>  
>  use super::ClockId;
> -use crate::{prelude::*, time::Instant, types::Opaque};
> +use crate::{
> +    prelude::*,
> +    time::{Delta, Instant},
> +    types::Opaque,
> +};
>  use core::marker::PhantomData;
>  use pin_init::PinInit;
>  
> @@ -164,6 +168,36 @@ pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
>          // handled on the C side.
>          unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
>      }
> +
> +    /// Forward the timer expiry for a given timer pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn raw_forward(self_ptr: *mut Self, now: Instant, interval: Delta) -> u64 {
> +        // SAFETY:
> +        // * The C API requirements for this function are fulfilled by our safety contract.
> +        // * `self_ptr` is guaranteed to point to a valid `Self` via our safety contract
> +        unsafe {
> +            bindings::hrtimer_forward(Self::raw_get(self_ptr), now.as_nanos(), interval.as_nanos())
> +        }
> +    }
> +
> +    /// Forward the timer expiry so it expires at `duration` after `now`.
> +    ///
> +    /// This is mainly useful for timer types that can start off providing a mutable reference (e.g.
> +    /// `Pin<Box<…>>`) before the timer is started.
> +    ///
> +    /// 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.

Looking at C `hrtimer_forward`, I don't think the description is
correct:

    u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
    {
      u64 orun = 1;
      ktime_t delta;

      delta = ktime_sub(now, hrtimer_get_expires(timer));

      if (delta < 0)
        return 0;

      if (WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED))
        return 0;

      if (interval < hrtimer_resolution)
        interval = hrtimer_resolution;

      if (unlikely(delta >= interval)) {
        s64 incr = ktime_to_ns(interval);

        orun = ktime_divns(delta, incr);
        hrtimer_add_expires_ns(timer, incr * orun);
        if (hrtimer_get_expires_tv64(timer) > now)
          return orun;
        /*
        * This (and the ktime_add() below) is the
        * correction for exact:
        */
        orun++;
      }
      hrtimer_add_expires(timer, interval);

      return orun;
    }

As I read the code:

  If the timer expires 2s after `now` and `interval` is 6s, then the new expiry
  time is moved 6s forward. Not to 6s after `now`. Return value will be 0.

  If the timer expires 3s after `now` and `interval` is 2s, then the
  expiry time is moved 2s forward and the return value is 1.

  If the timer expires 4s after `now` and `interval` is 2s, then the
  expiry time is moved 4s forward and the return value is 2.

  If the timer expires 5s after `now` and `interval` is 2s, then the
  expiry time is moved 4s forward and the return value is 2.

Can you capture this behavior in the docs?


Best regards,
Andreas Hindborg