Documentation/timers/timers-howto.txt outlines the intended usage of
usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
V2: added context mode as suggested by Julia Lawall <[email protected]>
added min<max case sugested by Joe Perches <[email protected]>
added in the range checks as they are resonably reliable based on
a review of all 1648 call sites of usleep_range()
1648 calls total
1488 pass numeric values only (90.29%)
27 min below 10us (1.81%)
40 min above 10ms (2.68%)
min out of spec 4.50%
76 preprocessor constants (4.61%)
1 min below 10us (1.31%)
8 min above 10ms (10.52%)
min out of spec 11.84%
85 expressions (5.15%)
1(0) min below 10us (1.50%)*
6(2) min above 10ms (7.50%)*
min out of spec 9.0%
Errors:
23 where min==max (1.39%)
0 where max < min (0.00%)
Total:
Bugs: 6.48%-10.70%*
Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
Detectable by coccinelle:
Bugs: 74/103 (71.8%)
Crit: 50/52 (96.1%)
* numbers estimated based on code review
Patch is againts 4.9.0 (localversion-next is next-20161214)
scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
1 file changed, 88 insertions(+)
create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
new file mode 100644
index 0000000..003e9ef
--- /dev/null
+++ b/scripts/coccinelle/api/bad_usleep_range.cocci
@@ -0,0 +1,88 @@
+/// report bad/problematic usleep_range usage
+//
+// This is a checker for the documented intended use of usleep_range
+// see: Documentation/timers/timers-howto.txt and
+// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
+// when mdelay might not be a suitable replacement
+//
+// Limitations:
+// * The numeric limits are only checked when numeric constants are in
+// use (as of 4.9.0 thats 90.29% of the calls) no constant folding
+// is done - so this can miss some out-of-range cases - but in 4.9.0
+// it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
+// (96.1%) of the critical cases (min < 10 and min==max - there
+// * There may be RT use-cases where both min < 10 and min==max)
+// justified (e.g. high-throughput drivers on a shielded core)
+//
+// 1) warn if min == max
+//
+// The problem is that usleep_range is calculating the delay by
+// exp = ktime_add_us(ktime_get(), min)
+// delta = (u64)(max - min) * NSEC_PER_USEC
+// so delta is set to 0 if min==max
+// and then calls
+// schedule_hrtimeout_range(exp, 0,...)
+// effectively this means that the clock subsystem has no room to
+// optimize. usleep_range() is in non-atomic context so a 0 range
+// makes very little sense as the task can be preempted anyway so
+// there is no guarantee that the 0 range would be adding much
+// precision - it just removes optimization potential, so it probably
+// never really makes sense.
+//
+// 2) warn if min < 10 or min > 20ms
+//
+// it makes little sense to use a non-atomic call for very short
+// delays because the scheduling jitter will most likely exceed
+// this limit - udelay() makes more sense in that case. For very
+// large delays using hrtimers is useless as preemption becomes
+// quite likely resulting in high inaccuracy anyway - so use
+// jiffies based msleep and don't burden the hrtimer subsystem.
+//
+// 3) warn if max < min
+//
+// Joe Perches <[email protected]> added a check for this case
+// that is definitely wrong.
+//
+// Confidence: Moderate
+// Copyright: (C) 2016 Nicholas Mc Guire, OSADL. GPLv2.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@nullrangectx depends on context@
+expression E1,E2;
+position p;
+@@
+
+* usleep_range@p(E1,E2)
+
+
+@nullrange@
+expression E1,E2;
+position p;
+@@
+
+ usleep_range@p(E1,E2)
+
+@script:python depends on !context@
+p << nullrange.p;
+min << nullrange.E1;
+max << nullrange.E2;
+@@
+
+if(min == max):
+ msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
+ coccilib.report.print_report(p[0], msg)
+if str.isdigit(min):
+ if(int(min) < 10):
+ msg = "ERROR: usleep_range min (%s) less than 10us - consider using udelay()" % (min)
+ coccilib.report.print_report(p[0], msg)
+ if(20000 < int(min)):
+ msg = "ERROR: usleep_range min (%s) exceed 20m - consider using mslee()" % (min)
+ coccilib.report.print_report(p[0], msg)
+ if(int(max) < int(min)):
+ msg = "ERROR: usleep_range max (%s) less than min (%s)" % (max,min)
+ coccilib.report.print_report(p[0], msg)
--
2.1.4
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> Documentation/timers/timers-howto.txt outlines the intended usage of
> usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
> V2: added context mode as suggested by Julia Lawall <[email protected]>
> added min<max case sugested by Joe Perches <[email protected]>
> added in the range checks as they are resonably reliable based on
> a review of all 1648 call sites of usleep_range()
>
> 1648 calls total
> 1488 pass numeric values only (90.29%)
> 27 min below 10us (1.81%)
> 40 min above 10ms (2.68%)
> min out of spec 4.50%
> 76 preprocessor constants (4.61%)
> 1 min below 10us (1.31%)
> 8 min above 10ms (10.52%)
> min out of spec 11.84%
> 85 expressions (5.15%)
> 1(0) min below 10us (1.50%)*
> 6(2) min above 10ms (7.50%)*
> min out of spec 9.0%
> Errors:
> 23 where min==max (1.39%)
> 0 where max < min (0.00%)
>
> Total:
> Bugs: 6.48%-10.70%*
> Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
> Detectable by coccinelle:
> Bugs: 74/103 (71.8%)
> Crit: 50/52 (96.1%)
> * numbers estimated based on code review
>
> Patch is againts 4.9.0 (localversion-next is next-20161214)
>
> scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
> 1 file changed, 88 insertions(+)
> create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
>
> diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> new file mode 100644
> index 0000000..003e9ef
> --- /dev/null
> +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> @@ -0,0 +1,88 @@
> +/// report bad/problematic usleep_range usage
> +//
> +// This is a checker for the documented intended use of usleep_range
> +// see: Documentation/timers/timers-howto.txt and
> +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> +// when mdelay might not be a suitable replacement
> +//
> +// Limitations:
> +// * The numeric limits are only checked when numeric constants are in
> +// use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> +// is done - so this can miss some out-of-range cases - but in 4.9.0
> +// it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> +// (96.1%) of the critical cases (min < 10 and min==max - there
> +// * There may be RT use-cases where both min < 10 and min==max)
> +// justified (e.g. high-throughput drivers on a shielded core)
> +//
> +// 1) warn if min == max
> +//
> +// The problem is that usleep_range is calculating the delay by
> +// exp = ktime_add_us(ktime_get(), min)
> +// delta = (u64)(max - min) * NSEC_PER_USEC
> +// so delta is set to 0 if min==max
> +// and then calls
> +// schedule_hrtimeout_range(exp, 0,...)
> +// effectively this means that the clock subsystem has no room to
> +// optimize. usleep_range() is in non-atomic context so a 0 range
> +// makes very little sense as the task can be preempted anyway so
> +// there is no guarantee that the 0 range would be adding much
> +// precision - it just removes optimization potential, so it probably
> +// never really makes sense.
> +//
> +// 2) warn if min < 10 or min > 20ms
> +//
> +// it makes little sense to use a non-atomic call for very short
> +// delays because the scheduling jitter will most likely exceed
> +// this limit - udelay() makes more sense in that case. For very
> +// large delays using hrtimers is useless as preemption becomes
> +// quite likely resulting in high inaccuracy anyway - so use
> +// jiffies based msleep and don't burden the hrtimer subsystem.
> +//
> +// 3) warn if max < min
> +//
> +// Joe Perches <[email protected]> added a check for this case
> +// that is definitely wrong.
> +//
> +// Confidence: Moderate
> +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL. GPLv2.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual org
> +virtual report
> +virtual context
> +
> +@nullrangectx depends on context@
> +expression E1,E2;
> +position p;
> +@@
> +
> +* usleep_range@p(E1,E2)
This is going to give a context warning on every call to usleep_range.
Why not E1,E1?
> +
> +
> +@nullrange@
> +expression E1,E2;
> +position p;
> +@@
> +
> + usleep_range@p(E1,E2)
> +
> +@script:python depends on !context@
> +p << nullrange.p;
> +min << nullrange.E1;
> +max << nullrange.E2;
> +@@
> +
> +if(min == max):
> + msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> + coccilib.report.print_report(p[0], msg)
> +if str.isdigit(min):
I guess this checks if min is a constant, but doesn't the last case also
need to check if max is a constant?
julia
> + if(int(min) < 10):
> + msg = "ERROR: usleep_range min (%s) less than 10us - consider using udelay()" % (min)
> + coccilib.report.print_report(p[0], msg)
> + if(20000 < int(min)):
> + msg = "ERROR: usleep_range min (%s) exceed 20m - consider using mslee()" % (min)
> + coccilib.report.print_report(p[0], msg)
> + if(int(max) < int(min)):
> + msg = "ERROR: usleep_range max (%s) less than min (%s)" % (max,min)
> + coccilib.report.print_report(p[0], msg)
> --
> 2.1.4
>
>
On Thu, Dec 15, 2016 at 06:52:28AM +0100, Julia Lawall wrote:
>
>
> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
>
> > Documentation/timers/timers-howto.txt outlines the intended usage of
> > usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > ---
> > V2: added context mode as suggested by Julia Lawall <[email protected]>
> > added min<max case sugested by Joe Perches <[email protected]>
> > added in the range checks as they are resonably reliable based on
> > a review of all 1648 call sites of usleep_range()
> >
> > 1648 calls total
> > 1488 pass numeric values only (90.29%)
> > 27 min below 10us (1.81%)
> > 40 min above 10ms (2.68%)
> > min out of spec 4.50%
> > 76 preprocessor constants (4.61%)
> > 1 min below 10us (1.31%)
> > 8 min above 10ms (10.52%)
> > min out of spec 11.84%
> > 85 expressions (5.15%)
> > 1(0) min below 10us (1.50%)*
> > 6(2) min above 10ms (7.50%)*
> > min out of spec 9.0%
> > Errors:
> > 23 where min==max (1.39%)
> > 0 where max < min (0.00%)
> >
> > Total:
> > Bugs: 6.48%-10.70%*
> > Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
> > Detectable by coccinelle:
> > Bugs: 74/103 (71.8%)
> > Crit: 50/52 (96.1%)
> > * numbers estimated based on code review
> >
> > Patch is againts 4.9.0 (localversion-next is next-20161214)
> >
> > scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
> > 1 file changed, 88 insertions(+)
> > create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
> >
> > diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> > new file mode 100644
> > index 0000000..003e9ef
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > @@ -0,0 +1,88 @@
> > +/// report bad/problematic usleep_range usage
> > +//
> > +// This is a checker for the documented intended use of usleep_range
> > +// see: Documentation/timers/timers-howto.txt and
> > +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> > +// when mdelay might not be a suitable replacement
> > +//
> > +// Limitations:
> > +// * The numeric limits are only checked when numeric constants are in
> > +// use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> > +// is done - so this can miss some out-of-range cases - but in 4.9.0
> > +// it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> > +// (96.1%) of the critical cases (min < 10 and min==max - there
> > +// * There may be RT use-cases where both min < 10 and min==max)
> > +// justified (e.g. high-throughput drivers on a shielded core)
> > +//
> > +// 1) warn if min == max
> > +//
> > +// The problem is that usleep_range is calculating the delay by
> > +// exp = ktime_add_us(ktime_get(), min)
> > +// delta = (u64)(max - min) * NSEC_PER_USEC
> > +// so delta is set to 0 if min==max
> > +// and then calls
> > +// schedule_hrtimeout_range(exp, 0,...)
> > +// effectively this means that the clock subsystem has no room to
> > +// optimize. usleep_range() is in non-atomic context so a 0 range
> > +// makes very little sense as the task can be preempted anyway so
> > +// there is no guarantee that the 0 range would be adding much
> > +// precision - it just removes optimization potential, so it probably
> > +// never really makes sense.
> > +//
> > +// 2) warn if min < 10 or min > 20ms
> > +//
> > +// it makes little sense to use a non-atomic call for very short
> > +// delays because the scheduling jitter will most likely exceed
> > +// this limit - udelay() makes more sense in that case. For very
> > +// large delays using hrtimers is useless as preemption becomes
> > +// quite likely resulting in high inaccuracy anyway - so use
> > +// jiffies based msleep and don't burden the hrtimer subsystem.
> > +//
> > +// 3) warn if max < min
> > +//
> > +// Joe Perches <[email protected]> added a check for this case
> > +// that is definitely wrong.
> > +//
> > +// Confidence: Moderate
> > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL. GPLv2.
> > +// Comments:
> > +// Options: --no-includes --include-headers
> > +
> > +virtual org
> > +virtual report
> > +virtual context
> > +
> > +@nullrangectx depends on context@
> > +expression E1,E2;
> > +position p;
> > +@@
> > +
> > +* usleep_range@p(E1,E2)
>
> This is going to give a context warning on every call to usleep_range.
> Why not E1,E1?
yes this triggers on all use of usleep_ranges - as the report mode is
checking for more than just min==max I thought its resonable to simply
report all cases - maybe not.
Not sure if it makes sense to add in the filter from below,
Thre actually are quite a few bad use patters beyond these basic ones
like
unsigned long timeout = jiffies + msecs_to_jiffies(250);
- while (time_before(jiffies, timeout)) {
value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
if (value == MBOX_OWNER_NONE)
break;
- usleep_range(10, 20);
}
>
> > +
> > +
> > +@nullrange@
> > +expression E1,E2;
> > +position p;
> > +@@
> > +
> > + usleep_range@p(E1,E2)
> > +
> > +@script:python depends on !context@
> > +p << nullrange.p;
> > +min << nullrange.E1;
> > +max << nullrange.E2;
> > +@@
> > +
> > +if(min == max):
> > + msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> > + coccilib.report.print_report(p[0], msg)
> > +if str.isdigit(min):
>
> I guess this checks if min is a constant, but doesn't the last case also
> need to check if max is a constant?
>
yes it does - seems that there simply was no
such case so it went unnoticed.
also just noticed that the org mode would also be using
coccilib.report rather than coccilib.org...
thx!
hofrat
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> On Thu, Dec 15, 2016 at 06:52:28AM +0100, Julia Lawall wrote:
> >
> >
> > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> >
> > > Documentation/timers/timers-howto.txt outlines the intended usage of
> > > usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
> > >
> > > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > > ---
> > > V2: added context mode as suggested by Julia Lawall <[email protected]>
> > > added min<max case sugested by Joe Perches <[email protected]>
> > > added in the range checks as they are resonably reliable based on
> > > a review of all 1648 call sites of usleep_range()
> > >
> > > 1648 calls total
> > > 1488 pass numeric values only (90.29%)
> > > 27 min below 10us (1.81%)
> > > 40 min above 10ms (2.68%)
> > > min out of spec 4.50%
> > > 76 preprocessor constants (4.61%)
> > > 1 min below 10us (1.31%)
> > > 8 min above 10ms (10.52%)
> > > min out of spec 11.84%
> > > 85 expressions (5.15%)
> > > 1(0) min below 10us (1.50%)*
> > > 6(2) min above 10ms (7.50%)*
> > > min out of spec 9.0%
> > > Errors:
> > > 23 where min==max (1.39%)
> > > 0 where max < min (0.00%)
> > >
> > > Total:
> > > Bugs: 6.48%-10.70%*
> > > Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
> > > Detectable by coccinelle:
> > > Bugs: 74/103 (71.8%)
> > > Crit: 50/52 (96.1%)
> > > * numbers estimated based on code review
> > >
> > > Patch is againts 4.9.0 (localversion-next is next-20161214)
> > >
> > > scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
> > > 1 file changed, 88 insertions(+)
> > > create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
> > >
> > > diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> > > new file mode 100644
> > > index 0000000..003e9ef
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > > @@ -0,0 +1,88 @@
> > > +/// report bad/problematic usleep_range usage
> > > +//
> > > +// This is a checker for the documented intended use of usleep_range
> > > +// see: Documentation/timers/timers-howto.txt and
> > > +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> > > +// when mdelay might not be a suitable replacement
> > > +//
> > > +// Limitations:
> > > +// * The numeric limits are only checked when numeric constants are in
> > > +// use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> > > +// is done - so this can miss some out-of-range cases - but in 4.9.0
> > > +// it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> > > +// (96.1%) of the critical cases (min < 10 and min==max - there
> > > +// * There may be RT use-cases where both min < 10 and min==max)
> > > +// justified (e.g. high-throughput drivers on a shielded core)
> > > +//
> > > +// 1) warn if min == max
> > > +//
> > > +// The problem is that usleep_range is calculating the delay by
> > > +// exp = ktime_add_us(ktime_get(), min)
> > > +// delta = (u64)(max - min) * NSEC_PER_USEC
> > > +// so delta is set to 0 if min==max
> > > +// and then calls
> > > +// schedule_hrtimeout_range(exp, 0,...)
> > > +// effectively this means that the clock subsystem has no room to
> > > +// optimize. usleep_range() is in non-atomic context so a 0 range
> > > +// makes very little sense as the task can be preempted anyway so
> > > +// there is no guarantee that the 0 range would be adding much
> > > +// precision - it just removes optimization potential, so it probably
> > > +// never really makes sense.
> > > +//
> > > +// 2) warn if min < 10 or min > 20ms
> > > +//
> > > +// it makes little sense to use a non-atomic call for very short
> > > +// delays because the scheduling jitter will most likely exceed
> > > +// this limit - udelay() makes more sense in that case. For very
> > > +// large delays using hrtimers is useless as preemption becomes
> > > +// quite likely resulting in high inaccuracy anyway - so use
> > > +// jiffies based msleep and don't burden the hrtimer subsystem.
> > > +//
> > > +// 3) warn if max < min
> > > +//
> > > +// Joe Perches <[email protected]> added a check for this case
> > > +// that is definitely wrong.
> > > +//
> > > +// Confidence: Moderate
> > > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL. GPLv2.
> > > +// Comments:
> > > +// Options: --no-includes --include-headers
> > > +
> > > +virtual org
> > > +virtual report
> > > +virtual context
> > > +
> > > +@nullrangectx depends on context@
> > > +expression E1,E2;
> > > +position p;
> > > +@@
> > > +
> > > +* usleep_range@p(E1,E2)
> >
> > This is going to give a context warning on every call to usleep_range.
> > Why not E1,E1?
> yes this triggers on all use of usleep_ranges - as the report mode is
> checking for more than just min==max I thought its resonable to simply
> report all cases - maybe not.
> Not sure if it makes sense to add in the filter from below,
I think that simply flagging all the calls for context mode is not a good
idea. Especilly since there is no explanation in this case. If it is too
awkward to support context mode, just drop it.
julia
>
> Thre actually are quite a few bad use patters beyond these basic ones
> like
> unsigned long timeout = jiffies + msecs_to_jiffies(250);
>
> - while (time_before(jiffies, timeout)) {
> value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
> if (value == MBOX_OWNER_NONE)
> break;
>
> - usleep_range(10, 20);
> }
>
> >
> > > +
> > > +
> > > +@nullrange@
> > > +expression E1,E2;
> > > +position p;
> > > +@@
> > > +
> > > + usleep_range@p(E1,E2)
> > > +
> > > +@script:python depends on !context@
> > > +p << nullrange.p;
> > > +min << nullrange.E1;
> > > +max << nullrange.E2;
> > > +@@
> > > +
> > > +if(min == max):
> > > + msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> > > + coccilib.report.print_report(p[0], msg)
> > > +if str.isdigit(min):
> >
> > I guess this checks if min is a constant, but doesn't the last case also
> > need to check if max is a constant?
> >
> yes it does - seems that there simply was no
> such case so it went unnoticed.
>
> also just noticed that the org mode would also be using
> coccilib.report rather than coccilib.org...
>
> thx!
> hofrat
>
>