Re: [patch] irqlock patch -G3. [was Re: odd memory corruption in 2.5.27?]

[george anzinger <george@mvista.com>]

Ingo Molnar wrote:
> 
> On Tue, 23 Jul 2002, george anzinger wrote:
> 
> > I just spent a month tracking down this issue.  It comes
> > down to the slab allocater using per cpu data structures and
> > protecting them with a combination of interrupt disables and
> > spin_locks.  Preemption is allowed (incorrectly) if
> > interrupts are off and preempt_count goes to zero on the
> > spin_unlock. [...]
> 
> > The proposed fix is to catch the attempted preemption in
> > preempt_schedule() and just return if the interrupt system
> > is off. [...]
> 
> this is most definitely not the correct fix ...
> 
> i'm quite convinced that the fix is to avoid illegal preemption, not to
> work it around.

I like this.  The only change I would make is to enable
interrupts in exit.c and entry.S where they are only enabled
under the debug condition now, (mostly I try to avoid
Heisenberg).  I really do like the ability to track down
this problem by just turing on the debug option.
> 
> i've written debugging code that caught and reported this slab.c bug
> within minutes. 

Yeah, its easy when you know what to look for :)

> The code detects the irqs-off condition in schedule(). You
> can find it my latest irqlock patchset, at:
> 
>    http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-G3
> 
> it fixes this and other related bugs as well.
> 
> Changes in -G3:
> 
>  - slab.c needs to spin_unlock_no_resched(), instead of spin_unlock(). (It
>    also has to check for preemption in the right spot.) This should fix
>    the memory corruption.
> 
>  - irq_exit() needs to run softirqs if interrupts not active - in the
>    previous patch it ran them when preempt_count() was 0, which is
>    incorrect.
> 
>  - spinlock macros are updated to enable preemption after enabling
>    interrupts. Besides avoiding false positive warnings, this also
> 
>  - fork.c has to call scheduler_tick() with preemption disabled -
>    otherwise scheduler_tick()'s spin_unlock can preempt!
> 
>  - irqs_disabled() macro introduced.
> 
>  - [ all other local_irq_enable() or sti instances conditional on
>      CONFIG_DEBUG_IRQ_SCHEDULE are to fix false positive warnings. ]
> 
> Changes in -G0:
> 
>  - fix buggy in_softirq(). Fortunately the bug made the test broader,
>    which didnt result in algorithmical breakage, just suboptimal
>    performance.
> 
>  - move do_softirq() processing into irq_exit() => this also fixes the
>    softirq processing bugs present in apic.c IRQ handlers that did not
>    test for softirqs after irq_exit().
> 
>  - simplify local_bh_enable().
> 
> Changes in -F9:
> 
>  - replace all instances of:
> 
>         local_save_flags(flags);
>         local_irq_disable();
> 
>    with the shorter form of:
> 
>         local_irq_save(flags);
> 
>   about 30 files are affected by this change.
> 
> Changes in -F8:
> 
>  - preempt/hardirq/softirq count separation, cleanups.
> 
>  - skbuff.c fix.
> 
>  - use irq_count() in scheduler_tick()
> 
> Changes in -F3:
> 
>  - the entry.S cleanups/speedups by Oleg Nesterov.
> 
>  - a rather critical synchronize_irq() bugfix: if a driver frees an
>    interrupt that is still being probed then synchronize_irq() locks up.
>    This bug has caused a spurious boot-lockup on one of my testsystems,
>    ifconfig would lock up trying to close eth0.
> 
>  - remove duplicate definitions from asm-i386/system.h, this fixes
>    compiler warnings.
> 
>         Ingo

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml