[Devel] [PATCH rh7] ms/hrtimer: Allow concurrent hrtimer_start() for self restarting timers

[Kirill Tkhai]

On 24.09.2018 15:07, Andrey Ryabinin wrote:
> From: Peter Zijlstra <peterz at infradead.org>
> 
> Because we drop cpu_base->lock around calling hrtimer::function, it is
> possible for hrtimer_start() to come in between and enqueue the timer.
> 
> If hrtimer::function then returns HRTIMER_RESTART we'll hit the BUG_ON
> because HRTIMER_STATE_ENQUEUED will be set.
> 
> Since the above is a perfectly valid scenario, remove the BUG_ON and
> make the enqueue_hrtimer() call conditional on the timer not being
> enqueued already.
> 
> NOTE: in that concurrent scenario its entirely common for both sites
> to want to modify the hrtimer, since hrtimers don't provide
> serialization themselves be sure to provide some such that the
> hrtimer::function and the hrtimer_start() caller don't both try and
> fudge the expiration state at the same time.
> 
> To that effect, add a WARN when someone tries to forward an already
> enqueued timer, the most common way to change the expiry of self
> restarting timers. Ideally we'd put the WARN in everything modifying
> the expiry but most of that is inlines and we don't need the bloat.
> 
> Fixes: 2d44ae4d7135 ("hrtimer: clean up cpu->base locking tricks")
> Signed-off-by: Peter Zijlstra (Intel) <peterz at infradead.org>
> Cc: Ben Segall <bsegall at google.com>
> Cc: Roman Gushchin <klamm at yandex-team.ru>
> Cc: Paul Turner <pjt at google.com>
> Link: http://lkml.kernel.org/r/20150415113105.GT5029@twins.programming.kicks-ass.net
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> 
> https://jira.sw.ru/browse/PSBM-88818
> (cherry picked from commit 5de2755c8c8b3a6b8414870e2c284914a2b42e4d)
> Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>

> ---
>  kernel/hrtimer.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index 70c7747aa61a..30bf706bc211 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -843,6 +843,9 @@ u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
>  	if (delta.tv64 < 0)
>  		return 0;
>  
> +	if (WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED))
> +		return 0;
> +
>  	if (interval.tv64 < timer->base->resolution.tv64)
>  		interval.tv64 = timer->base->resolution.tv64;
>  
> @@ -1222,6 +1225,10 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>  	 * Because we run timers from hardirq context, there is no chance
>  	 * they get migrated to another cpu, therefore its safe to unlock
>  	 * the timer base.
> +	 *
> +	 * Note: Because we dropped the cpu_base->lock above,
> +	 * hrtimer_start_range_ns() can have popped in and enqueued the timer
> +	 * for us already
>  	 */
>  	raw_spin_unlock(&cpu_base->lock);
>  	trace_hrtimer_expire_entry(timer, now);
> @@ -1234,10 +1241,9 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>  	 * we do not reprogramm the event hardware. Happens either in
>  	 * hrtimer_start_range_ns() or in hrtimer_interrupt()
>  	 */
> -	if (restart != HRTIMER_NORESTART) {
> -		BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
> +	if (restart != HRTIMER_NORESTART &&
> +		!(timer->state & HRTIMER_STATE_ENQUEUED))
>  		enqueue_hrtimer(timer, base);
> -	}
>  
>  	WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
>  
>