Possible race condition in i386 global_irq_lock handling.

static inline void irq_enter(int cpu, int irq)

{
	++local_irq_count(cpu);

	while (test_bit(0,&global_irq_lock)) {
		cpu_relax();
	}
}

Is it a race condition or am I getting it horribly wrong? Thx in
advance.

Quoted 11 lines · TeJun

static inline void irq_enter(int cpu, int irq)

{
	++local_irq_count(cpu);

	while (test_bit(0,&global_irq_lock)) {
		cpu_relax();
	}
}

Is it a race condition or am I getting it horribly wrong? Thx in advance.

Yes, it's a race. Actually a variant of the race that lead to the introduction of set_current_state():

test_bit is a simple read instruction. i386 cpus are free to execute it early, i.e. they can execute it before the write part of "++local_irq_count(cpu)".

I think smp_rmb() is the right barrier - could you write a patch and send it to Marcelo?

smb_rmb is enough in practice for x86 (in asm-i386), but not the right barrier in general because rmb only serializes reads against reads, so it would also make little sense while reading the i386 code. here you've to serialize a write against a read so it would be misleading unless you know exactly the lowlevel implementations of those barriers.

smp_mb() before the while loop should be the correct barrier for all archs and the asm generated on x86 will be the same.

alpha, ia64 and x86-64 (and probably others) needs it too.

Quoted 7 lines

smb_rmb is enough in practice for x86 (in asm-i386), but not the right barrier in general because rmb only serializes reads against reads, so it would also make little sense while reading the i386 code. here you've to serialize a write against a read so it would be misleading unless you know exactly the lowlevel implementations of those barriers.

Quoted 4 lines

smp_mb() before the while loop should be the correct barrier for all archs and the asm generated on x86 will be the same.

alpha, ia64 and x86-64 (and probably others) needs it too.

Can some kind soul please provide me with the needed mini-patch. I would like to try that on my constantly crashing SMP test box...

I'm attaching patch for i386. It makes three changes.

1. add smp_mb() between local_irq_count++ and global_irq_lock test

    in irq_enter().
 2. add smp_mb__after_clear_bit() before irqs_running() test in
    wait_on_irq().

3. remove irqs_running() test from synchronize_irq()

3. remove irqs_running() test from synchronize_irq()

thanks TeJun,

just one comment

Quoted 1 line · TeJun Huh

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

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.