[PATCH] genirq: ARM dyntick cleanup

[PATCH] genirq: ARM dyntick cleanup

[Thomas Gleixner]

Linus: "The hacks in kernel/irq/handle.c are really horrid. REALLY
horrid."

They are indeed. Move the dyntick quirks to ARM where they belong.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Index: linux-2.6.git/include/asm-arm/hw_irq.h
===================================================================
--- linux-2.6.git.orig/include/asm-arm/hw_irq.h	2006-07-03 00:13:24.000000000 +0200
+++ linux-2.6.git/include/asm-arm/hw_irq.h	2006-07-03 00:52:04.000000000 +0200
@@ -6,4 +6,15 @@
 
 #include <asm/mach/irq.h>
 
+#if defined(CONFIG_NO_IDLE_HZ)
+# include <asm/dyntick.h>
+# define handle_dynamic_tick(action)					\
+	if (!(action->flags & SA_TIMER) && system_timer->dyn_tick) {	\
+		write_seqlock(&xtime_lock);				\
+		if (system_timer->dyn_tick->state & DYN_TICK_ENABLED)	\
+			system_timer->dyn_tick->handler(irq, 0, regs);	\
+		write_sequnlock(&xtime_lock);				\
+	}
+#endif
+
 #endif
Index: linux-2.6.git/include/linux/irq.h
===================================================================
--- linux-2.6.git.orig/include/linux/irq.h	2006-07-03 00:13:24.000000000 +0200
+++ linux-2.6.git/include/linux/irq.h	2006-07-03 00:49:01.000000000 +0200
@@ -182,6 +182,10 @@ extern int setup_irq(unsigned int irq, s
 
 #ifdef CONFIG_GENERIC_HARDIRQS
 
+#ifndef handle_dynamic_tick
+# define handle_dynamic_tick(a)		do { } while (0)
+#endif
+
 #ifdef CONFIG_SMP
 static inline void set_native_irq_info(int irq, cpumask_t mask)
 {
Index: linux-2.6.git/kernel/irq/handle.c
===================================================================
--- linux-2.6.git.orig/kernel/irq/handle.c	2006-07-03 00:13:24.000000000 +0200
+++ linux-2.6.git/kernel/irq/handle.c	2006-07-03 00:47:47.000000000 +0200
@@ -16,10 +16,6 @@
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
 
-#if defined(CONFIG_NO_IDLE_HZ) && defined(CONFIG_ARM)
-#include <asm/dyntick.h>
-#endif
-
 #include "internals.h"
 
 /**
@@ -133,14 +129,7 @@ irqreturn_t handle_IRQ_event(unsigned in
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
-#if defined(CONFIG_NO_IDLE_HZ) && defined(CONFIG_ARM)
-	if (!(action->flags & SA_TIMER) && system_timer->dyn_tick != NULL) {
-		write_seqlock(&xtime_lock);
-		if (system_timer->dyn_tick->state & DYN_TICK_ENABLED)
-			system_timer->dyn_tick->handler(irq, 0, regs);
-		write_sequnlock(&xtime_lock);
-	}
-#endif
+	handle_dynamic_tick(action);
 
 	if (!(action->flags & IRQF_DISABLED))
 		local_irq_enable();
Index: linux-2.6.git/include/asm-arm/mach/time.h
===================================================================
--- linux-2.6.git.orig/include/asm-arm/mach/time.h	2006-06-21 08:32:46.000000000 +0200
+++ linux-2.6.git/include/asm-arm/mach/time.h	2006-07-03 00:54:07.000000000 +0200
@@ -69,6 +69,7 @@ extern void timer_tick(struct pt_regs *)
 /*
  * Kernel time keeping support.
  */
+struct timespec;
 extern int (*set_rtc)(void);
 extern void save_time_delta(struct timespec *delta, struct timespec *rtc);
 extern void restore_time_delta(struct timespec *delta, struct timespec *rtc);

Re: [PATCH] genirq: ARM dyntick cleanup

[Andrew Morton]

On Mon, 03 Jul 2006 02:18:48 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Linus: "The hacks in kernel/irq/handle.c are really horrid. REALLY
> horrid."
> 
> They are indeed. Move the dyntick quirks to ARM where they belong.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Index: linux-2.6.git/include/asm-arm/hw_irq.h
> ===================================================================
> --- linux-2.6.git.orig/include/asm-arm/hw_irq.h	2006-07-03 00:13:24.000000000 +0200
> +++ linux-2.6.git/include/asm-arm/hw_irq.h	2006-07-03 00:52:04.000000000 +0200
> @@ -6,4 +6,15 @@
>  
>  #include <asm/mach/irq.h>
>  
> +#if defined(CONFIG_NO_IDLE_HZ)
> +# include <asm/dyntick.h>
> +# define handle_dynamic_tick(action)					\
> +	if (!(action->flags & SA_TIMER) && system_timer->dyn_tick) {	\
> +		write_seqlock(&xtime_lock);				\
> +		if (system_timer->dyn_tick->state & DYN_TICK_ENABLED)	\
> +			system_timer->dyn_tick->handler(irq, 0, regs);	\
> +		write_sequnlock(&xtime_lock);				\
> +	}
> +#endif
> +
>  #endif
> Index: linux-2.6.git/include/linux/irq.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/irq.h	2006-07-03 00:13:24.000000000 +0200
> +++ linux-2.6.git/include/linux/irq.h	2006-07-03 00:49:01.000000000 +0200
> @@ -182,6 +182,10 @@ extern int setup_irq(unsigned int irq, s
>  
>  #ifdef CONFIG_GENERIC_HARDIRQS
>  
> +#ifndef handle_dynamic_tick
> +# define handle_dynamic_tick(a)		do { } while (0)
> +#endif
> +
>  #ifdef CONFIG_SMP
>  static inline void set_native_irq_info(int irq, cpumask_t mask)
>  {

This is not exactly a thing of beauty either.  It's much cleaner to use
__attribute__((weak)), but that will add an empty call-return to everyone's
interrupts.

The requirement "if you implement this then you must do so as a macro" is a
bit regrettable.  The ARCH_HAS_HANDLE_DYNAMIC_TICK approach would eliminate
that requirement.



btw, is this, from include/linux/irq.h:

/*
 * Please do not include this file in generic code.  There is currently
 * no requirement for any architecture to implement anything held
 * within this file.
 *
 * Thanks. --rmk
 */

still true?

Re: [PATCH] genirq: ARM dyntick cleanup

[Thomas Gleixner]

On Sun, 2006-07-02 at 17:35 -0700, Andrew Morton wrote:
> >  
> >  #ifdef CONFIG_GENERIC_HARDIRQS
> >  
> > +#ifndef handle_dynamic_tick
> > +# define handle_dynamic_tick(a)		do { } while (0)
> > +#endif
> > +
> >  #ifdef CONFIG_SMP
> >  static inline void set_native_irq_info(int irq, cpumask_t mask)
> >  {
> 
> This is not exactly a thing of beauty either.  It's much cleaner to use
> __attribute__((weak)), but that will add an empty call-return to everyone's
> interrupts.
> 
> The requirement "if you implement this then you must do so as a macro" is a
> bit regrettable.  The ARCH_HAS_HANDLE_DYNAMIC_TICK approach would eliminate
> that requirement.

This quirk should go away once we come around to generalize and
consolidate the dyntick stuff.

> btw, is this, from include/linux/irq.h:
> 
> /*
>  * Please do not include this file in generic code.  There is currently
>  * no requirement for any architecture to implement anything held
>  * within this file.
>  *
>  * Thanks. --rmk
>  */
> 
> still true?

I think what it means is that linux/irq.h must not be included in
drivers. drivers should include linux/interrupt.h instead.

	tglx

Re: [PATCH] genirq: ARM dyntick cleanup

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> > btw, is this, from include/linux/irq.h:
> > 
> > /*
> >  * Please do not include this file in generic code.  There is currently
> >  * no requirement for any architecture to implement anything held
> >  * within this file.
> >  *
> >  * Thanks. --rmk
> >  */
> > 
> > still true?
> 
> I think what it means is that linux/irq.h must not be included in 
> drivers. drivers should include linux/interrupt.h instead.

Christoph has had ideas for cleanups in the irq-header-files area for a 
long time. My rough battleplan would be this:

- linux/interrupt.h should remain the highlevel driver API [which can be
  used by both physical (genirq or non-genirq) or virtual platforms].
  Only this file should be included by drivers.

- rename linux/irq.h to linux/irqchips.h, to make it less likely for
  drivers to include it accidentally.

- rename asm/irq.h to asm/irqchips.h

- most of linux/hardirq.h should merge into interrupt.h [the rest into
  linux/irqchips.h] and hardirq.h should be eliminated.

- merge asm/hardirq.h and asm/hw_irq.h into asm/irqchips.h.

Christoph, agreed?

	Ingo

Re: [PATCH] genirq: ARM dyntick cleanup

[Russell King]

On Sun, Jul 02, 2006 at 05:35:27PM -0700, Andrew Morton wrote:
> This is not exactly a thing of beauty either.  It's much cleaner to use
> __attribute__((weak)), but that will add an empty call-return to everyone's
> interrupts.

Let's not go overboard with the weak stuff - it does not get removed
at link time, so it remains as dead code in the kernel image.

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

Re: [PATCH] genirq: ARM dyntick cleanup

[Andrew Morton]

On Mon, 3 Jul 2006 08:41:55 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Sun, Jul 02, 2006 at 05:35:27PM -0700, Andrew Morton wrote:
> > This is not exactly a thing of beauty either.  It's much cleaner to use
> > __attribute__((weak)), but that will add an empty call-return to everyone's
> > interrupts.
> 
> Let's not go overboard with the weak stuff - it does not get removed
> at link time, so it remains as dead code in the kernel image.

Well.

void handle_dynamic_tick(struct irqaction *action)
{
}

consumes one byte, doesn't it?  That's not very far overboard ;)

And we can optimise away that byte by doing what we do with cond_syscall().

Re: [PATCH] genirq: ARM dyntick cleanup

[Russell King]

On Mon, Jul 03, 2006 at 12:55:42AM -0700, Andrew Morton wrote:
> On Mon, 3 Jul 2006 08:41:55 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 
> > On Sun, Jul 02, 2006 at 05:35:27PM -0700, Andrew Morton wrote:
> > > This is not exactly a thing of beauty either.  It's much cleaner to use
> > > __attribute__((weak)), but that will add an empty call-return to everyone's
> > > interrupts.
> > 
> > Let's not go overboard with the weak stuff - it does not get removed
> > at link time, so it remains as dead code in the kernel image.
> 
> Well.
> 
> void handle_dynamic_tick(struct irqaction *action)
> {
> }
> 
> consumes one byte, doesn't it?  That's not very far overboard ;)

ROTFL!

All the word isn't x86.  On ARM it's 3 words for the stack setup and
one for the tear down, so 16 bytes, assuming the function doesn't
return a value.  If it does, add another 4 bytes.

So, on ARM potentially 16 to 20 bytes per weak function.  That's a
1600% to 2000% increase on your estimate.

(Unfortunately we have to tell the compiler to always generate stack
frames otherwise we can't get call traces out of the kernel.)

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

Re: [PATCH] genirq: ARM dyntick cleanup

[Andrew Morton]

On Mon, 3 Jul 2006 10:03:43 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Mon, Jul 03, 2006 at 12:55:42AM -0700, Andrew Morton wrote:
> > On Mon, 3 Jul 2006 08:41:55 +0100
> > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 
> > > On Sun, Jul 02, 2006 at 05:35:27PM -0700, Andrew Morton wrote:
> > > > This is not exactly a thing of beauty either.  It's much cleaner to use
> > > > __attribute__((weak)), but that will add an empty call-return to everyone's
> > > > interrupts.
> > > 
> > > Let's not go overboard with the weak stuff - it does not get removed
> > > at link time, so it remains as dead code in the kernel image.
> > 
> > Well.
> > 
> > void handle_dynamic_tick(struct irqaction *action)
> > {
> > }
> > 
> > consumes one byte, doesn't it?  That's not very far overboard ;)
> 
> ROTFL!
> 
> All the word isn't x86.  On ARM it's 3 words for the stack setup and
> one for the tear down, so 16 bytes, assuming the function doesn't
> return a value.  If it does, add another 4 bytes.
> 

Well yes, and there's also compiler-added alignment padding after the
function itself.

It's still pretty small beer.  It's a judgement call, which is why I
recommended it not be used here.

A lot of places where it's used (and useful) are in __init setup code.

Re: [PATCH] genirq: ARM dyntick cleanup

[Linus Torvalds]

On Mon, 3 Jul 2006, Andrew Morton wrote:
> 
> void handle_dynamic_tick(struct irqaction *action)
> {
> }
> 
> consumes one byte, doesn't it?  That's not very far overboard ;)

Nope, it consumes - very fundamentally - about 64 bytes.

Even in the absense of alignment (which means that it generally takes up a 
minimum of 16 bytes of real memory), it takes up what would tend to be a 
much more critical resource: a cache line.

So an empty macro is a _lot_ more efficient than an empty function call. 

For interrupt handlers, the L1 I$ miss rate is basically 100%. Even the L2 
I$ miss rate is quite noticeable, _especially_ for things that are not 
easy to prefetch (ie stuff that is out-of-line). Which means that it also 
likely takes an inordinate amount of cycles, for doing zero work.

		Linus

Re: [PATCH] genirq: ARM dyntick cleanup

[Linus Torvalds]

On Sun, 2 Jul 2006, Andrew Morton wrote:
> 
> The requirement "if you implement this then you must do so as a macro" is a
> bit regrettable.  The ARCH_HAS_HANDLE_DYNAMIC_TICK approach would eliminate
> that requirement.

Btw, this is WRONG.

The whole "ARCH_HAS_XYZZY" is nothing but crap. It's totally unreadable, 
compared to the _much_ simpler

	#ifndef xyzzy
	#define zyzzy() /* empty */
	#endif

which is a hell of a lot more obvious to everybody involved, not to 
mention being a lot easier to "grep" for (try it - "grep xyzzy" ends up 
showing _exactly_ what is going on for cases like this, unlike the 
ARCH_HAS_XYZZY crap).

And no, it does not require implementing xyzzy as a macro AT ALL. 

You can very easily just do

	/*
	 * We have a very complex xyzzy, we don't even want to
	 * inline it!
	 */
	extern void xyxxy(...);

	/* Tell the rest of the world that we do it! */
	#define xyzzy xyzzy

and you're now all set. No need for a new stupid name like ARCH_HAS_XYZZY, 
which adds _nothing_ but unnecessary complexity ("What was the condition 
for using that symbol again?" and ungreppability).

WE SHOULD GET RID OF ARCH_HAS_XYZZY. It's a disease.

			Linus

Re: [PATCH] genirq: ARM dyntick cleanup

[Arjan van de Ven]

On Mon, 2006-07-03 at 10:13 -0700, Linus Torvalds wrote:
>         #ifndef xyzzy
>         #define zyzzy() /* empty */
>         #endif
> 

now if you write it as

#define zyzzy() do { ; } while (0)

it even works in a

if (foo())
	zyzzy();
bar();

scenario 

(I know you know that, just pointing that out before people copy your
example :-)

Re: [PATCH] genirq: ARM dyntick cleanup

[Christoph Hellwig]

On Mon, Jul 03, 2006 at 08:57:35AM +0200, Ingo Molnar wrote:
> Christoph has had ideas for cleanups in the irq-header-files area for a 
> long time. My rough battleplan would be this:
> 
> - linux/interrupt.h should remain the highlevel driver API [which can be
>   used by both physical (genirq or non-genirq) or virtual platforms].
>   Only this file should be included by drivers.

Yes.  Note that it's not quite there yet.  Non-genirq architectures currently
have things like enable_irq/disable_irq in asm/irq.h  We really need to have
those prototypes only in linux/interrupt.h.  Unfortunately at least m68k and
sparc had those as macros so they'll need some tweaking first.

> - rename linux/irq.h to linux/irqchips.h, to make it less likely for
>   drivers to include it accidentally.

I find the name rather odd, how bout linux/genirq.h instead?

> 
> - rename asm/irq.h to asm/irqchips.h

Note that currently asm/irq.h is included all over.  

> - most of linux/hardirq.h should merge into interrupt.h [the rest into
>   linux/irqchips.h] and hardirq.h should be eliminated.

Yes.

> - merge asm/hardirq.h and asm/hw_irq.h into asm/irqchips.h.

I'm not sure we can get away with just one asm/*irq.h.  We need arch
bits for genirq and we need arch bits for what was in linux/hardirq.h
and I don't think we want to mix those up.  The latter is just irq_cpustat_t
which needs a big rework anyway to remove the arch independent use of a arch-
defined struct and use DECLARE_PERCPU for each field in each architecture
or a suitable per-arch meachnism.  The only irq_cpustat_t field that the
generic code uses is __softirq_pending and we should rather have a function
abstraction for that.

Re: [PATCH] genirq: ARM dyntick cleanup

[Ingo Molnar]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]


* Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Jul 03, 2006 at 08:57:35AM +0200, Ingo Molnar wrote:
> > Christoph has had ideas for cleanups in the irq-header-files area for a 
> > long time. My rough battleplan would be this:
> > 
> > - linux/interrupt.h should remain the highlevel driver API [which can be
> >   used by both physical (genirq or non-genirq) or virtual platforms].
> >   Only this file should be included by drivers.
> 
> Yes.  Note that it's not quite there yet.  Non-genirq architectures currently
> have things like enable_irq/disable_irq in asm/irq.h  We really need to have
> those prototypes only in linux/interrupt.h.  Unfortunately at least m68k and
> sparc had those as macros so they'll need some tweaking first.
> 
> > - rename linux/irq.h to linux/irqchips.h, to make it less likely for
> >   drivers to include it accidentally.
> 
> I find the name rather odd, how bout linux/genirq.h instead?

yeah, that would be fine too. The "irq chips" name has the advantage 
that it points out that this stuff deals with actual hardware, but no 
strong feelings either way.

> > - rename asm/irq.h to asm/irqchips.h
> 
> Note that currently asm/irq.h is included all over.  

yeah, but only 335 times in drivers/*, so it's a 4 minute job to convert 
them over. (ok, i just did it to check - it results in a 144K patch and 
it took 50 seconds to do. I've attached the result.)

> > - most of linux/hardirq.h should merge into interrupt.h [the rest into
> >   linux/irqchips.h] and hardirq.h should be eliminated.
> 
> Yes.
> 
> > - merge asm/hardirq.h and asm/hw_irq.h into asm/irqchips.h.
> 
> I'm not sure we can get away with just one asm/*irq.h.  We need arch 
> bits for genirq and we need arch bits for what was in linux/hardirq.h 
> and I don't think we want to mix those up.  The latter is just 
> irq_cpustat_t which needs a big rework anyway to remove the arch 
> independent use of a arch- defined struct and use DECLARE_PERCPU for 
> each field in each architecture or a suitable per-arch meachnism.  The 
> only irq_cpustat_t field that the generic code uses is 
> __softirq_pending and we should rather have a function abstraction for 
> that.

ok, agreed.

	Ingo

[-- Attachment #2: remove-asm-irq2.h.bz2 --]
[-- Type: application/x-bzip2, Size: 12688 bytes --]

Re: [PATCH] genirq: ARM dyntick cleanup

[Russell King]

On Tue, Jul 04, 2006 at 02:22:31PM +0200, Ingo Molnar wrote:
> > > - rename asm/irq.h to asm/irqchips.h
> > 
> > Note that currently asm/irq.h is included all over.  
> 
> yeah, but only 335 times in drivers/*, so it's a 4 minute job to convert 
> them over. (ok, i just did it to check - it results in a 144K patch and 
> it took 50 seconds to do. I've attached the result.)

Note that ARM drivers generally require asm/irq.h by way of it defining
the IRQ numbers for the platform, so this patch moves the include of it
to linux/genirq.h.

Also note that including genirq.h (formerly irq.h) is broken for
architectures which don't yet use this stuff - it'll probably cause
compile failures because it wants asm/hw_irq.h.

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

Re: [PATCH] genirq: ARM dyntick cleanup

[Randy.Dunlap]

On Mon, 03 Jul 2006 19:27:07 +0200 Arjan van de Ven wrote:

> On Mon, 2006-07-03 at 10:13 -0700, Linus Torvalds wrote:
> >         #ifndef xyzzy
> >         #define zyzzy() /* empty */
> >         #endif
> > 
> 
> now if you write it as
> 
> #define zyzzy() do { ; } while (0)
> 
> it even works in a
> 
> if (foo())
> 	zyzzy();
> bar();
> 
> scenario 
> 
> (I know you know that, just pointing that out before people copy your
> example :-)

OK, I'll bite.  What part of Linus's macro doesn't work.
I compiled your foo/zyzzy/bar example with both his "empty"
macro and the do-while macro.  Same code produced both ways,
no compile warnings/errors.

---
~Randy

Re: [PATCH] genirq: ARM dyntick cleanup

[Andrew Morton]

On Wed, 5 Jul 2006 16:24:25 -0700
"Randy.Dunlap" <rdunlap@xenotime.net> wrote:

> On Mon, 03 Jul 2006 19:27:07 +0200 Arjan van de Ven wrote:
> 
> > On Mon, 2006-07-03 at 10:13 -0700, Linus Torvalds wrote:
> > >         #ifndef xyzzy
> > >         #define zyzzy() /* empty */
> > >         #endif
> > > 
> > 
> > now if you write it as
> > 
> > #define zyzzy() do { ; } while (0)
> > 
> > it even works in a
> > 
> > if (foo())
> > 	zyzzy();
> > bar();
> > 
> > scenario 
> > 
> > (I know you know that, just pointing that out before people copy your
> > example :-)
> 
> OK, I'll bite.  What part of Linus's macro doesn't work.
> I compiled your foo/zyzzy/bar example with both his "empty"
> macro and the do-while macro.  Same code produced both ways,
> no compile warnings/errors.
> 

	if (foo())
		;

will generate `warning: empty body in an if-statement' when compiled with -W.

We go round this loop regularly.  Maybe someone should write it up.  Meanwhile,
use do{}while(0) and be happy and secure.

Re: [PATCH] genirq: ARM dyntick cleanup

[Randy.Dunlap]

On Wed, 5 Jul 2006 16:35:30 -0700 Andrew Morton wrote:

> On Wed, 5 Jul 2006 16:24:25 -0700
> "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> 
> > On Mon, 03 Jul 2006 19:27:07 +0200 Arjan van de Ven wrote:
> > 
> > > On Mon, 2006-07-03 at 10:13 -0700, Linus Torvalds wrote:
> > > >         #ifndef xyzzy
> > > >         #define zyzzy() /* empty */
> > > >         #endif
> > > > 
> > > 
> > > now if you write it as
> > > 
> > > #define zyzzy() do { ; } while (0)
> > > 
> > > it even works in a
> > > 
> > > if (foo())
> > > 	zyzzy();
> > > bar();
> > > 
> > > scenario 
> > > 
> > > (I know you know that, just pointing that out before people copy your
> > > example :-)
> > 
> > OK, I'll bite.  What part of Linus's macro doesn't work.
> > I compiled your foo/zyzzy/bar example with both his "empty"
> > macro and the do-while macro.  Same code produced both ways,
> > no compile warnings/errors.
> > 
> 
> 	if (foo())
> 		;
> 
> will generate `warning: empty body in an if-statement' when compiled with -W.
> 
> We go round this loop regularly.  Maybe someone should write it up.  Meanwhile,
> use do{}while(0) and be happy and secure.

Thanks, I was missing the -W.

or I should read the kernelnewbies FAQ  :)

http://kernelnewbies.org/FAQ/DoWhile0

---
~Randy

Re: [PATCH] genirq: ARM dyntick cleanup

[Linus Torvalds]

On Wed, 5 Jul 2006, Randy.Dunlap wrote:
> 
> OK, I'll bite.  What part of Linus's macro doesn't work.

Heh. This is "C language 101".

The reason we always write

	#define empty_statement do { } while (0)

instead of

	#define empty_statement /* empty */

is not that

	if (x)
		empty_statement;

wouldn't work like Arjan claimed, but because otherwise the empty 
statement won't parse perfectly as a real C statement.

In particular, you tend to get much better error messages if you have 
syntax errors _around_ the empty statement if it's done as that 
"do { } while (0)" thing. You also avoid compiler warnings about 
empty statements or statements without effects, that you'd get if you were 
to use

	#define empty_statement /* empty */

or

	#define empty_statement 0

for example (a expression statement is a perfectly valid statement, as is 
an empty one, but many compilers will warn on them).

It's also simply good practice - if you _always_ do the "do { } while (0)" 
thing, you'll never get bitten by having a macro that has several 
statements inside of it, and you'll also never get bitten by a macro that 
is _meant_ to be used as a statement being used as part of an expression 
instead.

It basically boils down to the fact that the "do { } while (0)" format is 
always syntactically correct, /regardless/ of what is inside of the 
braces, and should always give you meaningful error messages regardless of 
what is _around_ the macro usage.

For example:

	if (a)
		empty_statement
	b;

will give the _correct_ syntax error message ("expected ';'"), instead of 
silently turning into

	if (a)
		b;

or other nonsense.

But in the end, the real aim is to just teach your fingers to _always_ put 
the do/while(0) there, so that you never EVER write something like

	#define MACRO one; two;

which really breaks down.

This is, btw, the same reason a lot of people (including me, most of the 
time) will write

	#define VALUE (12)

instead of writing the simpler

	#define VALUE 12

just because it's good practice to _always_ have the parentheses around 
a macro that ends up being used as an expression.

So we always also write

	#define ADD(a,b) ((a)+(b))

because otherwise you eventually _will_ get bitten (we've had that 
particular bug bite us in the *ss lots of times, even though people should 
know better)

			Linus

Re: [PATCH] genirq: ARM dyntick cleanup

[Randy.Dunlap]

On Wed, 5 Jul 2006 16:53:22 -0700 (PDT) Linus Torvalds wrote:

> 
> 
> On Wed, 5 Jul 2006, Randy.Dunlap wrote:
> > 
> > OK, I'll bite.  What part of Linus's macro doesn't work.
> 
> Heh. This is "C language 101".

Yes, I got most of that.  :)
more below.

