Messages in this thread Patches in this message | | | Date | Mon, 28 Dec 1998 03:38:49 +0100 (CET) | From | Andrea Arcangeli <> | Subject | Re: [patch] disable_bh/enable_bh race fix [Re: Program to freeze keyboard in 2.1.131] |
| |
On Sun, 27 Dec 1998, Linus Torvalds wrote:
> You missed the basic problem: > > CPU#1 CPU#2 > > start_bh_atomic() start_bh_atomic() > access bh count access bh count > end_bh_atomic() end_bh_atomic()
Arggh, I confused global_bh_lock with global_bh_count!! Excuse me! I was only reading the first characters: "atomic_inc(&global_bh_" in start/stop_bh_atomic(). It's obvious that it couldn' t be global_bh_count to be increased because otherwise start_bh_atomic() would istant deadlock in sychronize_bh! Stupid me again...
Your point is clear to me now, thanks for your paticence...
--------------------------------------------------------------------------
But now I can' t see again why we doesn't wait_on_bh in synchronize_bh if we are running in a irq handler...
get_active_bhs() and other bh functions seems just safe to me also without holding the spinlock.
Here a new patch against 2.1.132 that should fix all races in disable/enable_bh (it's very late again but I try anyway... ;). I checked that the console race get fixed with it too (as with the atomic_t patch) and this new one should give consistency also to bh_mask.
Index: linux/include/linux/interrupt.h diff -u linux/include/linux/interrupt.h:1.1.1.3 linux/include/linux/interrupt.h:1.1.1.1.2.6 --- linux/include/linux/interrupt.h:1.1.1.3 Wed Dec 23 15:24:21 1998 +++ linux/include/linux/interrupt.h Mon Dec 28 03:26:27 1998 @@ -17,7 +17,8 @@ extern volatile unsigned char bh_running; -extern atomic_t bh_mask_count[32]; +extern spinlock_t bh_lock; +extern int bh_mask_count[32]; extern unsigned long bh_active; extern unsigned long bh_mask; extern void (*bh_base[32])(void); Index: linux/kernel/softirq.c diff -u linux/kernel/softirq.c:1.1.1.3 linux/kernel/softirq.c:1.1.1.1.2.5 --- linux/kernel/softirq.c:1.1.1.3 Wed Dec 23 15:25:17 1998 +++ linux/kernel/softirq.c Mon Dec 28 03:26:29 1998 @@ -20,7 +20,8 @@ /* intr_count died a painless death... -DaveM */ -atomic_t bh_mask_count[32]; +spinlock_t bh_lock = SPIN_LOCK_UNLOCKED; +int bh_mask_count[32]; unsigned long bh_active = 0; unsigned long bh_mask = 0; void (*bh_base[32])(void); Index: linux/include/asm-i386/softirq.h diff -u linux/include/asm-i386/softirq.h:1.1.1.2 linux/include/asm-i386/softirq.h:1.1.1.1.2.5 --- linux/include/asm-i386/softirq.h:1.1.1.2 Wed Dec 23 15:22:50 1998 +++ linux/include/asm-i386/softirq.h Mon Dec 28 03:26:26 1998 @@ -12,7 +12,7 @@ extern inline void init_bh(int nr, void (*routine)(void)) { bh_base[nr] = routine; - atomic_set(&bh_mask_count[nr], 0); + bh_mask_count[nr] = 0; bh_mask |= 1 << nr; } @@ -96,15 +96,23 @@ */ extern inline void disable_bh(int nr) { + unsigned long flags; + + spin_lock_irqsave(&bh_lock, flags); bh_mask &= ~(1 << nr); - atomic_inc(&bh_mask_count[nr]); + bh_mask_count[nr]++; + spin_unlock_irqrestore(&bh_lock, flags); synchronize_bh(); } extern inline void enable_bh(int nr) { - if (atomic_dec_and_test(&bh_mask_count[nr])) + unsigned long flags; + + spin_lock_irqsave(&bh_lock, flags); + if (!--bh_mask_count[nr]) bh_mask |= 1 << nr; + spin_unlock_irqrestore(&bh_lock, flags); } #endif /* __ASM_SOFTIRQ_H */
Here the irq.c patch that I return to don't understand why not to apply it... ;)
Index: arch/i386/kernel/irq.c =================================================================== RCS file: /var/cvs/linux/arch/i386/kernel/irq.c,v retrieving revision 1.1.1.1.2.10 diff -u -r1.1.1.1.2.10 irq.c --- irq.c 1998/12/23 00:29:43 1.1.1.1.2.10 +++ linux/arch/i386/kernel/irq.c 1998/12/27 16:30:20 @@ -21,6 +21,11 @@ * Copyright (C) 1998 Andrea Arcangeli */ +/* + * synchronize_bh can't synchronize _only_ if we are in a bh handler. + * Copyright (C) 1998 Andrea Arcangeli + */ + #include <linux/ptrace.h> #include <linux/errno.h> #include <linux/kernel_stat.h> @@ -449,7 +454,8 @@ */ void synchronize_bh(void) { - if (atomic_read(&global_bh_count) && !in_interrupt()) + if (atomic_read(&global_bh_count) && + !local_bh_count[smp_processor_id()]) wait_on_bh(); }
Andrea Arcangeli
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |