Re: 2.4.20 kernel/timer.c may incorrectly reenable interrupts

george anzinger
2003-04-11T07:04:35Z

Keith Owens wrote:
> 2.4.20 kernel/timer.c
>
> static inline void update_times(void)
> {
> unsigned long ticks;
>
> /*
> * update_times() is run from the raw timer_bh handler so we
> * just know that the irqs are locally enabled and so we don't
> * need to save/restore the flags of the local CPU here. -arca
> */
> write_lock_irq(&xtime_lock);
> vxtime_lock();
>
> ticks = jiffies - wall_jiffies;
> if (ticks) {
> wall_jiffies += ticks;
> update_wall_time(ticks);
> }
> vxtime_unlock();
> write_unlock_irq(&xtime_lock);
> calc_load(ticks);
> }
>
> I hit one case when the routine was called with interrupts disabled and
> it unconditionally enabled them, with nasty side effects. Code fragment
>
> local_irq_save();
> local_bh_disable();
> ....
> local_bh_enable();
> local_irq_restore();
>
> local_bh_enable() checks for pending softirqs, finds that there is an
> outstanding timer bh and runs it. do_softirq() -> tasklet_hi_action()
> -> bh_action() -> timer_bh() -> update_times() which unconditionally
> reenables interrupts. Then the timer code issued cli(), because
> interrupts were incorrectly reenabled it tried to get the global cli
> lock and hung.

If you look at do_softirq() you will see that it enables irqs
unconditionally while calling pending functions. It does, however,
save the irq on entry and restore it on exit (seems strange eh).
>
> There is no documentation that defines the required nesting order of
> local_irq and local_bh. Even if the above code fragment is deemed to
> be illegal, there are uses of local_bh_enable() all through the kernel,
> it will be difficult to prove that none of them are called with
> interrupts disabled. If there is any chance that local_bh_enable() is
> called with interrupts off, update_times() is wrong.

IMHO, update_times() is right! The code fragment you found is wrong.
If there is a real need we could code up a check to see if
local_bh_enable() is called with interrupts off.

As machines get faster and faster, it will be come more and more of a
burden to "stop the world" and sync with the interrupt system, which
is running at a much slower speed. This is what the cli / sti/
restore flags causes. I saw one test where the time to do the cli was
as long as the run_timer_list code, for example.

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