> The reason we always write
> 
> 	#define empty_statement do { } while (0)
> 
> instead of
> 
> 	#define empty_statement /* empty */
> 
> is not that
> 
> 	if (x)
> 		empty_statement;
> 
> wouldn't work like Arjan claimed, but because otherwise the empty 
> statement won't parse perfectly as a real C statement.
> 
> In particular, you tend to get much better error messages if you have 
> syntax errors _around_ the empty statement if it's done as that 
> "do { } while (0)" thing. You also avoid compiler warnings about 
> empty statements or statements without effects, that you'd get if you were 
> to use
> 
> 	#define empty_statement /* empty */
> 
> or
> 
> 	#define empty_statement 0
> 
> for example (a expression statement is a perfectly valid statement, as is 
> an empty one, but many compilers will warn on them).
> 
> It's also simply good practice - if you _always_ do the "do { } while (0)" 
> thing, you'll never get bitten by having a macro that has several 
> statements inside of it, and you'll also never get bitten by a macro that 
> is _meant_ to be used as a statement being used as part of an expression 
> instead.
> 
> It basically boils down to the fact that the "do { } while (0)" format is 
> always syntactically correct, /regardless/ of what is inside of the 
> braces, and should always give you meaningful error messages regardless of 
> what is _around_ the macro usage.

