irq_enter/irq_exit consolidation

PATCH In review Christoph Hellwig 3 participants 4 messages Sep 12, 2004 – Sep 13, 2004

Share & tools

Christoph Hellwig

author

2004-09-12 11:32 UTC #0 lore raw

PATCH 1/1 irq_enter/irq_exit consolidation

Move irq_enter/irq_exit from <asm/hardirq.h> to <linux/hardirq.h>. There some fishy things going on with the do_softirq invokation on arm, arm26 and s390.

arm calls __do_softirq directly without local irq disabling which looks like a real bug to me.

arm26 has a duplicate __do_softirq entry point (which makes me wonder whether it still compiles with any 2.6 tree since a few month)

s390 has an assembly wrapper around do_softirq.

I've extended the invoke_softirq mechanism used by s390 (also called by ksoftirqd) to the two arm variants, but the right thing to do is probably to use the normal do_softirq call in arm and set __ARCH_HAS_DO_SOFTIRQ + providing a per-arch do_softirq for all callers for s390 and maybe arm26.

arch/ia64/kernel/irq_ia64.c +0 ~~-15~~

@@ -59,21 +59,6 @@
 };
 EXPORT_SYMBOL(isa_irq_to_vector_map);
 
-static inline void
-irq_enter (void)
-{
-	preempt_count() += HARDIRQ_OFFSET;
-}
-
-static inline void
-irq_exit (void)
-{
-	preempt_count() -= IRQ_EXIT_OFFSET;
-	if (!in_interrupt() && local_softirq_pending())
-		do_softirq();
-	preempt_enable_no_resched();
-}
-
 int
 assign_irq_vector (int irq)
 {

===== include/asm-alpha/hardirq.h 1.9 vs edited =====

include/asm-alpha/hardirq.h +0 ~~-10~~

@@ -50,14 +50,4 @@
 #error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()						\
-do {								\
-		preempt_count() -= IRQ_EXIT_OFFSET;		\
-		if (!in_interrupt() &&				\
-		    softirq_pending(smp_processor_id()))	\
-			do_softirq();				\
-		preempt_enable_no_resched();			\
-} while (0)
-
 #endif /* _ALPHA_HARDIRQ_H */

===== include/asm-arm/hardirq.h 1.13 vs edited =====

include/asm-arm/hardirq.h +1 ~~-12~~

@@ -50,18 +50,7 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-
-#ifndef CONFIG_SMP
 extern asmlinkage void __do_softirq(void);
-
-#define irq_exit()							\
-	do {								\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && local_softirq_pending())		\
-			__do_softirq();					\
-		preempt_enable_no_resched();				\
-	} while (0)
-#endif
+#define invoke_softirq()	__do_softirq()
 
 #endif /* __ASM_HARDIRQ_H */

include/asm-arm26/hardirq.h +1 ~~-12~~

@@ -41,17 +41,6 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-
-#ifndef CONFIG_SMP
-#define irq_exit()							\
-	do {								\
-		preempt_count() -= HARDIRQ_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			__asm__("bl%? __do_softirq": : : "lr");/* out of line */\
-		preempt_enable_no_resched();				\
-	} while (0)
-
-#endif
+#define invoke_softirq()	__asm__("bl%? __do_softirq": : : "lr")
 
 #endif /* __ASM_HARDIRQ_H */

===== include/asm-cris/hardirq.h 1.4 vs edited =====

include/asm-cris/hardirq.h +0 -9

@@ -49,13 +49,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */

===== include/asm-h8300/hardirq.h 1.5 vs edited =====

include/asm-h8300/hardirq.h +0 -9

@@ -47,13 +47,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif

===== include/asm-i386/hardirq.h 1.22 vs edited =====

include/asm-i386/hardirq.h +0 ~~-12~~

@@ -46,16 +46,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define nmi_enter()		(irq_enter())
-#define nmi_exit()		(preempt_count() -= HARDIRQ_OFFSET)
-
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */

===== include/asm-m68k/hardirq.h 1.7 vs edited =====

include/asm-m68k/hardirq.h +0 -9

@@ -44,13 +44,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif

===== include/asm-m68knommu/hardirq.h 1.4 vs edited =====

include/asm-m68knommu/hardirq.h +0 -9

@@ -45,13 +45,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __M68K_HARDIRQ_H */

===== include/asm-mips/hardirq.h 1.9 vs edited =====

include/asm-mips/hardirq.h +0 -9

