lkml.org 
[lkml]   [1998]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2005-03-22 13:46    [W:0.026 / U:60.744 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site