Yes, I already understood that.  I was interested in Arjan's
specific example, which was:

if (foo())
	zyzzy();

in which he supplied the terminating semi-colon, and which Andrew
explained with the -W warning...

> For example:
> 
> 	if (a)
> 		empty_statement
> 	b;
> 
> will give the _correct_ syntax error message ("expected ';'"), instead of 
> silently turning into
> 
> 	if (a)
> 		b;
> 
> or other nonsense.

OK, good practice, yes.

> But in the end, the real aim is to just teach your fingers to _always_ put 
> the do/while(0) there, so that you never EVER write something like
> 
> 	#define MACRO one; two;
> 
> which really breaks down.
> 
> This is, btw, the same reason a lot of people (including me, most of the 
> time) will write
> 
> 	#define VALUE (12)
> 
> instead of writing the simpler
> 
> 	#define VALUE 12
> 
> just because it's good practice to _always_ have the parentheses around 
> a macro that ends up being used as an expression.
> 
> So we always also write
> 
> 	#define ADD(a,b) ((a)+(b))
> 
> because otherwise you eventually _will_ get bitten (we've had that 
> particular bug bite us in the *ss lots of times, even though people should 
> know better)

Yes, I have the () macro practice down.  I was just looking for the
problem with that one specific example, which you and Andrew have now
explained.  Thanks.

