Re: Possible race condition in i386 global_irq_lock handling.

[TeJun Huh <tejun@aratech.co.kr>]

 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