From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756051AbbDOK0s (ORCPT ); Wed, 15 Apr 2015 06:26:48 -0400 Received: from www.linutronix.de ([62.245.132.108]:39936 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754298AbbDOK0k (ORCPT ); Wed, 15 Apr 2015 06:26:40 -0400 Date: Wed, 15 Apr 2015 12:26:58 +0200 (CEST) From: Thomas Gleixner To: Peter Zijlstra 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() In-Reply-To: <20150415095011.743749536@infradead.org> Message-ID: References: <20150415094155.601987867@infradead.org> <20150415095011.743749536@infradead.org> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. So I rather prefer a subject line like this hrtimer: Allow concurrent hrtimer_start() for self restarting timers or such. > 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. Right. > 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()? Other than that I can see why you want that ... Thanks, tglx