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

[Peter Zijlstra <peterz@infradead.org>]

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;
 }
 
 /*
--- 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;
 }