Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the timer

[Kirill Tkhai <tkhai@yandex.ru>]

В Ср, 03/06/2015 в 23:13 +0200, Peter Zijlstra пишет:
On Wed, Jun 03, 2015 at 07:26:00PM +0300, Kirill Tkhai wrote:
> > > @@ -402,7 +394,8 @@ extern u64 hrtimer_get_next_event(void);
> > >   */
> > >  static inline int hrtimer_active(const struct hrtimer *timer)
> > >  {
> > > - return timer->state != HRTIMER_STATE_INACTIVE;
> > > + return timer->state != HRTIMER_STATE_INACTIVE ||
> > > + timer->base->running == timer;
> > >  }
> > 
> > It seems to be not good, because hrtimer_active() check stops
> > to be atomic. So the things like hrtimer_try_to_cancel() race
> > with a callback of self-rearming timer and may return a false
> > positive result.
> 
> Hurm.. the below isn't really pretty either I suppose. The best I can
> say about it is that's its not too expensive on x86.
> 
> I should probably go sleep..
> 
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -391,11 +391,25 @@ extern u64 hrtimer_get_next_event(void);
>   * A timer is active, when it is enqueued into the rbtree or the
>   * callback function is running or it's in the state of being migrated
>   * to another cpu.
> + *
> + * See __run_hrtimer().
>   */
> -static inline int hrtimer_active(const struct hrtimer *timer)
> +static inline bool hrtimer_active(const struct hrtimer *timer)
>  {
> -	return timer->state != HRTIMER_STATE_INACTIVE ||
> -		timer->base->running == timer;
> +	if (timer->state != HRTIMER_STATE_INACTIVE)
> +		return true;
> +
> +	smp_rmb(); /* C matches A */
> +
> +	if (timer->base->running == timer)
> +		return true;
> +
> +	smp_rmb(); /* D matches B */
> +
> +	if (timer->state != HRTIMER_STATE_INACTIVE)
> +		return true;
> +
> +	return false;

This races with two sequential timer handlers. hrtimer_active()
is preemptible everywhere, and no guarantees that all three "if"
conditions check the same timer tick.

How about transformation of hrtimer_bases.lock: raw_spinlock_t --> seqlock_t?

>  }
>  
>  /*
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1122,6 +1122,20 @@ static void __run_hrtimer(struct hrtimer
>  
>  	debug_deactivate(timer);
>  	base->running = timer;
> +
> +	/*
> +	 * Pairs with hrtimer_active().
> +	 *
> +	 *	[S] base->running = timer	[L] timer->state
> +	 *	    WMB 			    RMB
> +	 *	[S] timer->state = INACTIVE	[L] base->running
> +	 *
> +	 * BUG_ON(base->running != timer && timer->state != INACTIVE)
> +	 *
> +	 * If we observe INACTIVE we must observe base->running == timer.
> +	 */
> +	smp_wmb(); /* A matches C */
> +
>  	__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
>  	timer_stats_account_hrtimer(timer);
>  	fn = timer->function;
> @@ -1150,6 +1164,20 @@ static void __run_hrtimer(struct hrtimer
>  	    !(timer->state & HRTIMER_STATE_ENQUEUED))
>  		enqueue_hrtimer(timer, base);
>  
> +	/*
> +	 * Pairs with hrtimer_active().
> +	 *
> +	 *	[S] timer->state = ENQUEUED	[L] base->running
> +	 *	    WMB 			    RMB
> +	 *	[S] base->running = NULL	[L] timer->state
> +	 *
> +	 * BUG_ON(base->running == NULL && timer->state == INACTIVE)
> +	 *
> +	 * If we observe base->running == NULL, we must observe any preceding
> +	 * enqueue.
> +	 */
> +	smp_wmb(); /* B matches D */
> +
>  	WARN_ON_ONCE(base->running != timer);
>  	base->running = NULL;
>  }
>