Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754877AbcJKUk0 (ORCPT ); Tue, 11 Oct 2016 16:40:26 -0400 Received: from rhlx01.hs-esslingen.de ([129.143.116.10]:36778 "EHLO rhlx01.hs-esslingen.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753113AbcJKUkX (ORCPT ); Tue, 11 Oct 2016 16:40:23 -0400 Date: Tue, 11 Oct 2016 22:40:03 +0200 From: Andreas Mohr To: Doug Anderson Cc: Andreas Mohr , Thomas Gleixner , John Stultz , Brian Norris , Tao Huang , Tony Xie , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Message-ID: <20161011204003.GA21387@rhlx01.hs-esslingen.de> References: <1476133442-17757-1-git-send-email-dianders@chromium.org> <20161011193034.GA10646@rhlx01.hs-esslingen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Priority: none User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4264 Lines: 102 On Tue, Oct 11, 2016 at 01:02:10PM -0700, Doug Anderson wrote: > Andreas, > > On Tue, Oct 11, 2016 at 12:30 PM, Andreas Mohr wrote: > > This loop implementation relies on negative kmin (result of ktime_sub()) > > getting internally handled by schedule_hrtimeout_range() as a 0 result. > > If that ain't the case, then the loop itself will need to handle > > negative values. > > OK, scratch that, due to guaranteed-unchanged values of now and end, > > result of directly subsequent ktime_sub() is guaranteed to > > not deviate from what ktime_before() figured. > > However, this is somewhat differing handling of these two APIs. > > I read this as: no bug, feel free to ignore. Heh, yeah ("an alternate style of review" ;). > I agree that we could try to save some math by making the loop more > complicated. On the other hand, one could argue that a sufficiently > good compiler might actually be able to figure some of this stuff out, > since much of this stuff is just inlined 64-bit math comparisons. Indeed. "micro-optimization again". > > BTW, another argument for loop rework might be that > > the result of ktime_sub() might already be improper > > (due to being negative!) for > > subsequent invocation of schedule_hrtimeout_range(), > > i.e. there's an argument to be made that > > the loop tail check instead ought to be done as a negative-value check > > directly prior to schedule_hrtimeout_range() invocation. > > > > Hmm, if schedule_hrtimeout_range() can be relied on to > > internally properly be doing the negative check, > > then one could simply say that > > the annoyingly extra calculation via ktime_before() call > > should simply be removed completely. > > Which would be a good step since it would centralize protocol behaviour > > within the inner handling of the helper > > rather than bloating user-side instruction cache footprint. > > Ah, so you're saying just do a "while (true)" after changing the > behavior of schedule_hrtimeout_range_clock(). If everyone agrees that > they'd like to see this, I can do it. I'd have to change > schedule_hrtimeout_range_clock() to check for <= 0 instead of just > comparing against 0. ...and that would be an API change for > schedule_hrtimeout_range_clock(), and it seems like that would be yet > another source of bugs. ...and this is to save an instruction? It > doesn't seem worth it. Yup, I just realized that probably what we'd ideally want is a very thin decorating wrapper (loop handler) which cares about nothing else other than eradicating the relevant "feature" of the inner handler (namely, premature exit), and otherwise leaves a much as possible to central inner handler decision-making implementation. That to me sounds like the theoretically most precise (since dead simple) handling. And even if such exceedingly simple handling is dangerous - in case of failure we would realize it very soon (infinite loop), and would then know what will need fixing. I've been reviewing schedule_hrtimeout_range_clock() as well, and I'm actually puzzled that it checks for zero value only, and not less-equal zero. That to me does not seem like a true-to-the-core protocol to handle the task at hand. OTOH perhaps less-equal comparison was deemed to be much more expensive than a zero check, and thus possibly has been done in inner handlers only. For the loop code itself, I'm not sure whether to pursue it - given the schedule_hrtimeout_range_clock() API change risks you outlined, and especially given that optimization is likely to mostly benefit the "repeat" case only, which likely would be the less relevant non-hotpath case anyway. Plus, let's not forget the fact that even hotpath handling *is* an invocation of the entire delay handler, and then a couple measly opcodes to have the loop exited reliably. So... Now, at least we have a Reviewed-by: Andreas Mohr Oh well, lotsa discussion, some good thoughts, but no truly revolutionary outcome so far. However, sometimes the most important thing is to have had a good educating discussion (not everything in software circles is about the Get Rich Quick scheme - you do have to spend some ample time - decades... - to truly get mental concept things right). Andreas Mohr