@@ -52,13 +52,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()                                                     \
-do {                                                                   \
-	preempt_count() -= IRQ_EXIT_OFFSET;                     \
-	if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-		do_softirq();                                   \
-	preempt_enable_no_resched();                            \
-} while (0)
-
 #endif /* _ASM_HARDIRQ_H */

===== include/asm-parisc/hardirq.h 1.4 vs edited =====

include/asm-parisc/hardirq.h +0 -9

@@ -59,13 +59,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()								\
-do {										\
-		preempt_count() -= IRQ_EXIT_OFFSET;				\
-		if (!in_interrupt() && softirq_pending(smp_processor_id()))	\
-			do_softirq();						\
-		preempt_enable_no_resched();					\
-} while (0)
-
 #endif /* _PARISC_HARDIRQ_H */

===== include/asm-ppc/hardirq.h 1.25 vs edited =====

include/asm-ppc/hardirq.h +0 -9

@@ -53,14 +53,5 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-	preempt_count() -= IRQ_EXIT_OFFSET;				\
-	if (!in_interrupt() && softirq_pending(smp_processor_id()))	\
-		do_softirq();						\
-	preempt_enable_no_resched();					\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */
 #endif /* __KERNEL__ */

===== include/asm-ppc64/hardirq.h 1.14 vs edited =====

include/asm-ppc64/hardirq.h +0 -9

@@ -51,13 +51,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */

===== include/asm-s390/hardirq.h 1.14 vs edited =====

include/asm-s390/hardirq.h +0 ~~-13~~

@@ -66,17 +66,4 @@
 
 #define invoke_softirq() do_call_softirq()
 
