Commit 4da9152a authored by Thomas Gleixner's avatar Thomas Gleixner

timers: Lock base for same bucket optimization

Linus stumbled over the unlocked modification of the timer expiry value in
mod_timer() which is an optimization for timers which stay in the same
bucket - due to the bucket granularity - despite their expiry time getting
updated.

The optimization itself still makes sense even if we take the lock, because
in case that the bucket stays the same, we avoid the pointless
queue/enqueue dance.

Make the check and the modification of timer->expires protected by the base
lock and shuffle the remaining code around so we can keep the lock held
when we actually have to requeue the timer to a different bucket.

Fixes: f00c0afd ("timers: Implement optimization for same expiry time in mod_timer()")
Reported-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1610241711220.4983@nanos
Cc: stable@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
parent b831275a
...@@ -971,6 +971,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) ...@@ -971,6 +971,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
unsigned long clk = 0, flags; unsigned long clk = 0, flags;
int ret = 0; int ret = 0;
BUG_ON(!timer->function);
/* /*
* This is a common optimization triggered by the networking code - if * This is a common optimization triggered by the networking code - if
* the timer is re-modified to have the same timeout or ends up in the * the timer is re-modified to have the same timeout or ends up in the
...@@ -979,13 +981,16 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) ...@@ -979,13 +981,16 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
if (timer_pending(timer)) { if (timer_pending(timer)) {
if (timer->expires == expires) if (timer->expires == expires)
return 1; return 1;
/* /*
* Take the current timer_jiffies of base, but without holding * We lock timer base and calculate the bucket index right
* the lock! * here. If the timer ends up in the same bucket, then we
* just update the expiry time and avoid the whole
* dequeue/enqueue dance.
*/ */
base = get_timer_base(timer->flags); base = lock_timer_base(timer, &flags);
clk = base->clk;
clk = base->clk;
idx = calc_wheel_index(expires, clk); idx = calc_wheel_index(expires, clk);
/* /*
...@@ -995,14 +1000,14 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) ...@@ -995,14 +1000,14 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
*/ */
if (idx == timer_get_idx(timer)) { if (idx == timer_get_idx(timer)) {
timer->expires = expires; timer->expires = expires;
return 1; ret = 1;
goto out_unlock;
} }
} else {
base = lock_timer_base(timer, &flags);
} }
timer_stats_timer_set_start_info(timer); timer_stats_timer_set_start_info(timer);
BUG_ON(!timer->function);
base = lock_timer_base(timer, &flags);
ret = detach_if_pending(timer, base, false); ret = detach_if_pending(timer, base, false);
if (!ret && pending_only) if (!ret && pending_only)
...@@ -1035,9 +1040,10 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) ...@@ -1035,9 +1040,10 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
timer->expires = expires; timer->expires = expires;
/* /*
* If 'idx' was calculated above and the base time did not advance * If 'idx' was calculated above and the base time did not advance
* between calculating 'idx' and taking the lock, only enqueue_timer() * between calculating 'idx' and possibly switching the base, only
* and trigger_dyntick_cpu() is required. Otherwise we need to * enqueue_timer() and trigger_dyntick_cpu() is required. Otherwise
* (re)calculate the wheel index via internal_add_timer(). * we need to (re)calculate the wheel index via
* internal_add_timer().
*/ */
if (idx != UINT_MAX && clk == base->clk) { if (idx != UINT_MAX && clk == base->clk) {
enqueue_timer(base, timer, idx); enqueue_timer(base, timer, idx);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment