genirq: ARM dyntick cleanup

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 <[email protected]>

Index: linux-2.6.git/include/asm-arm/hw_irq.h
===================================================================

include/asm-arm/hw_irq.h +11 -0

@@ -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
===================================================================

include/linux/irq.h extern int setup_irq(unsigned int irq, s · line 191

Quoted 10 lines

#ifdef CONFIG_GENERIC_HARDIRQS

include/linux/irq.h +4 -0

+#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?

Quoted 10 lines

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
*/

Quoted 1 line

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.

Quoted 6 lines · Ingo Molnar

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.

Quoted 2 lines

- 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?

Quoted 1 line

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

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

Quoted 2 lines

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

Yes.

Quoted 1 line

- 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.

Quoted 3 lines

Quoted 1 line

- 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.)

Quoted 3 lines · Ingo Molnar

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

Quoted 3 lines

- 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.

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.

Quoted 3 lines · Andrew Morton

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.

Quoted 7 lines · On Mon, 3 Jul 2006 08:41:55 +0100 Russell King <[email protected]>…

Quoted 3 lines · Andrew Morton

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 ;)

Quoted 17 lines · Andrew Morton

Quoted 7 lines · On Mon, 3 Jul 2006 08:41:55 +0100 Russell King <[email protected]>…

Quoted 3 lines · Andrew Morton

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) {
}

Quoted 1 line

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.

void handle_dynamic_tick(struct irqaction *action) {
}

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

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.

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

Quoted 4 lines · Linus Torvalds

        #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 :-)

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

Quoted 4 lines · Linus Torvalds

        #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

Quoted 2 lines

(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.

Quoted 20 lines · On Wed, 5 Jul 2006 16:24:25 -0700 "Randy.Dunlap" <[email protected]> wrote:

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

Quoted 4 lines · Linus Torvalds

        #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

Quoted 8 lines

Quoted 2 lines

(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.

OK, I'll bite. What part of Linus's macro doesn't work.

Quoted 2 lines · Randy.Dunlap

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)

Quoted 1 line · Randy.Dunlap

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.