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

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

[Athira Rajeev]

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.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 0ea8702eb516..9e085813d9d2 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1054,7 +1054,7 @@ u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
 		s64 incr = ktime_to_ns(interval);
 
 		orun = ktime_divns(delta, incr);
-		hrtimer_add_expires_ns(timer, incr * orun);
+		hrtimer_add_expires_ns(timer, ktime_to_ns(delta));
 		if (hrtimer_get_expires_tv64(timer) > now)
 			return orun;
 		/*
-- 
2.27.0

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.