---
~Randy

Re: [PATCH] genirq: ARM dyntick cleanup

[Giacomo A. Catenazzi]

Linus Torvalds wrote:
> 
> On Wed, 5 Jul 2006, Randy.Dunlap wrote:
>> OK, I'll bite.  What part of Linus's macro doesn't work.
> 
> Heh. This is "C language 101".
> 
> The reason we always write
> 
> 	#define empty_statement do { } while (0)
> 
> instead of
> 
> 	#define empty_statement /* empty */
> 
> is not that
> 
> 	if (x)
> 		empty_statement;
> 
> wouldn't work like Arjan claimed, but because otherwise the empty 
> statement won't parse perfectly as a real C statement.

But the classical way of empty statments is "((void) 0)"
See K&R, glibc or SuS, for assert.h
( http://www.opengroup.org/onlinepubs/000095399/basedefs/assert.h.html )

or I miss something?

ciao
	cate

Re: [PATCH] genirq: ARM dyntick cleanup

[Christoph Hellwig]

On Tue, Jul 04, 2006 at 12:54:25PM +0100, Christoph Hellwig wrote:
> On Mon, Jul 03, 2006 at 08:57:35AM +0200, Ingo Molnar wrote:
> > Christoph has had ideas for cleanups in the irq-header-files area for a 
> > long time. My rough battleplan would be this:
> > 
> > - linux/interrupt.h should remain the highlevel driver API [which can be
> >   used by both physical (genirq or non-genirq) or virtual platforms].
> >   Only this file should be included by drivers.
> 
> Yes.  Note that it's not quite there yet.  Non-genirq architectures currently
> have things like enable_irq/disable_irq in asm/irq.h  We really need to have
> those prototypes only in linux/interrupt.h.  Unfortunately at least m68k and
> sparc had those as macros so they'll need some tweaking first.

Here's the patch I have lying around for a while.  I rediffed it to current
Linus' tree.  It still needs someone to test it on sparc32 and m68knommu
(compile at least).


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/arch/arm26/kernel/irq.c
===================================================================
--- linux-2.6.orig/arch/arm26/kernel/irq.c	2006-07-08 17:44:19.000000000 +0200
+++ linux-2.6/arch/arm26/kernel/irq.c	2006-07-08 20:29:25.000000000 +0200
@@ -86,7 +86,7 @@
  *
  *	This function may be called from IRQ context.
  */
-void disable_irq(unsigned int irq)
+void disable_irq_nosync(unsigned int irq)
 {
 	struct irqdesc *desc = irq_desc + irq;
 	unsigned long flags;
@@ -95,6 +95,12 @@
 		desc->enabled = 0;
 	spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
+EXPORT_SYMBOL(disable_irq_nosync);
+
+void disable_irq(unsigned int irq)
+{
+	return disable_irq_nosync(irq);
+}
 
 /**
  *	enable_irq - enable interrupt handling on an irq
Index: linux-2.6/arch/h8300/kernel/ints.c
===================================================================
--- linux-2.6.orig/arch/h8300/kernel/ints.c	2006-07-08 17:44:19.000000000 +0200
+++ linux-2.6/arch/h8300/kernel/ints.c	2006-07-08 20:29:25.000000000 +0200
@@ -208,11 +208,17 @@
 		IER_REGS |= 1 << (irq - EXT_IRQ0);
 }
 
-void disable_irq(unsigned int irq)
+void disable_irq_nosync(unsigned int irq)
 {
 	if (irq >= EXT_IRQ0 && irq <= (EXT_IRQ0 + EXT_IRQS))
 		IER_REGS &= ~(1 << (irq - EXT_IRQ0));
 }
+EXPORT_SYMBOL(disable_irq_nosync);
+
+void disable_irq(unsigned int irq)
+{
+	disable_irq_nosync(irq);
+}
 
 asmlinkage void process_int(int irq, struct pt_regs *fp)
 {
Index: linux-2.6/arch/m68knommu/kernel/Makefile
===================================================================
--- linux-2.6.orig/arch/m68knommu/kernel/Makefile	2006-01-12 16:27:05.000000000 +0100
+++ linux-2.6/arch/m68knommu/kernel/Makefile	2006-07-08 20:29:25.000000000 +0200
@@ -5,7 +5,7 @@
 extra-y := vmlinux.lds
 
 obj-y += dma.o entry.o init_task.o m68k_ksyms.o process.o ptrace.o semaphore.o \
-	 setup.o signal.o syscalltable.o sys_m68k.o time.o traps.o
+	 setup.o signal.o syscalltable.o sys_m68k.o time.o traps.o irq.o
 
 obj-$(CONFIG_MODULES)	+= module.o
 obj-$(CONFIG_COMEMPCI)	+= comempci.o
Index: linux-2.6/arch/m68knommu/kernel/irq.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/arch/m68knommu/kernel/irq.c	2006-07-08 20:29:25.000000000 +0200
@@ -0,0 +1,22 @@
+
+#include <linux/interrupt.h>
+
+/*
+ * Stubs for IRQ handling helpers.
+ * Seems wrong to me that these are no-ops on m68knommu.  --hch
+ */
+
+void disable_irq_nosync(unsigned int irq)
+{
+}
+EXPORT_SYMBOL(disable_irq_nosync);
+
+void disable_irq(unsigned int irq)
+{
+}
+EXPORT_SYMBOL(disable_irq);
+
+void enable_irq(unsigned int irq)
+{
+}
+EXPORT_SYMBOL(enable_irq);
Index: linux-2.6/include/asm-arm26/irq.h
===================================================================
--- linux-2.6.orig/include/asm-arm26/irq.h	2006-07-08 17:44:35.000000000 +0200
+++ linux-2.6/include/asm-arm26/irq.h	2006-07-08 20:29:25.000000000 +0200
@@ -22,12 +22,6 @@
 #define NO_IRQ	((unsigned int)(-1))
 #endif
 
-struct irqaction;
-
-#define disable_irq_nosync(i) disable_irq(i)
-
-extern void disable_irq(unsigned int);
-extern void enable_irq(unsigned int);
 
 #define __IRQT_FALEDGE	(1 << 0)
 #define __IRQT_RISEDGE	(1 << 1)
Index: linux-2.6/include/asm-frv/irq.h
===================================================================
--- linux-2.6.orig/include/asm-frv/irq.h	2006-07-08 17:44:35.000000000 +0200
+++ linux-2.6/include/asm-frv/irq.h	2006-07-08 20:29:25.000000000 +0200
@@ -35,9 +35,4 @@
 	return irq;
 }
 
-extern void disable_irq_nosync(unsigned int irq);
-extern void disable_irq(unsigned int irq);
-extern void enable_irq(unsigned int irq);
-
-
 #endif /* _ASM_IRQ_H_ */
Index: linux-2.6/include/asm-h8300/irq.h
===================================================================
--- linux-2.6.orig/include/asm-h8300/irq.h	2006-07-08 17:44:35.000000000 +0200
+++ linux-2.6/include/asm-h8300/irq.h	2006-07-08 20:29:25.000000000 +0200
@@ -59,8 +59,4 @@
 	return irq;
 }
 
