[PATCH] doc: add note on usleep_range range

[PATCH] doc: add note on usleep_range range

[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

Re: [PATCH] doc: add note on usleep_range range

[Jani Nikula]

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

Re: [PATCH] doc: add note on usleep_range range

[Nicholas Mc Guire]

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

Re: [PATCH] doc: add note on usleep_range range

[Jani Nikula]

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

Re: [PATCH] doc: add note on usleep_range range

[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

Re: [PATCH] doc: add note on usleep_range range

[Nicholas Mc Guire]

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 

Re: [PATCH] doc: add note on usleep_range range

[Joe Perches]

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.

Re: [PATCH] doc: add note on usleep_range range

[Nicholas Mc Guire]

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

Re: [PATCH] doc: add note on usleep_range range

[Joe Perches]

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.

Re: [PATCH] doc: add note on usleep_range range

[Pavel Machek]

[-- 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 --]

Re: [PATCH] doc: add note on usleep_range range

[Nicholas Mc Guire]

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

Re: [PATCH] doc: add note on usleep_range range

[Pavel Machek]

[-- 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 --]

Re: [PATCH] doc: add note on usleep_range range

[Nicholas Mc Guire]

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

Re: [PATCH] doc: add note on usleep_range range

[Pavel Machek]

[-- 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 --]