Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753936AbbDOLbZ (ORCPT ); Wed, 15 Apr 2015 07:31:25 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:34934 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754334AbbDOLbS (ORCPT ); Wed, 15 Apr 2015 07:31:18 -0400 Date: Wed, 15 Apr 2015 13:31:05 +0200 From: Peter Zijlstra To: Thomas Gleixner Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, Ben Segall , Roman Gushchin , Paul Turner Subject: Re: [PATCH 1/3] hrtimer: Fix race between hrtimer_start() and __run_hrtimer() Message-ID: <20150415113105.GT5029@twins.programming.kicks-ass.net> References: <20150415094155.601987867@infradead.org> <20150415095011.743749536@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4133 Lines: 108 On Wed, Apr 15, 2015 at 12:26:58PM +0200, Thomas Gleixner wrote: > On Wed, 15 Apr 2015, Peter Zijlstra wrote: > > hrtimer: Fix race between hrtimer_start() and __run_hrtimer() > > I don't think that subject line is correct. > > Back in the early hrtimer days we made deliberately the design > decision that this kind of usage is forbidden. The reason for this is > that the hrtimer infrastructure cannot provide proper > serialization. So we thought it would be a sane restruction that > restarting a timer from the callback should not be mixed with > concurrent restarts from a different call site. Ah I was not aware. Until I changed the locking it was possible simply because everything was serialized by the base lock. So the concurrent start would either land before the callback or after it but not in the middle like it can now. > So I rather prefer a subject line like this > > hrtimer: Allow concurrent hrtimer_start() for self restarting timers > /me copy/paste, done! :-) > > To that effect, add a WARN when someone tries to forward an already > > enqueued timer. > > The warnon itself is nice, but what about sites which use > hrtimer_set_expires() and hrtimer_start_expires()? They are all inlines, furthermore forward is the most common way to change the expiry of periodic / self restarting timers so would gain us most. How about this then? --- Subject: hrtimer: Allow concurrent hrtimer_start() for self restarting timers From: Peter Zijlstra Date: Tue May 20 15:49:48 CEST 2014 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") Cc: Ben Segall Cc: Roman Gushchin Cc: Thomas Gleixner Cc: Paul Turner Signed-off-by: Peter Zijlstra (Intel) --- kernel/time/hrtimer.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -791,6 +791,9 @@ u64 hrtimer_forward(struct hrtimer *time if (delta.tv64 < 0) return 0; + if (WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED)) + return 0; + if (interval.tv64 < hrtimer_resolution) interval.tv64 = hrtimer_resolution; @@ -1131,11 +1134,14 @@ static void __run_hrtimer(struct hrtimer * Note: We clear the CALLBACK bit after enqueue_hrtimer and * we do not reprogramm the event hardware. Happens either in * hrtimer_start_range_ns() or in hrtimer_interrupt() + * + * Note: Because we dropped the cpu_base->lock above, + * hrtimer_start_range_ns() can have popped in and enqueued the timer + * for us already. */ - 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)); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/