Add hard_irq_disable()
PATCH 2/3 Add hard_irq_disable()
Some architectures, like powerpc, implement lazy disabling of interrupts. That means that on those, local_irq_disable() doesn't actually disable interrupts on the CPU, but only sets some per CPU flag which cause them to be disabled only if an interrupt actually occurs.
However, in some cases, such as stop_machine, we really want interrupts to be fully disabled. For example, I have code using stop machine to do ECC error injection, used to verify operations of the ECC hardware, that sort of thing. It really needs to make sure that nothing is actually writing to memory while the injection happens. Similar examples can be found in other low level bits and pieces.
This patch implements a generic hard_irq_disable() function which is meant to be called -after- local_irq_disable() and ensures that interrupts are fully disabled on that CPU. The default implementation is a nop, though powerpc does already provide an appropriate one.
include/linux/interrupt.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
include/linux/interrupt.h +10 -0
@@ -241,6 +241,16 @@ static inline void __deprecated save_and#define save_and_cli (x) save_and_cli(&x)#endif /* CONFIG_SMP */ +/* Some architectures might implement lazy enabling/disabling of +* interrupts. In some cases, such as stop_machine, we might want +* to ensure that after a local_irq_disable(), interrupts have +* really been disabled in hardware. Such architectures need to +* implement the following hook. +*/ +#ifndef hard_irq_disable +#define hard_irq_disable () do { } while(0) +#endif +/* PLEASE, avoid to allocate new softirqs, if you need not _really_ high frequency threaded job scheduling. For almost all the purposes tasklets are more than enough. F.e. all serial device BHs et
Quoted 5 lines · On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt…
include/linux/interrupt.h +0
-0@@ -241,6 +241,16 @@ static inline void __deprecated save_and#define save_and_cli (x) save_and_cli(&x)#endif /* CONFIG_SMP */
include/linux/interrupt.h static inline void __deprecated save_and · line 252
Quoted 10 lines
include/linux/interrupt.h +9
-0+/* Some architectures might implement lazy enabling/disabling of +* interrupts. In some cases, such as stop_machine, we might want +* to ensure that after a local_irq_disable(), interrupts have +* really been disabled in hardware. Such architectures need to +* implement the following hook. +*/ +#ifndef hard_irq_disable +#define hard_irq_disable () do { } while(0) +#endif
We absolutely require that the architecture's hard_irq_disable() be defined when we do this. If it happens that the definition of hard_irq_disable() is implemented three levels deep in nested includes then we risk getting into a situation where different .c files see different implementations of hard_irq_disable(), which could lead to confusing results, to say the least.
Your implementation comes via the inclusion of system.h which then includes hw_irq.h. That's perhaps a little fragile and it would be better to
a) include in the comment the name of the arch file which must implement hard_irq_disable() and
b) include that file directly from this one.
Oh, and your comment layout style is wrong ;)
Quoted 8 lines · Andrew Morton
Quoted 6 lines · On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt…
include/linux/interrupt.h +0
-0@@ -241,6 +241,16 @@ static inline void __deprecated save_and#define save_and_cli (x) save_and_cli(&x)#endif /* CONFIG_SMP */
Quoted 15 lines
include/linux/interrupt.h static inline void __deprecated save_and · line 252
Quoted 9 lines
include/linux/interrupt.h +9
-0+/* Some architectures might implement lazy enabling/disabling of +* interrupts. In some cases, such as stop_machine, we might want +* to ensure that after a local_irq_disable(), interrupts have +* really been disabled in hardware. Such architectures need to +* implement the following hook. +*/ +#ifndef hard_irq_disable +#define hard_irq_disable () do { } while(0) +#endif We absolutely require that the architecture's
hard_irq_disable()be defined when we do this. If it happens that the definition ofhard_irq_disable()is implemented three levels deep in nested includes then we risk getting into a situation where different .c files see different implementations ofhard_irq_disable(), which could lead to confusing results, to say the least.
Yes, I'm indeed a bit worried about that... I've been wondering what's the best include path here... I tried to follow who gets to hw_irq.h and didn't come to any conclusive results.
powerpc gets it from asm/system.h but I haven't verified other arch (though it only matters on arch that have their own here).
I've verified that a #error on ppc up there will not trigger thus it's fine on powerpc, but I agree it's a bit fragile.
Quoted 7 lines
Your implementation comes via the inclusion of
system.hwhich then includeshw_irq.h. That's perhaps a little fragile and it would be better toa) include in the comment the name of the arch file which must implement
hard_irq_disable()andb) include that file directly from this one.
Fair enough. I was just worried that including hw_irq.h here might cause trouble for some archs though (as I said, we get it indirectly on powerpc via some other asm thingy, not via some linux/*.h). I've looked around and seen all sort of horrors in arch include dependencies (including some circular stuff that must work by mere luck).
Quoted 1 line
Oh, and your comment layout style is wrong ;)
What about my comment layout style ? I've been using that forever ... Or do you mean I should use a function documentation style layout there ?
Cheers,
Ben.
Quoted 29 lines · On Thu, 10 May 2007 16:35:51 +1000 Benjamin Herrenschmidt…
Quoted 8 lines · Andrew Morton
Quoted 6 lines · On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt…
include/linux/interrupt.h +0
-0@@ -241,6 +241,16 @@ static inline void __deprecated save_and#define save_and_cli (x) save_and_cli(&x)#endif /* CONFIG_SMP */ Quoted 15 lines
include/linux/interrupt.h static inline void __deprecated save_and · line 252
Quoted 9 lines
include/linux/interrupt.h +9
-0+/* Some architectures might implement lazy enabling/disabling of +* interrupts. In some cases, such as stop_machine, we might want +* to ensure that after a local_irq_disable(), interrupts have +* really been disabled in hardware. Such architectures need to +* implement the following hook. +*/ +#ifndef hard_irq_disable +#define hard_irq_disable () do { } while(0) +#endif We absolutely require that the architecture's
hard_irq_disable()be defined when we do this. If it happens that the definition ofhard_irq_disable()is implemented three levels deep in nested includes then we risk getting into a situation where different .c files see different implementations ofhard_irq_disable(), which could lead to confusing results, to say the least.Yes, I'm indeed a bit worried about that... I've been wondering what's the best include path here... I tried to follow who gets to
hw_irq.hand didn't come to any conclusive results.
Quoted 5 lines
powerpc gets it from
asm/system.hbut I haven't verified other arch (though it only matters on arch that have their own here).I've verified that a #error on ppc up there will not trigger thus it's fine on powerpc, but I agree it's a bit fragile.
I think saying "system.h must provide this" is reasonable. The fact that powerpc does that via another inclusion is a powerpc detail - just don't break it ;)
Quoted 14 lines
Quoted 7 lines
Your implementation comes via the inclusion of
system.hwhich then includeshw_irq.h. That's perhaps a little fragile and it would be better toa) include in the comment the name of the arch file which must implement
hard_irq_disable()andb) include that file directly from this one.
Fair enough. I was just worried that including
hw_irq.hhere might cause trouble for some archs though (as I said, we get it indirectly on powerpc via some other asm thingy, not via some linux/*.h). I've looked around and seen all sort of horrors in arch include dependencies (including some circular stuff that must work by mere luck).
Quoted 4 lines
Quoted 1 line
Oh, and your comment layout style is wrong ;)
What about my comment layout style ? I've been using that forever ... Or do you mean I should use a function documentation style layout there ?
/* This
* is
* wrong
*/
/*
* This
* is
* right
*/
Quoted 9 lines
Quoted 3 lines
What about my comment layout style ? I've been using that forever ... Or do you mean I should use a function documentation style layout there ?
/* This
* is
* wrong
*/
Quoted 5 lines
/*
* This
* is
* right
*/
Hrm... how bad are you about that one ? I must say I prefer my style :-)
Cheers,
Ben.
From: Benjamin Herrenschmidt <[email protected]>
Date: Thu, 10 May 2007 18:41:45 +1000
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
What about my comment layout style ? I've been using that forever ... Or do you mean I should use a function documentation style layout there ?
/* This
* is
* wrong
*/
Quoted 7 lines
Quoted 5 lines
/*
* This
* is
* right
*/Hrm... how bad are you about that one ? I must say I prefer my style :-)
I think the "wrong" one is right because screen real-estate matters. In fact that's the one I use and enforce throughout the networking and sparc ports.
I even hand edit out those empty comment lines in patches submitted to me when I spot them.
Quoted 9 lines · On Thu, 10 May 2007 18:41:45 +1000 Benjamin Herrenschmidt…
Quoted 9 lines
Quoted 3 lines
What about my comment layout style ? I've been using that forever ... Or do you mean I should use a function documentation style layout there ?
/* This
* is
* wrong
*/
Quoted 8 lines
Quoted 5 lines
/*
* This
* is
* right
*/Hrm... how bad are you about that one ? I must say I prefer my style :-)
I usually shrug and ignore it. It's more a matter of informing people about it than going in there and fixing them all.
We discussed this a couple of months back. davem landed firmly in the second camp and everyone then shut up ;)
From: Andrew Morton <[email protected]>
Date: Thu, 10 May 2007 01:50:36 -0700
Quoted 2 lines
We discussed this a couple of months back. davem landed firmly in the second camp and everyone then shut up ;)
No I landed in the first :-)))
I think the empty lines are a waste and only serve to eat up precious screen real-estate when reading code.
It is possible that I used to use the empty line thing in the past, but I definitely don't do that any more.
Quoted 6 lines · David Miller
From: Andrew Morton <[email protected]>
Date: Thu, 10 May 2007 01:50:36 -0700Quoted 2 lines
We discussed this a couple of months back. davem landed firmly in the second camp and everyone then shut up ;)
Quoted 7 lines
No I landed in the first :-)))
I think the empty lines are a waste and only serve to eat up precious screen real-estate when reading code.
It is possible that I used to use the empty line thing in the past, but I definitely don't do that any more.
Yup, I used to do the other one too but nowadays, I much prefer not wasting that additional line unless specific circumstances, like I want a kind of "title" in front of a whole block of other definitions with their own comments.
Something like:
/*
* foo management stuff
*/
/* This puts the bar in the foo
*/
code code code code
/* This does something you don't want to know about
*/
code code code code
But does it realy matter that much ? :-)
Cheers,
Ben.
Quoted 28 lines · Benjamin Herrenschmidt
Quoted 6 lines · David Miller
From: Andrew Morton <[email protected]>
Date: Thu, 10 May 2007 01:50:36 -0700Quoted 2 lines
We discussed this a couple of months back. davem landed firmly in the second camp and everyone then shut up ;)
Quoted 7 lines
No I landed in the first :-)))
I think the empty lines are a waste and only serve to eat up precious screen real-estate when reading code.
It is possible that I used to use the empty line thing in the past, but I definitely don't do that any more.
Yup, I used to do the other one too but nowadays, I much prefer not wasting that additional line unless specific circumstances, like I want a kind of "title" in front of a whole block of other definitions with their own comments.
Something like:
/*
* foo management stuff
*/
Quoted 7 lines
/* This puts the bar in the foo
*/
code code code code/* This does something you don't want to know about
*/
code code code code
Now your examples are just wrong. One liners should be:
/* This puts the bar in the foo */
Are we having fun yet? ;)
/me goes to look for something better to do
josh
Hi Andrew,
Quoted 8 lines · Andrew Morton
Quoted 6 lines · On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt…
include/linux/interrupt.h 2007-05-10 15:18:04.000000000 +1000 renamed from include/linux/interrupt.h 2007-05-10 14:51:22.000000000 +1000 +0
-0@@ -241,6 +241,16 @@ static inline void __deprecated save_and #define save_and_cli(x) save_and_cli(&x) #endif /* CONFIG_SMP */
Quoted 15 lines
include/linux/interrupt.h 2007-05-10 15:18:04.000000000 +1000 static inline void __deprecated save_and · line 252
Quoted 9 lines
include/linux/interrupt.h 2007-05-10 15:18:04.000000000 +1000 renamed from include/linux/interrupt.h 2007-05-10 14:51:22.000000000 +1000 +9
-0+/* Some architectures might implement lazy enabling/disabling of + * interrupts. In some cases, such as stop_machine, we might want + * to ensure that after a local_irq_disable(), interrupts have + * really been disabled in hardware. Such architectures need to + * implement the following hook. + */ +#ifndef hard_irq_disable +#define hard_irq_disable() do { } while(0) +#endifWe absolutely require that the architecture's
hard_irq_disable()be defined when we do this. If it happens that the definition ofhard_irq_disable()is implemented three levels deep in nested includes then we risk getting into a situation where different .c files see different implementations ofhard_irq_disable(), which could lead to confusing results, to say the least.
So you're saying that this mechanism forces the arch (that really wants hard_irq_disable) to _#define_ hard_irq_disable (as a macro), and if it implements it as an inline function, for example, then we're screwed?
Quoted 7 lines
Your implementation comes via the inclusion of
system.hwhich then includeshw_irq.h. That's perhaps a little fragile and it would be better toa) include in the comment the name of the arch file which must implement
hard_irq_disable()andb) include that file directly from this one.
Hmmm. How about:
1. Introduce some CONFIG_WANTS_HARD_IRQ_DISABLE that is #defined (or left undefined) by the arch/.../defconfig (depending upon whether or not that arch implements a hard_irq_disable() for itself or not)
2. Then pull-in that code into include/linux/interrupt.h somehow (through some known / fixed header file, or through asm/system.h, or anyhow -- it doesn't really matter)
3. And:
#ifndef CONFIG_WANTS_HARD_IRQ_DISABLE
#define hard_irq_disable() do { } while(0)
#endif
We don't need to standardize on some particular arch-specific header filename in this case.
Comments?
Satyam
Quoted 5 lines
So you're saying that this mechanism forces the arch (that really wants
hard_irq_disable) to _#define_hard_irq_disable(as a macro), and if it implements it as an inline function, for example, then we're screwed?
No. The idea is to do like we did for a few other things already (according to Linus request in fact), which is to write
static inline void hard_irq_disable(void)
{
.../...
}#define hard_irq_disable hard_irq_disable
This is nicer than having an ARCH_HAS_xxx
Quoted 8 lines
1. Introduce some
CONFIG_WANTS_HARD_IRQ_DISABLEthat is #defined (or left undefined) by thearch/.../defconfig(depending upon whether or not that arch implements ahard_irq_disable()for itself or not)2. Then pull-in that code into
include/linux/interrupt.hsomehow (through some known / fixed header file, or throughasm/system.h, or anyhow -- it doesn't really matter)
Quoted 5 lines
3. And:
#ifndef
CONFIG_WANTS_HARD_IRQ_DISABLE
#definehard_irq_disable()do { } while(0)
#endif
Well, last time I tried that, Linus NACKed it in favor of what I described above.
Quoted 2 lines
We don't need to standardize on some particular arch-specific header filename in this case.
True, that's my main problem here. Though really only the archs who actually implement something special here need to be careful.
Ben.
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 5 lines
So you're saying that this mechanism forces the arch (that really wants
hard_irq_disable) to _#define_hard_irq_disable(as a macro), and if it implements it as an inline function, for example, then we're screwed?No. The idea is to do like we did for a few other things already (according to Linus request in fact), which is to write
Quoted 7 lines
static inline void
hard_irq_disable(void){ .../... }#define
hard_irq_disablehard_irq_disableThis is nicer than having an
ARCH_HAS_xxx
Ok, that's reasonable, we don't want to end up with zillions of ARCH_HAS_THIS and ARCH_HAS_THAT.
But then, what _is_ the problem with your approach above? An arch that wants (and implements) hard_irq_disable will also #define that dummy macro, so we just need to pull in the appropriate header (directly, indirectly, anyhow -- we don't really care) into include/linux/interrupt.h and then just do the exact same "#ifndef hard_irq_disable" check that you're doing right now. I must be missing something trivial (either that or I need to go and have a coffee :-) because I don't see the possibility of hitting multiple _different_ definitions with the approach you mentioned just now.
Quoted 9 lines · Satyam Sharma
But then, what _is_ the problem with your approach above? An arch that wants (and implements)
hard_irq_disablewill also #define that dummy macro, so we just need to pull in the appropriate header (directly, indirectly, anyhow -- we don't really care) intoinclude/linux/interrupt.hand then just do the exact same "#ifndefhard_irq_disable" check that you're doing right now. I must be missing something trivial (either that or I need to go and have a coffee :-) because I don't see the possibility of hitting multiple _different_ definitions with the approach you mentioned just now.
Sure, the only problem is that I don't want to pull asm/hw_irq.h directly from linux/interrupts.h unless all arch maintainers around verify it's ok, because those headers are a bit of a can of worm at the moment ...
So I'd rather say that if your arch has a custom version of hard_irq_disable(), make sure that asm/system.h pulls it in a way or another. And that's already included.
Ben.