-#define irq_enter()							\
-do {									\
-	(preempt_count() += HARDIRQ_OFFSET);				\
-} while(0)
-#define irq_exit()							\
-do {									\
-	preempt_count() -= IRQ_EXIT_OFFSET;				\
-	if (!in_interrupt() && local_softirq_pending())			\
-		/* Use the async. stack for softirq */			\
-		do_call_softirq();					\
-	preempt_enable_no_resched();					\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */

===== include/asm-sh/hardirq.h 1.7 vs edited =====

include/asm-sh/hardirq.h +0 ~~-12~~

@@ -44,16 +44,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define nmi_enter()		(irq_enter())
-#define nmi_exit()		(preempt_count() -= HARDIRQ_OFFSET)
-
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __ASM_SH_HARDIRQ_H */

===== include/asm-sparc/hardirq.h 1.15 vs edited =====

include/asm-sparc/hardirq.h +0 -9

@@ -42,13 +42,4 @@
 #define SOFTIRQ_SHIFT   (PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_SHIFT   (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
 
-#define irq_enter()             (preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()                                                      \
-do {                                                                    \
-                preempt_count() -= IRQ_EXIT_OFFSET;                     \
-                if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-                        do_softirq();                                   \
-                preempt_enable_no_resched();                            \
-} while (0)
-
 #endif /* __SPARC_HARDIRQ_H */

===== include/asm-sparc64/hardirq.h 1.19 vs edited =====

include/asm-sparc64/hardirq.h +0 -9

@@ -41,13 +41,4 @@
 #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* !(__SPARC64_HARDIRQ_H) */

===== include/asm-v850/hardirq.h 1.5 vs edited =====

include/asm-v850/hardirq.h +0 -9

@@ -45,13 +45,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							      \
-do {									      \
-	preempt_count() -= IRQ_EXIT_OFFSET;				      \
-	if (!in_interrupt() && softirq_pending(smp_processor_id()))	      \
-		do_softirq();						      \
-	preempt_enable_no_resched();					      \
-} while (0)
-
 #endif /* __V850_HARDIRQ_H__ */

===== include/asm-x86_64/hardirq.h 1.7 vs edited =====

include/asm-x86_64/hardirq.h +0 ~~-12~~

@@ -46,16 +46,4 @@
 # error HARDIRQ_BITS is too low!
 #endif
 
-#define nmi_enter()		(irq_enter())
-#define nmi_exit()		(preempt_count() -= HARDIRQ_OFFSET)
-
-#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
-#define irq_exit()							\
-do {									\
-		preempt_count() -= IRQ_EXIT_OFFSET;			\
-		if (!in_interrupt() && softirq_pending(smp_processor_id())) \
-			do_softirq();					\
-		preempt_enable_no_resched();				\
-} while (0)
-
 #endif /* __ASM_HARDIRQ_H */

===== include/linux/hardirq.h 1.1 vs edited =====

include/linux/hardirq.h +16 -0

@@ -41,6 +36,22 @@
 # define preemptible()	0
 # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
 #endif
+
+#ifndef invoke_softirq
+#define invoke_softirq() do_softirq()
+#endif
+
+#define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
+#define irq_exit()						\
+do {								\
+	preempt_count() -= IRQ_EXIT_OFFSET;			\
+	if (!in_interrupt() && local_softirq_pending())		\
+		invoke_softirq();				\
+	preempt_enable_no_resched();				\
+} while (0)
+
+#define nmi_enter()		(preempt_count() += HARDIRQ_OFFSET)
+#define nmi_exit()		(preempt_count() -= HARDIRQ_OFFSET)
 
 #ifdef CONFIG_SMP
 extern void synchronize_irq(unsigned int irq);

===== include/linux/interrupt.h 1.30 vs edited =====

include/linux/interrupt.h +0 -5

@@ -100,11 +100,6 @@
 extern void FASTCALL(raise_softirq_irqoff(unsigned int nr));
 extern void FASTCALL(raise_softirq(unsigned int nr));
 
-#ifndef invoke_softirq
-#define invoke_softirq() do_softirq()
-#endif
-
-
 /* Tasklets --- multithreaded analogue of BHs.
 
    Main feature differing them of generic softirqs: tasklet

Russell King

2004-09-12 11:52 UTC #1 lore raw

Quoted 6 lines · Christoph Hellwig

Move irq_enter/irq_exit from <asm/hardirq.h> to <linux/hardirq.h>. There some fishy things going on with the do_softirq invokation on arm, arm26 and s390.
arm calls __do_softirq directly without local irq disabling which looks like a real bug to me.

The ARM interrupt subsystem guarantees that local IRQs are disabled prior to irq_exit() being invoked. This is a must for reasons other than softirq semantics - hardware IRQ controllers may require a nonatomic read-modify-write to update their IRQ masking state.

This guarantee must also exist on every other architecture, otherwise:

include/linux/hardirq.h line 47

Quoted 6 lines

===== include/linux/hardirq.h 1.1 vs edited =====
include/linux/hardirq.h +3 -0
+#define irq_exit()						\
+do {								\
+	preempt_count() -= IRQ_EXIT_OFFSET;			\

would be buggy - it's an inherently non-atomic operation. Not only that but the behaviour of nested interrupts would change depending on whether they interrupted before or after preempt count has been updated.

So maybe every other architecture except ARM is buggy? 8)

Signature · 4 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

David S. Miller

2004-09-12 22:59 UTC #2 lore raw

Quoted 10 lines · On Sun, 12 Sep 2004 12:44:48 +0100 Russell King <[email protected]>…

This guarantee must also exist on every other architecture, otherwise:
include/linux/hardirq.h line 47
Quoted 6 lines
===== include/linux/hardirq.h 1.1 vs edited =====
include/linux/hardirq.h +3 -0
+#define irq_exit()						\
+do {								\
+	preempt_count() -= IRQ_EXIT_OFFSET;			\
would be buggy - it's an inherently non-atomic operation.

It works out actually, if we take an interrupt in the middle of the operation, that's fine because the preemption count will be precisely the same as we first read it by the time we return from that interrupt, work out some example cases as I think that makes it easier to understand.

Russell King

2004-09-13 08:11 UTC #3 lore raw

Quoted 14 lines · David S. Miller

Quoted 10 lines · On Sun, 12 Sep 2004 12:44:48 +0100 Russell King <[email protected]>…
This guarantee must also exist on every other architecture, otherwise:
include/linux/hardirq.h line 47
Quoted 6 lines
===== include/linux/hardirq.h 1.1 vs edited =====
include/linux/hardirq.h +3 -0
+#define irq_exit()						\
+do {								\
+	preempt_count() -= IRQ_EXIT_OFFSET;			\
would be buggy - it's an inherently non-atomic operation.

Quoted 5 lines

It works out actually, if we take an interrupt in the middle of the operation, that's fine because the preemption count will be precisely the same as we first read it by the time we return from that interrupt, work out some example cases as I think that makes it easier to understand.

I realise that, and it's precisely why I wrote the sentence following the one you quoted above.

However, ARM ain't buggy whatever.

Signature · 4 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core