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@xxxxxxxxxxxxxx>
CC: Yinghai Lu <yhlu.kernel@xxxxxxxxx>
CC: Ingo Molnar <mingo@xxxxxxx>
---
 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)




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/