-extern void enable_irq(unsigned int);
-extern void disable_irq(unsigned int);
-#define disable_irq_nosync(x)	disable_irq(x)
-
 #endif /* _H8300_IRQ_H_ */
Index: linux-2.6/include/asm-m68k/irq.h
===================================================================
--- linux-2.6.orig/include/asm-m68k/irq.h	2006-07-08 17:44:35.000000000 +0200
+++ linux-2.6/include/asm-m68k/irq.h	2006-07-08 20:29:42.000000000 +0200
@@ -59,9 +59,6 @@
 #define IRQ_USER	8
 
 extern unsigned int irq_canonicalize(unsigned int irq);
-extern void enable_irq(unsigned int);
-extern void disable_irq(unsigned int);
-#define disable_irq_nosync	disable_irq
 
 struct pt_regs;
 
Index: linux-2.6/include/asm-m68knommu/irq.h
===================================================================
--- linux-2.6.orig/include/asm-m68knommu/irq.h	2006-07-08 17:44:36.000000000 +0200
+++ linux-2.6/include/asm-m68knommu/irq.h	2006-07-08 20:30:04.000000000 +0200
@@ -80,11 +80,4 @@
 
 #endif /* CONFIG_M68360 */
 
-/*
- * Some drivers want these entry points
- */
-#define enable_irq(x)	do { } while (0)
-#define disable_irq(x)	do { } while (0)
-#define disable_irq_nosync(x)	disable_irq(x)
-
 #endif /* _M68K_IRQ_H_ */
