Re: [RFC] hrtimer: Fix the hrtimer_forward to use correct delta for expiry time

[Peter Zijlstra]

On Thu, Sep 30, 2021 at 06:22:19PM +0530, Athira Rajeev wrote:
> Hrtimer uses "hrtimer_forward" function to forward the timer
> expiry. This function takes the "time" from when to forward
> and how much "interval" to forward as inputs. In some cases, it
> is observed that though correct interval is given to forward the
> timer, the expiry time is set to value less than expected interval.
> This will cause the timer to expire ahead of expected expiry time.
> Before updating the timer expiry value, hrtimer_forward checks to
> see if there is any delta between the time from when to forwad
> and previously set expiry. And this behaviiour is observed when
> delta is large value.
> 
> For example, consider case where we want to forward the expiry
> from "now" to 10 milliseconds. Below is trace prints captured by
> dumping the values used in "hrtimer_forward". And this instance is
> captured while forwarding timer from perf event multiplexing code
> which uses hrtimer.
> 
> <<>>
> [001] d....   304.118944: perf_mux_hrtimer_restart: Restarting timer from perf_mux_hrtimer_restart
> [001] d....   304.118945: hrtimer_forward: In nanoseconds, now is 303938589749, delta is 52868589749, hrtimer_get_expires(timer) is 251070000000, interval is 10000000
> [001] d....   304.118945: hrtimer_forward: Caller is perf_mux_hrtimer_restart+0xb0/0x120
> [001] d....   304.118946: hrtimer_forward: Caller's caller is merge_sched_in+0x268/0x4e0
> [001] d....   304.118946: hrtimer_forward: orun is 5286
> [001] d....   304.118947: hrtimer_forward: In hrtimer_add_expires_ns, before ktime_add_ns timer->node.expires in ns is 251070000000, timer->_softexpires is 251070000000,  ns is 52860000000
> [001] d....   304.118948: hrtimer_forward: In hrtimer_add_expires_ns, after ktime_add_ns timer->node.expires in ns is 303930000000, timer->_softexpires is 303930000000
> [001] d....   304.118948: hrtimer_forward: In hrtimer_add_expires, in nanoseconds, before ktime_add_safe, timer->node.expires in ns is 303930000000, timer->_softexpires is 303930000000
> [001] d....   304.118949: hrtimer_forward: In hrtimer_add_expires, in nanoseconds, after ktime_add_safe, timer->node.expires in ns is 303940000000, timer->_softexpires is 303940000000
> [001] d....   304.118949: hrtimer_forward: After hrtimer_add_expires, hrtimer_get_remaining in nanoseconds is 1405169
> 
> <<>>
> 
> In this example,
> timer->node.expires = 251070000000 ns ( previously set expiry )
> now = 303938589749 ns
> delta = 52868589749 ns
> 
> Here delta is "52868589749" which is greater than the interval to
> forward ( ie 10000000 ns ). Hence hrtimer_forwards adds this difference
> to present timer->node.expires in hrtimer_add_expires_ns() function. But
> instead of using actual "delta", code is using (delta/interval * interval)
> as below:
> 
> <<>>
> s64 incr = ktime_to_ns(interval);
> orun = ktime_divns(delta, incr);
> hrtimer_add_expires_ns(timer, incr * orun);
> <<>>
> 
> Hence we are actually using "orun * 10000000". In this example, it will be
> "52860000000" since "orun" does not include the mod value. Here we are
> missing "8589749" nanoseconds in the timer expiry. Hence, the final expiry
> time will be set to "303940000000".
> 
> Since we are not using exact delta, the hrtime remaining will be
> around 1405169 nanoseconds and will cause the timer to expire in 1.4 ms
> instead of 10 ms. Fix this by using "delta" instead of using the divided
> value.
> 

You misunderstand, the behaviour is correct and expected. What
hrtimer_forward_now() does is guarantee the expiry time ends up on an
integer multiple of @interval.

This is important to achieve periodic timers. Your patch would cause
period drift.