Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499200

Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process()

Show all headers | View raw

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 <andi@lisas.de> 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 <andim2@users.sf.net>


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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Douglas Anderson <dianders@chromium.org> - 2016-10-10 23:20 +0200
  Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Doug Anderson <dianders@chromium.org> - 2016-10-11 00:40 +0200
  Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Thomas Gleixner <tglx@linutronix.de> - 2016-10-11 09:20 +0200
    Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Doug Anderson <dianders@chromium.org> - 2016-10-11 18:40 +0200
      Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Mark Brown <broonie@kernel.org> - 2016-10-12 11:30 +0200
    Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Andreas Mohr <andi@lisas.de> - 2016-10-11 21:00 +0200
      Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:20 +0200
        Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Doug Anderson <dianders@chromium.org> - 2016-10-12 19:40 +0200
    Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Heiko Stuebner <heiko@sntech.de> - 2016-10-11 22:40 +0200
  Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Brian Norris <briannorris@chromium.org> - 2016-10-11 21:00 +0200
  Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Andreas Mohr <andi@lisas.de> - 2016-10-11 21:50 +0200
    Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() Doug Anderson <dianders@chromium.org> - 2016-10-11 22:10 +0200
      Re: [PATCH v2] timers: Fix usleep_range() in the context of  wake_up_process() Andreas Mohr <andi@lisas.de> - 2016-10-11 22:50 +0200
  Re: [v2] timers: Fix usleep_range() in the context of  wake_up_process() Guenter Roeck <linux@roeck-us.net> - 2016-10-12 18:10 +0200
    Re: [v2] timers: Fix usleep_range() in the context of  wake_up_process() Guenter Roeck <linux@roeck-us.net> - 2016-10-12 19:00 +0200
    Re: [v2] timers: Fix usleep_range() in the context of wake_up_process() Doug Anderson <dianders@chromium.org> - 2016-10-12 19:00 +0200

csiph-web