[mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[KOSAKI Motohiro]

Applied after: linux-next.patch

==
Subject: [mmotm][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

commit 08678b0841267c1d00d771fe01548d86043d065e introduced
irq_desc_lock_class variable.
But it is used only if CONFIG_LOCKDEP=Y.
otherwise, following warnings happend.

	  CC      kernel/irq/handle.o
	kernel/irq/handle.c:26: warning: 'irq_desc_lock_class' defined but not used


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yinghai Lu <yhlu.kernel@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
---
 kernel/irq/handle.c |    2 ++
 1 file changed, 2 insertions(+)

Index: b/kernel/irq/handle.c
===================================================================
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -20,10 +20,12 @@
 
 #include "internals.h"
 
+#ifdef CONFIG_LOCKDEP
 /*
  * lockdep: we want to handle all irq_desc locks as a single lock-class:
  */
 static struct lock_class_key irq_desc_lock_class;
+#endif
 
 /**
  * handle_bad_irq - handle spurious and unhandled irqs

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Andrew Morton]

On Tue, 16 Dec 2008 17:08:43 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> 
> 
> Applied after: linux-next.patch
> 
> ==
> Subject: [mmotm][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP
> 
> commit 08678b0841267c1d00d771fe01548d86043d065e introduced
> irq_desc_lock_class variable.
> But it is used only if CONFIG_LOCKDEP=Y.
> otherwise, following warnings happend.
> 
> 	  CC      kernel/irq/handle.o
> 	kernel/irq/handle.c:26: warning: 'irq_desc_lock_class' defined but not used
> 
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Yinghai Lu <yhlu.kernel@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/irq/handle.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: b/kernel/irq/handle.c
> ===================================================================
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -20,10 +20,12 @@
>  
>  #include "internals.h"
>  
> +#ifdef CONFIG_LOCKDEP
>  /*
>   * lockdep: we want to handle all irq_desc locks as a single lock-class:
>   */
>  static struct lock_class_key irq_desc_lock_class;
> +#endif
>  
>  /**
>   * handle_bad_irq - handle spurious and unhandled irqs
> 

No, lockdep.h (which we forgot to include) already handles that:

# define lockdep_set_class(lock, key)		do { (void)(key); } while (0)

the problem is that the code which references irq_desc_lock_class is
inside #ifdef CONFIG_SPARSE_IRQ, so this is a better fix:


--- a/kernel/irq/handle.c~irq-enclose-irq_desc_lock_class-in-config_lockdep
+++ a/kernel/irq/handle.c
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/random.h>
+#include <linux/lockdep.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
 #include <linux/rculist.h>
@@ -20,11 +21,6 @@
 
 #include "internals.h"
 
-/*
- * lockdep: we want to handle all irq_desc locks as a single lock-class:
- */
-static struct lock_class_key irq_desc_lock_class;
-
 /**
  * handle_bad_irq - handle spurious and unhandled irqs
  * @irq:       the interrupt number
@@ -61,6 +57,12 @@ void __init __attribute__((weak)) arch_e
 }
 
 #ifdef CONFIG_SPARSE_IRQ
+
+/*
+ * lockdep: we want to handle all irq_desc locks as a single lock-class:
+ */
+static struct lock_class_key irq_desc_lock_class;
+
 static struct irq_desc irq_desc_init = {
 	.irq	    = -1,
 	.status	    = IRQ_DISABLED,
_

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 16 Dec 2008 17:08:43 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > 
> > 
> > Applied after: linux-next.patch
> > 
> > ==
> > Subject: [mmotm][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP
> > 
> > commit 08678b0841267c1d00d771fe01548d86043d065e introduced
> > irq_desc_lock_class variable.
> > But it is used only if CONFIG_LOCKDEP=Y.
> > otherwise, following warnings happend.
> > 
> > 	  CC      kernel/irq/handle.o
> > 	kernel/irq/handle.c:26: warning: 'irq_desc_lock_class' defined but not used
> > 
> > 
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > CC: Yinghai Lu <yhlu.kernel@gmail.com>
> > CC: Ingo Molnar <mingo@elte.hu>
> > ---
> >  kernel/irq/handle.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > Index: b/kernel/irq/handle.c
> > ===================================================================
> > --- a/kernel/irq/handle.c
> > +++ b/kernel/irq/handle.c
> > @@ -20,10 +20,12 @@
> >  
> >  #include "internals.h"
> >  
> > +#ifdef CONFIG_LOCKDEP
> >  /*
> >   * lockdep: we want to handle all irq_desc locks as a single lock-class:
> >   */
> >  static struct lock_class_key irq_desc_lock_class;
> > +#endif
> >  
> >  /**
> >   * handle_bad_irq - handle spurious and unhandled irqs
> > 
> 
> No, lockdep.h (which we forgot to include) already handles that:
> 
> # define lockdep_set_class(lock, key)		do { (void)(key); } while (0)
> 
> the problem is that the code which references irq_desc_lock_class is
> inside #ifdef CONFIG_SPARSE_IRQ, so this is a better fix:

agreed that this is the better fix - applied to tip/irq/sparseirq, thanks!

	Ingo

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > >  #include "internals.h"
> > >  
> > > +#ifdef CONFIG_LOCKDEP
> > >  /*
> > >   * lockdep: we want to handle all irq_desc locks as a single lock-class:
> > >   */
> > >  static struct lock_class_key irq_desc_lock_class;
> > > +#endif
> > >  
> > >  /**
> > >   * handle_bad_irq - handle spurious and unhandled irqs
> > > 
> > 
> > No, lockdep.h (which we forgot to include) already handles that:
> > 
> > # define lockdep_set_class(lock, key)		do { (void)(key); } while (0)
> > 
> > the problem is that the code which references irq_desc_lock_class is
> > inside #ifdef CONFIG_SPARSE_IRQ, so this is a better fix:
> 
> agreed that this is the better fix - applied to tip/irq/sparseirq, 
> thanks!

actually, this breaks the build on !SPARSEIRQ because we will use that 
class in the non-sparseirq case. So we've converted a build warning to a 
build failure ;-)

	Ingo

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[KOSAKI Motohiro]

Hi!

>> > No, lockdep.h (which we forgot to include) already handles that:
>> >
>> > # define lockdep_set_class(lock, key)               do { (void)(key); } while (0)
>> >
>> > the problem is that the code which references irq_desc_lock_class is
>> > inside #ifdef CONFIG_SPARSE_IRQ, so this is a better fix:
>>
>> agreed that this is the better fix - applied to tip/irq/sparseirq,
>> thanks!
>
> actually, this breaks the build on !SPARSEIRQ because we will use that
> class in the non-sparseirq case. So we've converted a build warning to a
> build failure ;-)

Please give me your .config and tell me your arch.
my ia64 box (ia64 is !SPARSEIRQ) can build the akpm patch.

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> Hi!
> 
> >> > No, lockdep.h (which we forgot to include) already handles that:
> >> >
> >> > # define lockdep_set_class(lock, key)               do { (void)(key); } while (0)
> >> >
> >> > the problem is that the code which references irq_desc_lock_class is
> >> > inside #ifdef CONFIG_SPARSE_IRQ, so this is a better fix:
> >>
> >> agreed that this is the better fix - applied to tip/irq/sparseirq,
> >> thanks!
> >
> > actually, this breaks the build on !SPARSEIRQ because we will use that 
> > class in the non-sparseirq case. So we've converted a build warning to 
> > a build failure ;-)
> 
> Please give me your .config and tell me your arch. my ia64 box (ia64 is 
> !SPARSEIRQ) can build the akpm patch.

The expected build failure is obvious from reading the code:

 #ifdef CONFIG_TRACE_IRQFLAGS
 void early_init_irq_lock_class(void)
 {
 #ifndef CONFIG_SPARSE_IRQ
         struct irq_desc *desc;
         int i;

         for_each_irq_desc(i, desc) {
                 if (!desc)
                         continue;

                 lockdep_set_class(&desc->lock, &irq_desc_lock_class);

Note that it's an #ifndef sparseirq, not an #ifdef sparseirq condition.

	Ingo

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[KOSAKI Motohiro]

>> > actually, this breaks the build on !SPARSEIRQ because we will use that
>> > class in the non-sparseirq case. So we've converted a build warning to
>> > a build failure ;-)
>>
>> Please give me your .config and tell me your arch. my ia64 box (ia64 is
>> !SPARSEIRQ) can build the akpm patch.
>
> The expected build failure is obvious from reading the code:
>
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  void early_init_irq_lock_class(void)
>  {
>  #ifndef CONFIG_SPARSE_IRQ
>         struct irq_desc *desc;
>         int i;
>
>         for_each_irq_desc(i, desc) {
>                 if (!desc)
>                         continue;
>
>                 lockdep_set_class(&desc->lock, &irq_desc_lock_class);
>
> Note that it's an #ifndef sparseirq, not an #ifdef sparseirq condition.

I see. thanks.
It seems my first proposal is better.

or, following #ifdef ?

#if defined(CONFIG_SPARSE_IRQ) || defined(CONFIG_TRACE_IRQFLAGS)

 /*
 * lockdep: we want to handle all irq_desc locks as a single lock-class:
 */
 static struct lock_class_key irq_desc_lock_class;

#endif

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> >> > actually, this breaks the build on !SPARSEIRQ because we will use that
> >> > class in the non-sparseirq case. So we've converted a build warning to
> >> > a build failure ;-)
> >>
> >> Please give me your .config and tell me your arch. my ia64 box (ia64 is
> >> !SPARSEIRQ) can build the akpm patch.
> >
> > The expected build failure is obvious from reading the code:
> >
> >  #ifdef CONFIG_TRACE_IRQFLAGS
> >  void early_init_irq_lock_class(void)
> >  {
> >  #ifndef CONFIG_SPARSE_IRQ
> >         struct irq_desc *desc;
> >         int i;
> >
> >         for_each_irq_desc(i, desc) {
> >                 if (!desc)
> >                         continue;
> >
> >                 lockdep_set_class(&desc->lock, &irq_desc_lock_class);
> >
> > Note that it's an #ifndef sparseirq, not an #ifdef sparseirq condition.
> 
> I see. thanks.
> It seems my first proposal is better.
> 
> or, following #ifdef ?
> 
> #if defined(CONFIG_SPARSE_IRQ) || defined(CONFIG_TRACE_IRQFLAGS)
> 
>  /*
>  * lockdep: we want to handle all irq_desc locks as a single lock-class:
>  */
>  static struct lock_class_key irq_desc_lock_class;

instead of increasing the #ifdef jungle, how about removing some? For 
example is this distinction:

> >  #ifndef CONFIG_SPARSE_IRQ

really needed? We should use symmetric lock class annotations, regardless 
of how irq_desc[] is laid out.

	Ingo

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[KOSAKI Motohiro]

>> or, following #ifdef ?
>>
>> #if defined(CONFIG_SPARSE_IRQ) || defined(CONFIG_TRACE_IRQFLAGS)
>>
>>  /*
>>  * lockdep: we want to handle all irq_desc locks as a single lock-class:
>>  */
>>  static struct lock_class_key irq_desc_lock_class;
>
> instead of increasing the #ifdef jungle, how about removing some? For
> example is this distinction:
>
>> >  #ifndef CONFIG_SPARSE_IRQ
>
> really needed? We should use symmetric lock class annotations, regardless
> of how irq_desc[] is laid out.

it seems make much sense. I'll test your idea tommorow.

thanks.

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[KOSAKI Motohiro]

> >> or, following #ifdef ?
> >>
> >> #if defined(CONFIG_SPARSE_IRQ) || defined(CONFIG_TRACE_IRQFLAGS)
> >>
> >>  /*
> >>  * lockdep: we want to handle all irq_desc locks as a single lock-class:
> >>  */
> >>  static struct lock_class_key irq_desc_lock_class;
> >
> > instead of increasing the #ifdef jungle, how about removing some? For
> > example is this distinction:
> >
> >> >  #ifndef CONFIG_SPARSE_IRQ
> >
> > really needed? We should use symmetric lock class annotations, regardless
> > of how irq_desc[] is laid out.
> 
> it seems make much sense. I'll test your idea tommorow.

Ingo, you are right. I confirmed your idea works well.


I tested following ten pattern.

o handle.c can compile without any warnings?

SPARSE_IRQ	TRACE_IRQ	LOCKDEP
------------------------------------------
n		n		n
Y		n		n
n		Y		n
n		n		Y
Y		Y		n
N		Y		Y
Y		n		Y
Y		Y		Y


o builded kernel works well? (tested on x86_64)

SPARSE_IRQ	TRACE_IRQ	LOCKDEP
------------------------------------------
n		n		n
Y		Y		Y


==
Subject: [PATCH] irq: remove unnecessary ifdef

commit 08678b0841267c1d00d771fe01548d86043d065e introduced
irq_desc_lock_class variable.

But it is used only if CONFIG_SPARSE_IRQ=Y or CONFIG_TRACE_IRQFLAGS=Y.
otherwise, following warnings happend.

	CC      kernel/irq/handle.o
	kernel/irq/handle.c:26: warning: 'irq_desc_lock_class' defined but not used

Actually, current early_init_irq_lock_class has a bit strange and messy ifdef.
In addition, it is not valueable.

1. this function protected by !CONFIG_SPARSE_IRQ. but it is not necessary.
   if CONFIG_SPARSE_IRQ=Y, desc of all irq number are initialized by NULL at first.
   then this function calling is safe.

2. this function protected by CONFIG_TRACE_IRQFLAGS too. but it is not necessary too.
   because lockdep_set_class() doesn't have bad side effect even if CONFIG_TRACE_IRQFLAGS=n, 

This patch bloat kernel size a bit on CONFIG_TRACE_IRQFLAGS=n and CONFIG_SPARSE_IRQ=Y.
that's ok. early_init_irq_lock_class() is not fastpatch at all.

To avoid ifdef messy is important than few byte diet.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yinghai Lu <yhlu.kernel@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
---
 include/linux/lockdep.h |    2 +-
 kernel/irq/handle.c     |    5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

Index: b/kernel/irq/handle.c
===================================================================
--- a/kernel/irq/handle.c	2008-12-17 10:45:56.000000000 +0900
+++ b/kernel/irq/handle.c	2008-12-17 17:26:42.000000000 +0900
@@ -417,11 +417,8 @@ out:
 }
 #endif
 
-
-#ifdef CONFIG_TRACE_IRQFLAGS
 void early_init_irq_lock_class(void)
 {
-#ifndef CONFIG_SPARSE_IRQ
 	struct irq_desc *desc;
 	int i;
 
@@ -431,9 +428,7 @@ void early_init_irq_lock_class(void)
 
 		lockdep_set_class(&desc->lock, &irq_desc_lock_class);
 	}
-#endif
 }
-#endif
 
 #ifdef CONFIG_SPARSE_IRQ
 unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
Index: b/include/linux/lockdep.h
===================================================================
--- a/include/linux/lockdep.h	2008-12-17 10:46:26.000000000 +0900
+++ b/include/linux/lockdep.h	2008-12-17 17:27:18.000000000 +0900
@@ -407,7 +407,7 @@ do {								\
 
 #endif /* CONFIG_LOCKDEP */
 
-#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_GENERIC_HARDIRQS)
+#ifdef CONFIG_GENERIC_HARDIRQS
 extern void early_init_irq_lock_class(void);
 #else
 static inline void early_init_irq_lock_class(void)

Re: [mmotm and linux-next][PATCH] irq: enclose irq_desc_lock_class in CONFIG_LOCKDEP

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > >> or, following #ifdef ?
> > >>
> > >> #if defined(CONFIG_SPARSE_IRQ) || defined(CONFIG_TRACE_IRQFLAGS)
> > >>
> > >>  /*
> > >>  * lockdep: we want to handle all irq_desc locks as a single lock-class:
> > >>  */
> > >>  static struct lock_class_key irq_desc_lock_class;
> > >
> > > instead of increasing the #ifdef jungle, how about removing some? For
> > > example is this distinction:
> > >
> > >> >  #ifndef CONFIG_SPARSE_IRQ
> > >
> > > really needed? We should use symmetric lock class annotations, regardless
> > > of how irq_desc[] is laid out.
> > 
> > it seems make much sense. I'll test your idea tommorow.
> 
> Ingo, you are right. I confirmed your idea works well.
> 
> 
> I tested following ten pattern.
> 
> o handle.c can compile without any warnings?
> 
> SPARSE_IRQ	TRACE_IRQ	LOCKDEP
> ------------------------------------------
> n		n		n
> Y		n		n
> n		Y		n
> n		n		Y
> Y		Y		n
> N		Y		Y
> Y		n		Y
> Y		Y		Y
> 
> 
> o builded kernel works well? (tested on x86_64)
> 
> SPARSE_IRQ	TRACE_IRQ	LOCKDEP
> ------------------------------------------
> n		n		n
> Y		Y		Y
> 
> 
> ==
> Subject: [PATCH] irq: remove unnecessary ifdef

Applied to tip/irq/sparseirq, thanks!

	Ingo