Message-ID: <20161215061624.GA23684@osadl.at>
Date:   Thu, 15 Dec 2016 06:16:24 +0000
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     Nicholas Mc Guire <hofrat@...dl.org>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <mmarek@...e.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Joe Perches <joe@...ches.com>, cocci@...teme.lip6.fr,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] Coccinelle: check usleep_range() usage

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 <hofrat@...dl.org>
> > ---
> > V2: added context mode as suggested by Julia Lawall <julia.lawall@...6.fr>
> >     added min<max case sugested by Joe Perches <joe@...ches.com>
> >     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 <joe@...ches.com> 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
> > +
> > +@...lrangectx 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);
                }

> 
> > +
> > +
> > +@...lrange@
> > +expression E1,E2;
> > +position p;
> > +@@
> > +
> > +  usleep_range@p(E1,E2)
> > +
> > +@...ipt: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
 

Powered by blists - more mailing lists