Messages in this thread | | | Date | Fri, 11 Apr 2003 00:15:54 -0700 | From | george anzinger <> | Subject | Re: 2.4.20 kernel/timer.c may incorrectly reenable interrupts |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |