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:

> oh - nevermind I get it but I think you made a mistake andreas, comment below
>
> On Fri, 2025-04-25 at 17:06 -0400, Lyude Paul wrote:
>>
>>
>> Perhaps I will understand this at some point after sending this email, but as
>> I'm writing this I have to admit I'm very confused. This is the first time
>> I've actually looked directly at the hrtimer_forward() source and I have to
>> say this is 100% not what I expected the term "overrun" to mean. Honestly,
>> enough so I'm kind of wondering if overrun is even the right word for the C
>> documentation to be using here.
>>
>> To make sure I'm understanding this right, an overrun is not "how many times
>> we would have executed the timer between now and the new execution time" (e.g.
>> "how many times did our new expiration value overrun the previous expiry
>> interval"). Instead it's actually "if the timer's next execution time is
>> greater than the previous expiry time then the timer will be forwarded by
>> `interval`, but if the timer's execution time is shorter than the previous
>> expiry time then the new execution time will be determined by figuring out if
>> the timer were to execute at `interval` what the closest expiry time at that
>> interval to the previous expiry time would be". Which, I'm afraid to admit
>> doesn't actually make any sense to me and makes me feel like "overrun" is
>> entirely the wrong word to be used here.
>>
>> I'm having a little trouble understanding how I'd really describe this in the
>> documentation because I'm also having a lot of trouble understanding why this
>> behavior is the way it is and why someone would want it to work like this.
>> Should this be something like "Forward the timer to the closest expiry time to
>> the current expiry time that can be reached if the timer were to execute at
>> the given interval"?. Or should I maybe just copy the C documentation as close
>> as possible and just leave this strange behavior as an exercise for the
>> reader?
>
> Yeah I think you misunderstood how the code works. Going to show how the code
> would run through using the last example you gave of:
>
>>   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.
>
> The timer value wouldn't actually be moved forward here and the return value
> would be 0:
>
> u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
> //                                 ^ now+5         ^ 5          ^ 2
> //                                 = 10
> {
> 	u64 orun = 1;
> 	ktime_t delta;
>
> 	//                5  - 10 = -5
> 	delta = ktime_sub(now, hrtimer_get_expires(timer));
>
> 	// -5 < 0 = true
> 	if (delta < 0)
> 		return 0; // 0 overruns, timer executes at the same interval
>
> 	// (we don't execute the rest, so I've ommitted it)
> 	// ...
> }
> EXPORT_SYMBOL_GPL(hrtimer_forward);

Thanks for explaining, that makes a lot more sense 😅 I think I flipped the sign
of `delta`.

However, I still think the documentation is not correct. How is this instead:

  Conditionally forward the timer.

  If the timer expires after `now`, this function does
  nothing and returns 0.

  If the timer expired at or before `now`, this function forwards the timer by `interval`
  until the timer expires after `now` and then returns the number of times
  the timer was forwarded by `interval`.

  This is mainly useful for timer types etc etc ...

Best regards,
Andreas Hindborg