interrupt checks for spinlocks
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines
Date: Sun, 3 Nov 2002 16:28:13 -0800
From: William Lee Irwin III <[email protected]>Quoted 5 lines
Quoted 2 lines
(1) check that spinlocks are not taken in interrupt context without interrupts disabled
Bill, why is this bad? I routinely use this technique. -- Pete
If you receive the same interrupt on the same cpu and re-enter code that does that, you will deadlock.
How would that happen? I thought it was not possible to re-enter an interrupt, and that it was pretty fundamental for Linux. When did we allow it, and what are implications for architectures?
-- Pete
From: William Lee Irwin III <[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
(1) check that spinlocks are not taken in interrupt context without interrupts disabled
Quoted 1 line · Pete Zaitcev
Quoted 1 line
Quoted 1 line
Bill, why is this bad? I routinely use this technique.
From: William Lee Irwin III <[email protected]>
Quoted 2 lines
Quoted 2 lines
If you receive the same interrupt on the same cpu and re-enter code that does that, you will deadlock.
Quoted 3 lines · Pete Zaitcev
How would that happen? I thought it was not possible to re-enter an interrupt, and that it was pretty fundamental for Linux. When did we allow it, and what are implications for architectures?
This non-reentrant stuff hurts my head. Another patch down the toilet, I guess.
Bill
Quoted 2 lines · William Lee Irwin III
This non-reentrant stuff hurts my head. Another patch down the toilet, I guess.
No, I think you have a good idea. Pete is right, though, the current interrupt is disabled... but normally the other interrupts are still enabled.
Your ideas #2, #3, and #4 are good.
Because once the lock is tainted, you still want to ensure process context disables interrupts before grabbing the lock.
Robert Love
Quoted 2 lines · William Lee Irwin III
Quoted 2 lines
This non-reentrant stuff hurts my head. Another patch down the toilet, I guess.
Quoted 7 lines · Robert Love
No, I think you have a good idea. Pete is right, though, the current interrupt is disabled... but normally the other interrupts are still enabled.
Your ideas #2, #3, and #4 are good.
Because once the lock is tainted, you still want to ensure process context disables interrupts before grabbing the lock.
Robert Love
I'll go figure out why before posting a follow-up. This is not doing what I wanted it to because the only one I originally wanted was (1), having to do with interrupt-time recursion on rwlocks and writer starvation caused by it.
Bill
Quoted 4 lines · William Lee Irwin III
I'll go figure out why before posting a follow-up. This is not doing what I wanted it to because the only one I originally wanted was (1), having to do with interrupt-time recursion on rwlocks and writer starvation caused by it.
You can do #1, but you need to figure out if your interrupt is the only interrupt using the lock or not (possibly hard).
In other words, a lock unique to your interrupt handler does not need to disable interrupts (since only that handler can grab the lock and it is disabled).
If other handlers can grab the lock, interrupts need to be disabled.
So a test of irqs_disabled() would show a false-positive in the first case. No easy way to tell..
Robert Love
Quoted 4 lines · William Lee Irwin III
Quoted 4 lines
I'll go figure out why before posting a follow-up. This is not doing what I wanted it to because the only one I originally wanted was (1), having to do with interrupt-time recursion on rwlocks and writer starvation caused by it.
Quoted 9 lines · Robert Love
You can do #1, but you need to figure out if your interrupt is the only interrupt using the lock or not (possibly hard).
In other words, a lock unique to your interrupt handler does not need to disable interrupts (since only that handler can grab the lock and it is disabled).
If other handlers can grab the lock, interrupts need to be disabled. So a test ofirqs_disabled()would show a false-positive in the first case. No easy way to tell..
Robert Love
Sounds like I rip out that check and ignore the fact it's useless for its original intended purpose. No matter, it'll turn up something.
Incoming.
Bill
Quoted 9 lines · Robert Love
You can do #1, but you need to figure out if your interrupt is the only interrupt using the lock or not (possibly hard).
In other words, a lock unique to your interrupt handler does not need to disable interrupts (since only that handler can grab the lock and it is disabled).
If other handlers can grab the lock, interrupts need to be disabled. So a test ofirqs_disabled()would show a false-positive in the first case. No easy way to tell..
Robert Love
Attempt #1:
===== include/linux/spinlock.h 1.18 vs edited =====
include/linux/spinlock.h +44 -0
@@ -38,6 +38,44 @@#include <asm/spinlock.h> /* +* if a lock is ever taken in interrupt context, it must always be +* taken with interrupts disabled. If a locking call is made that +* unconditionally disables and then re-enables interrupts, it must +* be made with interrupts enabled. +*/ +#ifndef irq_tainted_lock +#define irq_tainted_lock (lock) 0 +#endif + +#ifndef irq_taint_lock +#define irq_taint_lock (lock) do { } while (0) +#define + +#ifndef CONFIG_DEBUG_SPINLOCK +#define check_spinlock_irq (lock) do { } while (0) +#define check_spinlock_irqs_disabled (lock) do { } while (0) +#else +#define check_spinlock_irq (lock) \ + do { \ + if (irqs_disabled()) { \ + printk("spinlock taken unconditionally " \ + "re-enabling interrupts\n"); \ + dump_stack(); \ + } \ + } while (0) +#define check_spinlock_irqs_disabled (lock) \ + do { \ + if (in_interrupt()) \ + irq_taint_lock(lock); \ + else if (irq_tainted_lock(lock)) { \ + printk("spinlock taken in process context " \ + "without disabling interrupts\n"); \ + dump_stack(); \ + } \ + } while (0) +#endif + +/* * !CONFIG_SMP and spin_lock_init not previously defined * (e.g. by including include/asm/spinlock.h) */
@@ -87,6 +125,7 @@*/ #define spin_lock (lock) \ do { \ + check_spinlock_irqs_disabled(lock); \ preempt_disable(); \ _raw_spin_lock(lock); \ } while(0)
@@ -102,6 +141,7 @@#define read_lock (lock) \ do { \ + check_spinlock_irqs_disabled(lock); \ preempt_disable(); \ _raw_read_lock(lock); \ } while(0)
@@ -114,6 +154,7 @@#define write_lock (lock) \ do { \ + check_spinlock_irqs_disabled(lock); \ preempt_disable(); \ _raw_write_lock(lock); \ } while(0)
@@ -136,6 +177,7 @@#define spin_lock_irq (lock) \ do { \ + check_spinlock_irq(lock); \ local_irq_disable(); \ preempt_disable(); \ _raw_spin_lock(lock); \
@@ -157,6 +199,7 @@#define read_lock_irq (lock) \ do { \ + check_spinlock_irq(lock); \ local_irq_disable(); \ preempt_disable(); \ _raw_read_lock(lock); \
@@ -178,6 +221,7 @@#define write_lock_irq (lock) \ do { \ + check_spinlock_irq(lock); \ local_irq_disable(); \ preempt_disable(); \ _raw_write_lock(lock); \
Quoted 5 lines · Robert Love
In other words, a lock unique to your interrupt handler does not need to disable interrupts (since only that handler can grab the lock and it is disabled).
If other handlers can grab the lock, interrupts need to be disabled.
The only way would be running with SA_INTERRUPT for that isr and any others on that line which might contend for the same lock. Determining otherwise seems like too much trouble, and anyway i can't recall ever seeing such a scenario in drivers/ Basically i think we should forget about option 1.
Zwane
Signature · 1 line
function.linuxpower.ca