* [PATCH] doc: add note on usleep_range range
@ 2016-12-13 3:58 Nicholas Mc Guire
2016-12-13 9:10 ` Jani Nikula
2016-12-27 21:56 ` Pavel Machek
0 siblings, 2 replies; 14+ messages in thread
From: Nicholas Mc Guire @ 2016-12-13 3:58 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jonathan Corbet, linux-kernel, linux-doc, Nicholas Mc Guire
useleep_range() with a delta of 0 makes no sense and only prevents the
timer subsystem from optimizing interrupts. As any user of usleep_range()
is in non-atomic context the timer jitter is in the range of 10s of
microseconds anyway.
This adds a note making it clear that a range of 0 is a bad idea.
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
as of 4.9.0 there are about 20 cases of usleep_ranges() that have
min==max and none of them really look like they are necessary, so
it does seem like a relatively common misunderstanding worth
noting in the documentation.
Patch is against 4.9.0 (localversion-next is 20161212)
Documentation/timers/timers-howto.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
index 038f8c7..b5cdf82 100644
--- a/Documentation/timers/timers-howto.txt
+++ b/Documentation/timers/timers-howto.txt
@@ -93,6 +93,13 @@ NON-ATOMIC CONTEXT:
tolerances here are very situation specific, thus it
is left to the caller to determine a reasonable range.
+ A range of 0, that is usleep_range(100,100) or the
+ like, do not make sense as this code is in a
+ non-atomic section and a system can not be expected
+ to have jitter 0. For any non-RT code any delta
+ less than 50 microseconds probably is only preventing
+ timer subsystem optimization but providing no benefit.
+
SLEEPING FOR LARGER MSECS ( 10ms+ )
* Use msleep or possibly msleep_interruptible
--
2.1.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 3:58 [PATCH] doc: add note on usleep_range range Nicholas Mc Guire
@ 2016-12-13 9:10 ` Jani Nikula
2016-12-13 9:19 ` Nicholas Mc Guire
2016-12-27 21:56 ` Pavel Machek
1 sibling, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2016-12-13 9:10 UTC (permalink / raw)
To: Nicholas Mc Guire, Thomas Gleixner
Cc: Jonathan Corbet, linux-kernel, linux-doc, Nicholas Mc Guire,
Dan Carpenter, Julia Lawall
On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> useleep_range() with a delta of 0 makes no sense and only prevents the
> timer subsystem from optimizing interrupts. As any user of usleep_range()
> is in non-atomic context the timer jitter is in the range of 10s of
> microseconds anyway.
>
> This adds a note making it clear that a range of 0 is a bad idea.
So I don't really have anything to do with the timer subsystem, I'm just
their "consumer", so take this with a grain of salt.
Documentation is good, but I don't think this will be enough.
I think the only thing that will work is to detect and complain about
things like this automatically. Some ideas:
* WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
might be drastic, but it would get the job done eventually.
* If you want to avoid the runtime overhead (and complaints about the
backtraces), you could wrap usleep_range() in a macro that does
BUILD_BUG_ON(min == max) if the parameters are build time constants
(they usually are). But you'd have to fix all the problem cases first.
* You could try (to persuade Julia or Dan) to come up with a
cocci/smatch check for usleep_range() calls where min == max, so we
could get bug reports for this. This probably works on expressions, so
this would catch also cases where the parameters aren't built time
constants.
BR,
Jani.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> as of 4.9.0 there are about 20 cases of usleep_ranges() that have
> min==max and none of them really look like they are necessary, so
> it does seem like a relatively common misunderstanding worth
> noting in the documentation.
>
> Patch is against 4.9.0 (localversion-next is 20161212)
>
> Documentation/timers/timers-howto.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
> index 038f8c7..b5cdf82 100644
> --- a/Documentation/timers/timers-howto.txt
> +++ b/Documentation/timers/timers-howto.txt
> @@ -93,6 +93,13 @@ NON-ATOMIC CONTEXT:
> tolerances here are very situation specific, thus it
> is left to the caller to determine a reasonable range.
>
> + A range of 0, that is usleep_range(100,100) or the
> + like, do not make sense as this code is in a
> + non-atomic section and a system can not be expected
> + to have jitter 0. For any non-RT code any delta
> + less than 50 microseconds probably is only preventing
> + timer subsystem optimization but providing no benefit.
> +
> SLEEPING FOR LARGER MSECS ( 10ms+ )
> * Use msleep or possibly msleep_interruptible
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 9:10 ` Jani Nikula
@ 2016-12-13 9:19 ` Nicholas Mc Guire
2016-12-13 10:18 ` Jani Nikula
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Nicholas Mc Guire @ 2016-12-13 9:19 UTC (permalink / raw)
To: Jani Nikula
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > useleep_range() with a delta of 0 makes no sense and only prevents the
> > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > is in non-atomic context the timer jitter is in the range of 10s of
> > microseconds anyway.
> >
> > This adds a note making it clear that a range of 0 is a bad idea.
>
> So I don't really have anything to do with the timer subsystem, I'm just
> their "consumer", so take this with a grain of salt.
>
> Documentation is good, but I don't think this will be enough.
>
> I think the only thing that will work is to detect and complain about
> things like this automatically. Some ideas:
>
> * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> might be drastic, but it would get the job done eventually.
>
> * If you want to avoid the runtime overhead (and complaints about the
> backtraces), you could wrap usleep_range() in a macro that does
> BUILD_BUG_ON(min == max) if the parameters are build time constants
> (they usually are). But you'd have to fix all the problem cases first.
>
> * You could try (to persuade Julia or Dan) to come up with a
> cocci/smatch check for usleep_range() calls where min == max, so we
> could get bug reports for this. This probably works on expressions, so
> this would catch also cases where the parameters aren't built time
> constants.
>
I fully agree - without automation it is almost usless
the coccinelle spatch is a seperate patch and it is tested butnot yet
submitted.
the spatch for this iss actually trivial
@nulldelta@
constant C;
position p;
@@
* usleep_range@p(C,C)
thx!
hofrat
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 9:19 ` Nicholas Mc Guire
@ 2016-12-13 10:18 ` Jani Nikula
2016-12-13 12:05 ` Julia Lawall
2016-12-14 0:27 ` Joe Perches
2 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2016-12-13 10:18 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
On Tue, 13 Dec 2016, Nicholas Mc Guire <der.herr@hofr.at> wrote:
> I fully agree - without automation it is almost usless
> the coccinelle spatch is a seperate patch and it is tested butnot yet
> submitted.
Good, good! Sorry for the noise.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 9:19 ` Nicholas Mc Guire
2016-12-13 10:18 ` Jani Nikula
@ 2016-12-13 12:05 ` Julia Lawall
2016-12-13 12:24 ` Nicholas Mc Guire
2016-12-14 0:27 ` Joe Perches
2 siblings, 1 reply; 14+ messages in thread
From: Julia Lawall @ 2016-12-13 12:05 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Jani Nikula, Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:
> On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> > On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > useleep_range() with a delta of 0 makes no sense and only prevents the
> > > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > > is in non-atomic context the timer jitter is in the range of 10s of
> > > microseconds anyway.
> > >
> > > This adds a note making it clear that a range of 0 is a bad idea.
> >
> > So I don't really have anything to do with the timer subsystem, I'm just
> > their "consumer", so take this with a grain of salt.
> >
> > Documentation is good, but I don't think this will be enough.
> >
> > I think the only thing that will work is to detect and complain about
> > things like this automatically. Some ideas:
> >
> > * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> > might be drastic, but it would get the job done eventually.
> >
> > * If you want to avoid the runtime overhead (and complaints about the
> > backtraces), you could wrap usleep_range() in a macro that does
> > BUILD_BUG_ON(min == max) if the parameters are build time constants
> > (they usually are). But you'd have to fix all the problem cases first.
> >
> > * You could try (to persuade Julia or Dan) to come up with a
> > cocci/smatch check for usleep_range() calls where min == max, so we
> > could get bug reports for this. This probably works on expressions, so
> > this would catch also cases where the parameters aren't built time
> > constants.
> >
>
> I fully agree - without automation it is almost usless
> the coccinelle spatch is a seperate patch and it is tested butnot yet
> submitted.
>
> the spatch for this iss actually trivial
>
> @nulldelta@
> constant C;
> position p;
> @@
>
> * usleep_range@p(C,C)
People never use more complex expressions?
julia
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 12:05 ` Julia Lawall
@ 2016-12-13 12:24 ` Nicholas Mc Guire
0 siblings, 0 replies; 14+ messages in thread
From: Nicholas Mc Guire @ 2016-12-13 12:24 UTC (permalink / raw)
To: Julia Lawall
Cc: Jani Nikula, Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter
On Tue, Dec 13, 2016 at 01:05:12PM +0100, Julia Lawall wrote:
>
>
> On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:
>
> > On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> > > On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > > useleep_range() with a delta of 0 makes no sense and only prevents the
> > > > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > > > is in non-atomic context the timer jitter is in the range of 10s of
> > > > microseconds anyway.
> > > >
> > > > This adds a note making it clear that a range of 0 is a bad idea.
> > >
> > > So I don't really have anything to do with the timer subsystem, I'm just
> > > their "consumer", so take this with a grain of salt.
> > >
> > > Documentation is good, but I don't think this will be enough.
> > >
> > > I think the only thing that will work is to detect and complain about
> > > things like this automatically. Some ideas:
> > >
> > > * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> > > might be drastic, but it would get the job done eventually.
> > >
> > > * If you want to avoid the runtime overhead (and complaints about the
> > > backtraces), you could wrap usleep_range() in a macro that does
> > > BUILD_BUG_ON(min == max) if the parameters are build time constants
> > > (they usually are). But you'd have to fix all the problem cases first.
> > >
> > > * You could try (to persuade Julia or Dan) to come up with a
> > > cocci/smatch check for usleep_range() calls where min == max, so we
> > > could get bug reports for this. This probably works on expressions, so
> > > this would catch also cases where the parameters aren't built time
> > > constants.
> > >
> >
> > I fully agree - without automation it is almost usless
> > the coccinelle spatch is a seperate patch and it is tested butnot yet
> > submitted.
> >
> > the spatch for this iss actually trivial
> >
> > @nulldelta@
> > constant C;
> > position p;
> > @@
> >
> > * usleep_range@p(C,C)
>
> People never use more complex expressions?
>
well yes
@nulldelta@
expression E;
position p;
@@
* usleep_range@p(E,E)
but that seems to be it.
and the vast majority is simply constants
thx!
hofrat
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 9:19 ` Nicholas Mc Guire
2016-12-13 10:18 ` Jani Nikula
2016-12-13 12:05 ` Julia Lawall
@ 2016-12-14 0:27 ` Joe Perches
2016-12-14 0:37 ` Nicholas Mc Guire
2 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2016-12-14 0:27 UTC (permalink / raw)
To: Nicholas Mc Guire, Jani Nikula
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
a, On Tue, 2016-12-13 at 09:19 +0000, Nicholas Mc Guire wrote:
> On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> > On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > useleep_range() with a delta of 0 makes no sense and only prevents the
> > > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > > is in non-atomic context the timer jitter is in the range of 10s of
> > > microseconds anyway.
> > >
> > > This adds a note making it clear that a range of 0 is a bad idea.
> >
> > So I don't really have anything to do with the timer subsystem, I'm just
> > their "consumer", so take this with a grain of salt.
> >
> > Documentation is good, but I don't think this will be enough.
> >
> > I think the only thing that will work is to detect and complain about
> > things like this automatically. Some ideas:
> >
> > * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> > might be drastic, but it would get the job done eventually.
> >
> > * If you want to avoid the runtime overhead (and complaints about the
> > backtraces), you could wrap usleep_range() in a macro that does
> > BUILD_BUG_ON(min == max) if the parameters are build time constants
> > (they usually are). But you'd have to fix all the problem cases first.
> >
> > * You could try (to persuade Julia or Dan) to come up with a
> > cocci/smatch check for usleep_range() calls where min == max, so we
> > could get bug reports for this. This probably works on expressions, so
> > this would catch also cases where the parameters aren't built timea,
> > constants.
You could also add a macro for usleep_range like
#define usleep_range(a, b) \
({ \
if (__builtin_constant_p(a) && __builtin_constant_p(b)) { \
if (a == b) \
__compiletime_warning("Better to use usleep_range with different values"); \
else if (a > b) \
__compiletime_error("usleep_range uses smaller value first"); \
} \
usleep_range(a, b); \
})
and add parentheses around the actual function
definition for usleep_range in kernel/time/timer.c
so the macro works and these messages get emitted
at compile-time.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-14 0:27 ` Joe Perches
@ 2016-12-14 0:37 ` Nicholas Mc Guire
2016-12-14 6:10 ` Joe Perches
0 siblings, 1 reply; 14+ messages in thread
From: Nicholas Mc Guire @ 2016-12-14 0:37 UTC (permalink / raw)
To: Joe Perches
Cc: Jani Nikula, Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
On Tue, Dec 13, 2016 at 04:27:32PM -0800, Joe Perches wrote:
> a, On Tue, 2016-12-13 at 09:19 +0000, Nicholas Mc Guire wrote:
> > On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> > > On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > > useleep_range() with a delta of 0 makes no sense and only prevents the
> > > > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > > > is in non-atomic context the timer jitter is in the range of 10s of
> > > > microseconds anyway.
> > > >
> > > > This adds a note making it clear that a range of 0 is a bad idea.
> > >
> > > So I don't really have anything to do with the timer subsystem, I'm just
> > > their "consumer", so take this with a grain of salt.
> > >
> > > Documentation is good, but I don't think this will be enough.
> > >
> > > I think the only thing that will work is to detect and complain about
> > > things like this automatically. Some ideas:
> > >
> > > * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> > > might be drastic, but it would get the job done eventually.
> > >
> > > * If you want to avoid the runtime overhead (and complaints about the
> > > backtraces), you could wrap usleep_range() in a macro that does
> > > BUILD_BUG_ON(min == max) if the parameters are build time constants
> > > (they usually are). But you'd have to fix all the problem cases first.
> > >
> > > * You could try (to persuade Julia or Dan) to come up with a
> > > cocci/smatch check for usleep_range() calls where min == max, so we
> > > could get bug reports for this. This probably works on expressions, so
> > > this would catch also cases where the parameters aren't built timea,
> > > constants.
>
> You could also add a macro for usleep_range like
>
> #define usleep_range(a, b) \
> ({ \
> if (__builtin_constant_p(a) && __builtin_constant_p(b)) { \
> if (a == b) \
> __compiletime_warning("Better to use usleep_range with different values"); \
> else if (a > b) \
> __compiletime_error("usleep_range uses smaller value first"); \
> } \
> usleep_range(a, b); \
> })
>
thanks for that "template"
> and add parentheses around the actual function
> definition for usleep_range in kernel/time/timer.c
> so the macro works and these messages get emitted
> at compile-time.
>
while compiletime warnings are a way to go I think that an
external tool is more effective than anoying eveyone during
build - ideally this type of issue is filtered out in the
subsystem trees or -next latest so getting it into a
coccinelle spatch and into one of the CI seems the most
resonable way to go. And as a side-effect tools external
to the build process allow analysis into the history of the
kernel development (like statistics on API usage and bug
history).
thx!
hofrat
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-14 0:37 ` Nicholas Mc Guire
@ 2016-12-14 6:10 ` Joe Perches
0 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2016-12-14 6:10 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Jani Nikula, Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc, Dan Carpenter, Julia Lawall
On Wed, 2016-12-14 at 00:37 +0000, Nicholas Mc Guire wrote:
> On Tue, Dec 13, 2016 at 04:27:32PM -0800, Joe Perches wrote:
> > a, On Tue, 2016-12-13 at 09:19 +0000, Nicholas Mc Guire wrote:
> > > On Tue, Dec 13, 2016 at 11:10:50AM +0200, Jani Nikula wrote:
> > > > On Tue, 13 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > > > useleep_range() with a delta of 0 makes no sense and only prevents the
> > > > > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > > > > is in non-atomic context the timer jitter is in the range of 10s of
> > > > > microseconds anyway.
> > > > >
> > > > > This adds a note making it clear that a range of 0 is a bad idea.
> > > >
> > > > So I don't really have anything to do with the timer subsystem, I'm just
> > > > their "consumer", so take this with a grain of salt.
> > > >
> > > > Documentation is good, but I don't think this will be enough.
> > > >
> > > > I think the only thing that will work is to detect and complain about
> > > > things like this automatically. Some ideas:
> > > >
> > > > * WARN_ON(min == max) or WARN_ON_ONCE(min == max) in usleep_range()
> > > > might be drastic, but it would get the job done eventually.
> > > >
> > > > * If you want to avoid the runtime overhead (and complaints about the
> > > > backtraces), you could wrap usleep_range() in a macro that does
> > > > BUILD_BUG_ON(min == max) if the parameters are build time constants
> > > > (they usually are). But you'd have to fix all the problem cases first.
> > > >
> > > > * You could try (to persuade Julia or Dan) to come up with a
> > > > cocci/smatch check for usleep_range() calls where min == max, so we
> > > > could get bug reports for this. This probably works on expressions, so
> > > > this would catch also cases where the parameters aren't built timea,
> > > > constants.
> >
> > You could also add a macro for usleep_range like
> >
> > #define usleep_range(a, b) \
> > ({ \
> > if (__builtin_constant_p(a) && __builtin_constant_p(b)) { \
> > if (a == b) \
> > __compiletime_warning("Better to use usleep_range with different values"); \
> > else if (a > b) \
> > __compiletime_error("usleep_range uses smaller value first"); \
> > } \
> > usleep_range(a, b); \
> > })
> >
>
> thanks for that "template"
>
> > and add parentheses around the actual function
> > definition for usleep_range in kernel/time/timer.c
> > so the macro works and these messages get emitted
> > at compile-time.
> >
>
> while compiletime warnings are a way to go I think that an
> external tool is more effective than anoying eveyone during
> build
I don't.
Annoying people at build-time is probably _the single most_
effective way to get source code defects fixed.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-13 3:58 [PATCH] doc: add note on usleep_range range Nicholas Mc Guire
2016-12-13 9:10 ` Jani Nikula
@ 2016-12-27 21:56 ` Pavel Machek
2017-01-07 19:41 ` Nicholas Mc Guire
1 sibling, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2016-12-27 21:56 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Thomas Gleixner, Jonathan Corbet, linux-kernel, linux-doc
[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]
On Tue 2016-12-13 04:58:43, Nicholas Mc Guire wrote:
> useleep_range() with a delta of 0 makes no sense and only prevents the
> timer subsystem from optimizing interrupts. As any user of usleep_range()
> is in non-atomic context the timer jitter is in the range of 10s of
> microseconds anyway.
>
> This adds a note making it clear that a range of 0 is a bad idea.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> as of 4.9.0 there are about 20 cases of usleep_ranges() that have
> min==max and none of them really look like they are necessary, so
> it does seem like a relatively common misunderstanding worth
> noting in the documentation.
>
> Patch is against 4.9.0 (localversion-next is 20161212)
>
> Documentation/timers/timers-howto.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
> index 038f8c7..b5cdf82 100644
> --- a/Documentation/timers/timers-howto.txt
> +++ b/Documentation/timers/timers-howto.txt
> @@ -93,6 +93,13 @@ NON-ATOMIC CONTEXT:
> tolerances here are very situation specific, thus it
> is left to the caller to determine a reasonable range.
>
> + A range of 0, that is usleep_range(100,100) or the
> + like, do not make sense as this code is in a
> + non-atomic section and a system can not be expected
> + to have jitter 0. For any non-RT code any delta
Would it be possible to fix english here?
"to have zero jitter" at least. I believe it is "does not".
I don't see how atomic vs. non-atomic context makes difference. There
are sources of jitter that affect atomic context...
> + less than 50 microseconds probably is only preventing
> + timer subsystem optimization but providing no benefit.
And I don't trust you here. _If_ it prevents timer optimalization,
_then_ it provides benefit, at least in the average case.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2016-12-27 21:56 ` Pavel Machek
@ 2017-01-07 19:41 ` Nicholas Mc Guire
2017-01-10 21:25 ` Pavel Machek
0 siblings, 1 reply; 14+ messages in thread
From: Nicholas Mc Guire @ 2017-01-07 19:41 UTC (permalink / raw)
To: Pavel Machek
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc
On Tue, Dec 27, 2016 at 10:56:26PM +0100, Pavel Machek wrote:
> On Tue 2016-12-13 04:58:43, Nicholas Mc Guire wrote:
> > useleep_range() with a delta of 0 makes no sense and only prevents the
> > timer subsystem from optimizing interrupts. As any user of usleep_range()
> > is in non-atomic context the timer jitter is in the range of 10s of
> > microseconds anyway.
> >
> > This adds a note making it clear that a range of 0 is a bad idea.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >
> > as of 4.9.0 there are about 20 cases of usleep_ranges() that have
> > min==max and none of them really look like they are necessary, so
> > it does seem like a relatively common misunderstanding worth
> > noting in the documentation.
> >
> > Patch is against 4.9.0 (localversion-next is 20161212)
> >
> > Documentation/timers/timers-howto.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
> > index 038f8c7..b5cdf82 100644
> > --- a/Documentation/timers/timers-howto.txt
> > +++ b/Documentation/timers/timers-howto.txt
> > @@ -93,6 +93,13 @@ NON-ATOMIC CONTEXT:
> > tolerances here are very situation specific, thus it
> > is left to the caller to determine a reasonable range.
> >
> > + A range of 0, that is usleep_range(100,100) or the
> > + like, do not make sense as this code is in a
> > + non-atomic section and a system can not be expected
> > + to have jitter 0. For any non-RT code any delta
>
> Would it be possible to fix english here?
Aggreed that is crappy language - my bad - will fix - thanks!
>
> "to have zero jitter" at least. I believe it is "does not".
>
> I don't see how atomic vs. non-atomic context makes difference. There
> are sources of jitter that affect atomic context...
The relevance is that while there is jitter in atomic context it can
be quite small (depending on your hardware and the specifics of system
config) but in non-atomic context the jitter is so large that it
makes no relevant difference if you give usleep_range slack of a few
microseconds.
>
> > + less than 50 microseconds probably is only preventing
> > + timer subsystem optimization but providing no benefit.
>
> And I don't trust you here. _If_ it prevents timer optimalization,
> _then_ it provides benefit, at least in the average case.
>
here is the data:
System: Intel Core i7 CPU 920 @ 2.67GHz Ocotocore
OS: Debian 8.1 (but thats quite irrelevant)
Kernel: 4.10-rc2 (localversion-next next-20170106)
config: x86_64_defconfig (Voluntary | Preempt)
Test-setup - poped this into akernel module and just
brute force load/unload it in a loop - not very elegant
but it does the job.
static int __init usleep_test_init(void)
{
ktime_t now,last;
unsigned long min,max;
min = 200;
max = 250;
last = ktime_get();
usleep_range(min, max);
now = ktime_get();
printk("%llu\n", ktime_to_ns(now)-ktime_to_ns(last));
return 0;
}
Results:
usleep_range() 5000 samples - idle system
100,100 200,200 190,200
Min. :188481 Min. :201917 Min. :197793
1st Qu.:207062 1st Qu.:207057 1st Qu.:207051
Median :207139 Median :207133 Median :207133
Mean :207254 Mean :207233 Mean :207244
3rd Qu.:207341 erd Qu.:207262 3rd Qu.:207610
Max. :225340 Max. :214222 Max. :214885
100,200 to 200,200 is maybe relevant impact for
some systems with respect to the outliers, but
mean and median are almost the same, for
190,200 to 200,200 there is statistically no
significant difference with respect to performance
Note that the timestamp before and after also has
jitter - so only part of the jitter can be attributed
to usleep_range() it self. But idle system optimization
is not that interesting for most systems.
On a loaded box:
Load here means that 8 find / | grep bla loops were started
(not pinned to any particular core) and then the
usleep_range() test ran 5000 times.
Same setup as above but this time we differenciae between
PREEMTI and PREEMPT_VOLUNTARY
CONFIG_PREEMPT_VOLUNTARY=y
usleep_range() 5000 samples - load ~ 8
100,200 190,200 200,200
Min. : 107812 Min. : 203307 Min. : 203432
1st Qu.: 558221 1st Qu.: 557490 1st Qu.: 510356
Median :1123425 Median : 1121939 Median : 1123316
Mean :1103718 Mean : 1100965 Mean : 1100542
3rd Qu.:1541986 3rd Qu.: 1531478 3rd Qu.: 1517414
Max. :8979183 Max. :13765789 Max. :12476136
CONFIG_PREEMPT=y
usleep_range() 5000 samples - load ~ 8
100,200 190,200 200,200
Min. : 115321 Min. : 203963 Min. : 203864
1st Qu.: 510296 1st Qu.: 451479 1st Qu.: 548131
Median : 1148660 Median : 1062576 Median : 1145228
Mean : 1193449 Mean : 1079379 Mean : 1154728
3rd Qu.: 1601552 3rd Qu.: 1378622 3rd Qu.: 1570742
Max. :12936192 Max. :12346313 Max. :13858732
So for a loaded system it simply makes no relevant difference
if you grant the subsystem 10 microseconds range or not. In
fact if one wanted 200 us and would allow fow 200,250 it would
be quite hard to notice the difference.
usleep_range(200,200) vs usleep_range(200,250)
Min. : 203864 Min. : 214003
1st Qu.: 548131 1st Qu.: 520436
Median : 1145228 Median : 1138698
Mean : 1154728 Mean : 1201871
3rd Qu.: 1570742 3rd Qu.: 1581952
Max. :13858732 Max. :12491198
I would call the difference insignificant - ploted as curves you
can hardly tell the distribution appart. As soon as you are looking
at more than a single tasks to optimize the difference would
probably completely disapear.
thx!
hofrat
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2017-01-07 19:41 ` Nicholas Mc Guire
@ 2017-01-10 21:25 ` Pavel Machek
2017-01-11 8:50 ` Nicholas Mc Guire
0 siblings, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2017-01-10 21:25 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc
[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]
Hi!
> > "to have zero jitter" at least. I believe it is "does not".
> >
> > I don't see how atomic vs. non-atomic context makes difference. There
> > are sources of jitter that affect atomic context...
>
> The relevance is that while there is jitter in atomic context it can
> be quite small (depending on your hardware and the specifics of system
> config) but in non-atomic context the jitter is so large that it
> makes no relevant difference if you give usleep_range slack of a few
> microseconds.
I disagree here. Even in non-atomic code, you'll get _no_ jitter most
of the time. If you care about average case, small slack may still
make sense.
> > > + less than 50 microseconds probably is only preventing
> > > + timer subsystem optimization but providing no benefit.
> >
> > And I don't trust you here. _If_ it prevents timer optimalization,
> > _then_ it provides benefit, at least in the average case.
> >
> here is the data:
>
> System: Intel Core i7 CPU 920 @ 2.67GHz Ocotocore
> OS: Debian 8.1 (but thats quite irrelevant)
> Kernel: 4.10-rc2 (localversion-next next-20170106)
> config: x86_64_defconfig (Voluntary | Preempt)
>
> Test-setup - poped this into akernel module and just
> brute force load/unload it in a loop - not very elegant
> but it does the job.
>
> static int __init usleep_test_init(void)
> {
> ktime_t now,last;
> unsigned long min,max;
> min = 200;
> max = 250;
> last = ktime_get();
> usleep_range(min, max);
> now = ktime_get();
> printk("%llu\n", ktime_to_ns(now)-ktime_to_ns(last));
> return 0;
> }
>
> Results:
>
> usleep_range() 5000 samples - idle system
> 100,100 200,200 190,200
> Min. :188481 Min. :201917 Min. :197793
> 1st Qu.:207062 1st Qu.:207057 1st Qu.:207051
> Median :207139 Median :207133 Median :207133
> Mean :207254 Mean :207233 Mean :207244
> 3rd Qu.:207341 erd Qu.:207262 3rd Qu.:207610
> Max. :225340 Max. :214222 Max. :214885
>
> 100,200 to 200,200 is maybe relevant impact for
> some systems with respect to the outliers, but
> mean and median are almost the same, for
> 190,200 to 200,200 there is statistically no
> significant difference with respect to performance
> Note that the timestamp before and after also has
> jitter - so only part of the jitter can be attributed
> to usleep_range() it self. But idle system optimization
> is not that interesting for most systems.
I disagree here. Most of systems are idle, most of the time. You say
that basically everyone should provide 50 usec of slack... So I guess
I'd like to see comparisons for 200,200 and 200,250 (and perhaps also
200,500 or something).
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2017-01-10 21:25 ` Pavel Machek
@ 2017-01-11 8:50 ` Nicholas Mc Guire
2017-01-12 10:32 ` Pavel Machek
0 siblings, 1 reply; 14+ messages in thread
From: Nicholas Mc Guire @ 2017-01-11 8:50 UTC (permalink / raw)
To: Pavel Machek
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc
On Tue, Jan 10, 2017 at 10:25:29PM +0100, Pavel Machek wrote:
> Hi!
>
> > > "to have zero jitter" at least. I believe it is "does not".
> > >
> > > I don't see how atomic vs. non-atomic context makes difference. There
> > > are sources of jitter that affect atomic context...
> >
> > The relevance is that while there is jitter in atomic context it can
> > be quite small (depending on your hardware and the specifics of system
> > config) but in non-atomic context the jitter is so large that it
> > makes no relevant difference if you give usleep_range slack of a few
> > microseconds.
>
> I disagree here. Even in non-atomic code, you'll get _no_ jitter most
> of the time. If you care about average case, small slack may still
> make sense.
yes - thats what the results say - the mean does not differe significantly
so if you care about average case - it makes no difference.
>
> > > > + less than 50 microseconds probably is only preventing
> > > > + timer subsystem optimization but providing no benefit.
> > >
> > > And I don't trust you here. _If_ it prevents timer optimalization,
> > > _then_ it provides benefit, at least in the average case.
> > >
> > here is the data:
> >
> > System: Intel Core i7 CPU 920 @ 2.67GHz Ocotocore
> > OS: Debian 8.1 (but thats quite irrelevant)
> > Kernel: 4.10-rc2 (localversion-next next-20170106)
> > config: x86_64_defconfig (Voluntary | Preempt)
> >
> > Test-setup - poped this into akernel module and just
> > brute force load/unload it in a loop - not very elegant
> > but it does the job.
> >
> > static int __init usleep_test_init(void)
> > {
> > ktime_t now,last;
> > unsigned long min,max;
> > min = 200;
> > max = 250;
> > last = ktime_get();
> > usleep_range(min, max);
> > now = ktime_get();
> > printk("%llu\n", ktime_to_ns(now)-ktime_to_ns(last));
> > return 0;
> > }
> >
> > Results:
> >
> > usleep_range() 5000 samples - idle system
> > 100,100 200,200 190,200
> > Min. :188481 Min. :201917 Min. :197793
> > 1st Qu.:207062 1st Qu.:207057 1st Qu.:207051
> > Median :207139 Median :207133 Median :207133
> > Mean :207254 Mean :207233 Mean :207244
> > 3rd Qu.:207341 erd Qu.:207262 3rd Qu.:207610
> > Max. :225340 Max. :214222 Max. :214885
> >
> > 100,200 to 200,200 is maybe relevant impact for
> > some systems with respect to the outliers, but
> > mean and median are almost the same, for
> > 190,200 to 200,200 there is statistically no
> > significant difference with respect to performance
> > Note that the timestamp before and after also has
> > jitter - so only part of the jitter can be attributed
> > to usleep_range() it self. But idle system optimization
> > is not that interesting for most systems.
>
> I disagree here. Most of systems are idle, most of the time. You say
> that basically everyone should provide 50 usec of slack... So I guess
> I'd like to see comparisons for 200,200 and 200,250 (and perhaps also
> 200,500 or something).
>
I did not say that everyone should use 50us of slack - rather the statement
was "makes no relevant difference if you give usleep_range slack of a few
microseconds." and that min==max makes *no* sense and that providing
even just small slack (in 10s of us range) makes a relevant difference
at system level.
Regarding idle system - the statement is that optimizing for idle
system makes no sense - not that idle system is rare. In an idle
system (as you can see in the above table) there is *no* diffeence
in the mean values - just to highligt this
100,200 200,200 190,200
Mean :207254 Mean :207233 Mean :207244
so for an idle system it makes very little difference (and I still doubt
that anyone could find this sub promille difference by testing at the
application level) - conversely for a loaded system the whole issue is
irrelevant as the jitter is completely dominated from system activity and
the usleep_range() parameters have more or less no impact.
In summary:
idle-system: 10s of us difference between min/max has if at all
marginal impact
loaded-system: no negative impact at all
but the system as a whole can profit from reducing the number of hires
timersit needs to hanle. Thus I still see no reason to not consider
usleep_range(min,max) with min==max as a mistake.
But to put a numer on it - if max-min < 10us I would consider it wrong
I think that basically never makes sense for any non RT (PREEMT-RT that
is) thread.
thx!
hofrat
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] doc: add note on usleep_range range
2017-01-11 8:50 ` Nicholas Mc Guire
@ 2017-01-12 10:32 ` Pavel Machek
0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2017-01-12 10:32 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Nicholas Mc Guire, Thomas Gleixner, Jonathan Corbet,
linux-kernel, linux-doc
[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]
On Wed 2017-01-11 08:50:07, Nicholas Mc Guire wrote:
> On Tue, Jan 10, 2017 at 10:25:29PM +0100, Pavel Machek wrote:
> > Hi!
> >
> > > > "to have zero jitter" at least. I believe it is "does not".
> > > >
> > > > I don't see how atomic vs. non-atomic context makes difference. There
> > > > are sources of jitter that affect atomic context...
> > >
> > > The relevance is that while there is jitter in atomic context it can
> > > be quite small (depending on your hardware and the specifics of system
> > > config) but in non-atomic context the jitter is so large that it
> > > makes no relevant difference if you give usleep_range slack of a few
> > > microseconds.
> >
> > I disagree here. Even in non-atomic code, you'll get _no_ jitter most
> > of the time. If you care about average case, small slack may still
> > make sense.
>
> yes - thats what the results say - the mean does not differe significantly
> so if you care about average case - it makes no difference.
You did not demonstrate that.
> > > usleep_range() 5000 samples - idle system
> > > 100,100 200,200 190,200
> > > Min. :188481 Min. :201917 Min. :197793
> > > 1st Qu.:207062 1st Qu.:207057 1st Qu.:207051
> > > Median :207139 Median :207133 Median :207133
> > > Mean :207254 Mean :207233 Mean :207244
> > > 3rd Qu.:207341 erd Qu.:207262 3rd Qu.:207610
> > > Max. :225340 Max. :214222 Max. :214885
> > >
> > > 100,200 to 200,200 is maybe relevant impact for
> > > some systems with respect to the outliers, but
> > > mean and median are almost the same, for
> > > 190,200 to 200,200 there is statistically no
> > > significant difference with respect to performance
> > > Note that the timestamp before and after also has
> > > jitter - so only part of the jitter can be attributed
> > > to usleep_range() it self. But idle system optimization
> > > is not that interesting for most systems.
> >
> > I disagree here. Most of systems are idle, most of the time. You say
> > that basically everyone should provide 50 usec of slack... So I guess
> > I'd like to see comparisons for 200,200 and 200,250 (and perhaps also
> > 200,500 or something).
> >
> I did not say that everyone should use 50us of slack - rather the statement
> was "makes no relevant difference if you give usleep_range slack of a few
> microseconds." and that min==max makes *no* sense and that providing
> even just small slack (in 10s of us range) makes a relevant difference
> at system level.
You did not demonstrate any "relevant difference at system level".
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-01-12 10:32 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 3:58 [PATCH] doc: add note on usleep_range range Nicholas Mc Guire
2016-12-13 9:10 ` Jani Nikula
2016-12-13 9:19 ` Nicholas Mc Guire
2016-12-13 10:18 ` Jani Nikula
2016-12-13 12:05 ` Julia Lawall
2016-12-13 12:24 ` Nicholas Mc Guire
2016-12-14 0:27 ` Joe Perches
2016-12-14 0:37 ` Nicholas Mc Guire
2016-12-14 6:10 ` Joe Perches
2016-12-27 21:56 ` Pavel Machek
2017-01-07 19:41 ` Nicholas Mc Guire
2017-01-10 21:25 ` Pavel Machek
2017-01-11 8:50 ` Nicholas Mc Guire
2017-01-12 10:32 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).