Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1596192 > unrolled thread

[PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in irqchip implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in irqchip implementations Julia Cartwright <julia@ni.com> - 2017-03-09 17:40 +0100
    Re: [PATCH 01/19] Coccinelle: locks: identify callers of  spin_lock{,_irq,_irqsave}() in irqchip implementations Julia Lawall <julia.lawall@lip6.fr> - 2017-03-09 21:20 +0100
      Re: [PATCH 01/19] Coccinelle: locks: identify callers of  spin_lock{,_irq,_irqsave}() in irqchip implementations Julia Cartwright <julia@ni.com> - 2017-03-09 23:50 +0100

#1596192 — [PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in irqchip implementations

On PREEMPT_RT, the spinlock_t type becomes an object which sleeps under
contention.  The codepaths used to support scheduling (irq dispatching, arch
code, the scheduler, timers) therefore must make use of the
raw_spin_lock{,_irq,_irqsave}() variations which preserve the non-sleeping
spinlock behavior.

Because the irq_chip callbacks are invoked in the process of interrupt
dispatch, they cannot therefore make use of spin_lock_t type.  Instead, the
usage of raw_spinlock_t is appropriate.

Provide a spatch to identify (and attempt to patch) such problematic irqchip
implementations.

Note to those generating patches using this spatch; in order to maintain
correct semantics w/ PREEMPT_RT, it is necessary to audit the
raw_spinlock_t-protected codepaths to ensure their execution is bounded and
minimal.  This is a manual audit process.

See commit 47b03ca903fb0 ("pinctrl: qcom: Use raw spinlock variants") as an
example of _one_ such instance, which fixed a real bug seen in the field.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Julia Cartwright <julia@ni.com>
---
 .../coccinelle/locks/irq_chip_raw_spinlock.cocci   | 96 ++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci

diff --git a/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
new file mode 100644
index 000000000000..73fd4519ae29
--- /dev/null
+++ b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
@@ -0,0 +1,96 @@
+// Copyright: (C) 2017 National Instruments Corp. GPLv2.
+// Author: Julia Cartwright <julia@ni.com>
+//
+// Identify callers of non-raw spinlock_t functions in hardirq irq_chip
+// callbacks.
+//
+// spin_lock{_irq,_irqsave}(), w/ PREEMPT_RT are "sleeping" spinlocks, and so
+// therefore "sleep" under contention; identify (and potentially patch) callers
+// to use raw_spinlock_t instead.
+//
+// Confidence: Moderate
+
+virtual report
+virtual patch
+
+@match@
+identifier __irqchip;
+identifier __irq_mask;
+@@
+	static struct irq_chip __irqchip = {
+		.irq_mask	= __irq_mask,
+	};
+
+@match2 depends on match@
+identifier match.__irq_mask;
+identifier data;
+identifier x;
+identifier l;
+type T;
+position j0;
+expression flags;
+@@
+	static void __irq_mask(struct irq_data *data)
+	{
+		...
+		T *x;
+		...
+(
+		spin_lock_irqsave(&x->l@j0, flags);
+|
+		spin_lock_irq(&x->l@j0);
+|
+		spin_lock(&x->l@j0);
+)
+		...
+	}
+
+@match3 depends on match2 && patch@
+type match2.T;
+identifier match2.l;
+@@
+	T {
+		...
+-		spinlock_t	l;
++		raw_spinlock_t	l;
+		...
+	};
+
+@match4 depends on match2 && patch@
+type match2.T;
+identifier match2.l;
+expression flags;
+T *x;
+@@
+
+(
+-spin_lock(&x->l)
++raw_spin_lock(&x->l)
+|
+-spin_lock_irqsave(&x->l, flags)
++raw_spin_lock_irqsave(&x->l, flags)
+|
+-spin_lock_irq(&x->l)
++raw_spin_lock_irq(&x->l)
+|
+-spin_unlock(&x->l)
++raw_spin_unlock(&x->l)
+|
+-spin_unlock_irq(&x->l)
++raw_spin_unlock_irq(&x->l)
+|
+-spin_unlock_irqrestore(&x->l, flags)
++raw_spin_unlock_irqrestore(&x->l, flags)
+|
+-spin_lock_init(&x->l)
++raw_spin_lock_init(&x->l)
+)
+
+@script:python wat depends on match2 && report@
+j0 << match2.j0;
+t << match2.T;
+l << match2.l;
+@@
+
+msg = "Use of non-raw spinlock is illegal in this context (%s::%s)" % (t, l)
+coccilib.report.print_report(j0[0], msg)
-- 
2.11.1

[toc] | [next] | [standalone]

#1596372 — Re: [PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in irqchip implementations

> +@match2 depends on match@
> +identifier match.__irq_mask;
> +identifier data;
> +identifier x;
> +identifier l;
> +type T;
> +position j0;
> +expression flags;
> +@@
> +	static void __irq_mask(struct irq_data *data)
> +	{
> +		...
> +		T *x;
> +		...
> +(
> +		spin_lock_irqsave(&x->l@j0, flags);
> +|
> +		spin_lock_irq(&x->l@j0);
> +|
> +		spin_lock(&x->l@j0);
> +)
> +		...
> +	}

I guess that here you want a match if there is a lock anywhere in the
function?  Currently, the rule requires that the lock appear on every
control-flow path.  If you put exists after depends on match in the rule
header, it will match if there exists a control-flow patch that contains a
local call.

Also, ... matches the shortest path between the pattern before the ... and
the pattern after.  Thus, x would have to be the first variable in the
function of pointer type.  To eliminate this constraint, put when any on
each of the ...s.  This will additionally allow more than one lock call in
the function.

All in all, I would suggest the following for this rule:

@match2 depends on match exists@
identifier match.__irq_mask;
identifier data;
identifier x;
identifier l;
type T;
position j0;
expression flags;
@@
	static void __irq_mask(struct irq_data *data)
        {
                ... when any
                T *x;
                ... when any
(
                spin_lock_irqsave(&x->l@j0, flags);
|
                spin_lock_irq(&x->l@j0);
|
                spin_lock(&x->l@j0);
)
                ... when any
        }

julia

[toc] | [prev] | [next] | [standalone]

#1596447 — Re: [PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in irqchip implementations

[Multipart message — attachments visible in raw view] — view raw

Hello Julia-

Thanks for the feedback.

On Thu, Mar 09, 2017 at 09:15:21PM +0100, Julia Lawall wrote:
> > +@match2 depends on match@
> > +identifier match.__irq_mask;
> > +identifier data;
> > +identifier x;
> > +identifier l;
> > +type T;
> > +position j0;
> > +expression flags;
> > +@@
> > +	static void __irq_mask(struct irq_data *data)
> > +	{
> > +		...
> > +		T *x;
> > +		...
> > +(
> > +		spin_lock_irqsave(&x->l@j0, flags);
> > +|
> > +		spin_lock_irq(&x->l@j0);
> > +|
> > +		spin_lock(&x->l@j0);
> > +)
> > +		...
> > +	}
> 
> I guess that here you want a match if there is a lock anywhere in the
> function?

Most generally, yes.  Any invocation of spin_lock{,_irq,_irqsave}() in
the irq_mask callback of an irq_chip implementation (irq_mask is only
_one_ such problematic callback, but a fairly representative one).

I should probably introduce a more generic report-mode rule which more
matches spin_lock{,_irq,_irqsave}(e), leaving this rule which requires
the lock accessed through local pointer-indirection only used as a
condition for patch mode.

I'll play with this a bit.

> Currently, the rule requires that the lock appear on every
> control-flow path.  If you put exists after depends on match in the rule
> header, it will match if there exists a control-flow patch that contains a
> local call.

Thanks, this makes sense.

> Also, ... matches the shortest path between the pattern before the ... and
> the pattern after.  Thus, x would have to be the first variable in the
> function of pointer type.  To eliminate this constraint, put when any on
> each of the ...s.  This will additionally allow more than one lock call in
> the function.
> 
> All in all, I would suggest the following for this rule:
> 
> @match2 depends on match exists@
> identifier match.__irq_mask;
> identifier data;
> identifier x;
> identifier l;
> type T;
> position j0;
> expression flags;
> @@
> 	static void __irq_mask(struct irq_data *data)
>         {
>                 ... when any
>                 T *x;
>                 ... when any
> (
>                 spin_lock_irqsave(&x->l@j0, flags);
> |
>                 spin_lock_irq(&x->l@j0);
> |
>                 spin_lock(&x->l@j0);
> )
>                 ... when any
>         }

Great, thanks, Julia!

   - The Other Julia

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web