[PATCH] Coccinelle: uslee_range: ensure delta not zero

[PATCH] Coccinelle: uslee_range: ensure delta not zero

[Nicholas Mc Guire]

usleep_range() min==max makes little sense at last for non-RT, so issue
a warning if delta is 0.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

As of 4.9.0 this finds about 20 cases - all of which look like the 
should be passing a range.

Patch is against 4.9.0 (localversion-next is next-20161213)

 scripts/coccinelle/api/bad_usleep_range.cocci | 55 +++++++++++++++++++++++++++
 1 file changed, 55 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..7e05f3e
--- /dev/null
+++ b/scripts/coccinelle/api/bad_usleep_range.cocci
@@ -0,0 +1,55 @@
+/// bad uslee_range - 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 for any non-RT systems.
+//
+//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
+//
+// Confidence: Moderate
+// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+
+@nullrange@
+expression E;
+constant C;
+position p;
+@@
+
+<+...
+(
+  usleep_range@p(C,C)
+|
+  usleep_range@p(E,E)
+)
+...+>
+
+
+@script:python depends on org@
+p << nullrange.p;
+range << nullrange.C;
+@@
+
+cocci.print_main("WARNING: inefficient usleep_range with range 0 (min==max)",p)
+
+@script:python depends on report@
+p << nullrange.p;
+@@
+
+coccilib.report.print_report(p[0],"WARNING: inefficient usleep_range with range 0 (min==max)")
+
-- 
2.1.4

Re: [PATCH] Coccinelle: uslee_range: ensure delta not zero

[Julia Lawall]

On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:

> usleep_range() min==max makes little sense at last for non-RT, so issue
> a warning if delta is 0.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> As of 4.9.0 this finds about 20 cases - all of which look like the
> should be passing a range.
>
> Patch is against 4.9.0 (localversion-next is next-20161213)
>
>  scripts/coccinelle/api/bad_usleep_range.cocci | 55 +++++++++++++++++++++++++++
>  1 file changed, 55 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..7e05f3e
> --- /dev/null
> +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> @@ -0,0 +1,55 @@
> +/// bad uslee_range - 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 for any non-RT systems.
> +//
> +//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
> +//
> +// Confidence: Moderate
> +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual org
> +virtual report
> +
> +@nullrange@
> +expression E;
> +constant C;
> +position p;
> +@@
> +
> +<+...
> +(
> +  usleep_range@p(C,C)
> +|
> +  usleep_range@p(E,E)
> +)
> +...+>

The outer <+... ...+> is not needed.

You could support context too.

The E,E case subsumes the C,C case.  Unless you want to put different
messages for the two cases, there is no need for both of them.

julia

> +
> +
> +@script:python depends on org@
> +p << nullrange.p;
> +range << nullrange.C;
> +@@
> +
> +cocci.print_main("WARNING: inefficient usleep_range with range 0 (min==max)",p)
> +
> +@script:python depends on report@
> +p << nullrange.p;
> +@@
> +
> +coccilib.report.print_report(p[0],"WARNING: inefficient usleep_range with range 0 (min==max)")
> +
> --
> 2.1.4
>
>

Re: [PATCH] Coccinelle: uslee_range: ensure delta not zero

[Nicholas Mc Guire]

On Tue, Dec 13, 2016 at 01:09:38PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:
> 
> > usleep_range() min==max makes little sense at last for non-RT, so issue
> > a warning if delta is 0.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >
> > As of 4.9.0 this finds about 20 cases - all of which look like the
> > should be passing a range.
> >
> > Patch is against 4.9.0 (localversion-next is next-20161213)
> >
> >  scripts/coccinelle/api/bad_usleep_range.cocci | 55 +++++++++++++++++++++++++++
> >  1 file changed, 55 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..7e05f3e
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > @@ -0,0 +1,55 @@
> > +/// bad uslee_range - 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 for any non-RT systems.
> > +//
> > +//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
> > +//
> > +// Confidence: Moderate
> > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> > +// Comments:
> > +// Options: --no-includes --include-headers
> > +
> > +virtual org
> > +virtual report
> > +
> > +@nullrange@
> > +expression E;
> > +constant C;
> > +position p;
> > +@@
> > +
> > +<+...
> > +(
> > +  usleep_range@p(C,C)
> > +|
> > +  usleep_range@p(E,E)
> > +)
> > +...+>
> 
> The outer <+... ...+> is not needed.
> 
> You could support context too.
> 
> The E,E case subsumes the C,C case.  Unless you want to put different
> messages for the two cases, there is no need for both of them.
>
ok will kick that - in my script I was also doing range checks
on constants to distinuish between the udelay range < 10us and
the msleep* range > 10ms - but that is not as clear a case as the
min==max case - will remove the constant case - the unnecessary
<+... ...+> just means I still did not understand when it is actually needed
will remove that as well.

thx!
hofrat

Re: [PATCH] Coccinelle: uslee_range: ensure delta not zero

[Julia Lawall]

On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:

> On Tue, Dec 13, 2016 at 01:09:38PM +0100, Julia Lawall wrote:
> >
> >
> > On Tue, 13 Dec 2016, Nicholas Mc Guire wrote:
> >
> > > usleep_range() min==max makes little sense at last for non-RT, so issue
> > > a warning if delta is 0.
> > >
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > ---
> > >
> > > As of 4.9.0 this finds about 20 cases - all of which look like the
> > > should be passing a range.
> > >
> > > Patch is against 4.9.0 (localversion-next is next-20161213)
> > >
> > >  scripts/coccinelle/api/bad_usleep_range.cocci | 55 +++++++++++++++++++++++++++
> > >  1 file changed, 55 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..7e05f3e
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > > @@ -0,0 +1,55 @@
> > > +/// bad uslee_range - 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 for any non-RT systems.
> > > +//
> > > +//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
> > > +//
> > > +// Confidence: Moderate
> > > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> > > +// Comments:
> > > +// Options: --no-includes --include-headers
> > > +
> > > +virtual org
> > > +virtual report
> > > +
> > > +@nullrange@
> > > +expression E;
> > > +constant C;
> > > +position p;
> > > +@@
> > > +
> > > +<+...
> > > +(
> > > +  usleep_range@p(C,C)
> > > +|
> > > +  usleep_range@p(E,E)
> > > +)
> > > +...+>
> >
> > The outer <+... ...+> is not needed.
> >
> > You could support context too.
> >
> > The E,E case subsumes the C,C case.  Unless you want to put different
> > messages for the two cases, there is no need for both of them.
> >
> ok will kick that - in my script I was also doing range checks
> on constants to distinuish between the udelay range < 10us and
> the msleep* range > 10ms - but that is not as clear a case as the
> min==max case - will remove the constant case - the unnecessary
> <+... ...+> just means I still did not understand when it is actually needed
> will remove that as well.

<+... ...+> is most useful when inside { }

julia