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

[george anzinger <george@mvista.com>]

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