Index: linux-2.6/include/asm-sparc/irq.h
===================================================================
--- linux-2.6.orig/include/asm-sparc/irq.h	2006-07-08 20:27:38.000000000 +0200
+++ linux-2.6/include/asm-sparc/irq.h	2006-07-08 20:29:25.000000000 +0200
@@ -36,10 +36,6 @@
 BTFIXUPDEF_CALL(void, clear_profile_irq, int)
 BTFIXUPDEF_CALL(void, load_profile_irq, int, unsigned int)
 
-extern void disable_irq_nosync(unsigned int irq);
-extern void disable_irq(unsigned int irq);
-extern void enable_irq(unsigned int irq);
-
 static inline void disable_pil_irq(unsigned int irq)
 {
 	BTFIXUP_CALL(disable_pil_irq)(irq);
Index: linux-2.6/include/linux/interrupt.h
===================================================================
--- linux-2.6.orig/include/linux/interrupt.h	2006-07-08 17:44:36.000000000 +0200
+++ linux-2.6/include/linux/interrupt.h	2006-07-08 20:32:01.000000000 +0200
@@ -99,11 +99,11 @@
 # define local_irq_enable_in_hardirq()	local_irq_enable()
 #endif
 
-#ifdef CONFIG_GENERIC_HARDIRQS
 extern void disable_irq_nosync(unsigned int irq);
 extern void disable_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 
+#ifdef CONFIG_GENERIC_HARDIRQS
 /*
  * Special lockdep variants of irq disabling/enabling.
  * These should be used for locking constructs that
Index: linux-2.6/arch/sparc/kernel/irq.c
===================================================================
--- linux-2.6.orig/arch/sparc/kernel/irq.c	2006-07-08 17:44:21.000000000 +0200
+++ linux-2.6/arch/sparc/kernel/irq.c	2006-07-08 20:27:38.000000000 +0200
@@ -549,6 +549,25 @@
 
 EXPORT_SYMBOL(request_irq);
 
+void disable_irq_nosync(unsigned int irq)
+{
+	BTFIXUP_CALL(disable_irq)(irq);
+}
+EXPORT_SYMBOL(disable_irq_nosync);
+
+void disable_irq(unsigned int irq)
+{
+	BTFIXUP_CALL(disable_irq)(irq);
+}
+EXPORT_SYMBOL(disable_irq);
+
+void enable_irq(unsigned int irq)
+{
+	BTFIXUP_CALL(enable_irq)(irq);
+}
+EXPORT_SYMBOL(enable_irq);
+
+
 /* We really don't need these at all on the Sparc.  We only have
  * stubs here because they are exported to modules.
  */
Index: linux-2.6/include/asm-sparc/irq.h
===================================================================
--- linux-2.6.orig/include/asm-sparc/irq.h	2006-07-08 17:44:36.000000000 +0200
+++ linux-2.6/include/asm-sparc/irq.h	2006-07-08 20:27:38.000000000 +0200
@@ -36,20 +36,6 @@
 BTFIXUPDEF_CALL(void, clear_profile_irq, int)
 BTFIXUPDEF_CALL(void, load_profile_irq, int, unsigned int)
 
-static inline void disable_irq_nosync(unsigned int irq)
-{
-	BTFIXUP_CALL(disable_irq)(irq);
-}
-
-static inline void disable_irq(unsigned int irq)
-{
-	BTFIXUP_CALL(disable_irq)(irq);
-}
-
-static inline void enable_irq(unsigned int irq)
-{
-	BTFIXUP_CALL(enable_irq)(irq);
-}
 
 static inline void disable_pil_irq(unsigned int irq)
 {