Messages in this thread | | | Date | Sun, 24 Aug 2003 12:06:51 +0900 | From | TeJun Huh <> | Subject | Re: Possible race condition in i386 global_irq_lock handling. |
| |
Hello Andrea,
On Fri, Aug 22, 2003 at 06:25:46PM +0200, Andrea Arcangeli wrote: > thanks TeJun, > > just one comment > > On Fri, Aug 22, 2003 at 10:18:40AM +0900, TeJun Huh wrote: > > 3. remove irqs_running() test from synchronize_irq() > > I'm not convinced this one is needed. An irq can still run on another > cpu but the cli();sti() may execute while it's here: > > irq running synchronize_irq() > -------------- ----------------- > do_IRQ > handle_IRQ_event > cli() > sti() > > irq_enter -> way too late > > in short, doing irqs_running() doesn't seem to weaken the semantics of > synchronize_irq() to me. > > I think it should be changed this way instead: > > void synchronize_irq(void) > { > smp_mb(); > if (irqs_running()) { > /* Stupid approach */ > cli(); > sti(); > } > } > > to be sure to read the local irq area after the previous code (the > test_and_set_bit of the global_irq_lock of a cli() in your version would > achieve the same implicit smp_mb too, so maybe your only point for doing > cli()/sti() was to execute the smp_mb before the irqs_running?). the > above version is more finegrined and it looks equivalent to yours. > > Andrea
Yes, you're right. Adding just smp_mb() should guarantee that no cpu is executing interrupt handler which may not see memory contents modified before synchronize_irq() after synchronize_irq() returns. I think we need some decent comments there. :-)
As now I know that test_and_set_bit() implies memory barrier, smb_mb__after_clear_bit() can be removed. I'll make and post a patch which fixes this race and the bh race of the other thread.
Thanks.
-- tejun - 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/
| |