[PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Zwane Mwaikambo]

This little bit was missing from the previous patch. It will enable
interrupts whilst a cpu is spinning on a lock in spin_lock_irq as well as
spin_lock_irqsave. UP/SMP compile and runtime/stress tested on i386,
UP/SMP compile tested on amd64.

Signed-off-by: Zwane Mwaikambo <zwane@fsmlabs.com>

 include/asm-i386/spinlock.h |    1 +
 include/linux/spinlock.h    |    9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.6-mm5/include/asm-i386/spinlock.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.6-mm5/include/asm-i386/spinlock.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 spinlock.h
--- linux-2.6.6-mm5/include/asm-i386/spinlock.h	22 May 2004 16:45:24 -0000	1.1.1.1
+++ linux-2.6.6-mm5/include/asm-i386/spinlock.h	26 May 2004 05:36:34 -0000
@@ -174,6 +174,7 @@ here:
 		:"=m" (lock->lock) : : "memory");
 }

+#define _raw_spin_lock_irq(lock)	_raw_spin_lock_flags(lock, X86_EFLAGS_IF)
 static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
Index: linux-2.6.6-mm5/include/linux/spinlock.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.6-mm5/include/linux/spinlock.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 spinlock.h
--- linux-2.6.6-mm5/include/linux/spinlock.h	22 May 2004 16:45:22 -0000	1.1.1.1
+++ linux-2.6.6-mm5/include/linux/spinlock.h	26 May 2004 06:36:55 -0000
@@ -44,9 +44,14 @@
 #ifdef CONFIG_SMP
 #include <asm/spinlock.h>

+#ifndef _raw_spin_lock_irq
+#define _raw_spin_lock_irq(lock)		_raw_spin_lock(lock)
+#endif
+
 #else

-#define _raw_spin_lock_flags(lock, flags) _raw_spin_lock(lock)
+#define _raw_spin_lock_flags(lock, flags)	_raw_spin_lock(lock)
+#define _raw_spin_lock_irq(lock)		_raw_spin_lock(lock)

 #if !defined(CONFIG_PREEMPT) && !defined(CONFIG_DEBUG_SPINLOCK)
 # define atomic_dec_and_lock(atomic,lock) atomic_dec_and_test(atomic)
@@ -289,7 +294,7 @@ do { \
 do { \
 	local_irq_disable(); \
 	preempt_disable(); \
-	_raw_spin_lock(lock); \
+	_raw_spin_lock_irq(lock); \
 } while (0)

 #define spin_lock_bh(lock) \

Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Keith Owens]

On Wed, 26 May 2004 03:11:07 -0400 (EDT), 
Zwane Mwaikambo <zwane@fsmlabs.com> wrote:
>This little bit was missing from the previous patch. It will enable
>interrupts whilst a cpu is spinning on a lock in spin_lock_irq as well as
>spin_lock_irqsave. UP/SMP compile and runtime/stress tested on i386,
>UP/SMP compile tested on amd64.
>
>+#define _raw_spin_lock_irq(lock)	_raw_spin_lock_flags(lock, X86_EFLAGS_IF)

You are assuming that all uses of spin_lock_irq() are done when
interrupts are already enabled.  This _should_ be true, because the
matching spin_unlock_irq() will unconditionally reenable interrupts.
However I have seen buggy code where spin_lock_irq() was issued with
interrupts disabled.  By unconditionally passing X86_EFLAGS_IF, that
buggy code can now run in one of two states :-

state 1
Enter with interrupts disabled
Do some work
spin_lock_irq()
No lock contention, do not enable interrupts
Do some more work
spin_unlock_irq()

state 2
Enter with interrupts disabled
Do some work
spin_lock_irq()
Lock contention, enable interrupts, get lock, disable interrupts
Do some more work
spin_unlock_irq()

Your patch opens a window where data that was protected by the disabled
interrupt on entry becomes unprotected while waiting for the lock and
can therefore change.

It could be that I am worrying unnecessarily, after all any code that
calls spin_lock_irq() with interrupts already disabled is probably
wrong to start off with.  But it does need to be considered as a
possible failure mode.

Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Zwane Mwaikambo]

On Wed, 26 May 2004, Keith Owens wrote:

> Your patch opens a window where data that was protected by the disabled
> interrupt on entry becomes unprotected while waiting for the lock and
> can therefore change.
>
> It could be that I am worrying unnecessarily, after all any code that
> calls spin_lock_irq() with interrupts already disabled is probably
> wrong to start off with.  But it does need to be considered as a
> possible failure mode.

Granted there might be code like that, i'll throw in some debugging code
locally to test for such a condition. It wouldn't necessarily be a bug but
a very uncool way of obfuscating the locking.

Thanks,
	Zwane

Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Duncan Sands]

> However I have seen buggy code where spin_lock_irq() was issued with
> interrupts disabled. [...]

Some time ago I sent a patch to lkml that tests for this [1].
And guess what - it happens all over the place [2].  Also, the
scheduler often gets called with interrupts disabled (schedule()
does spin_lock_irq), but the cases I checked all turned out to be
OK [3].  Perhaps it is more problematic now?

Ciao,

Duncan.

[1] http://seclists.org/lists/linux-kernel/2003/May/5585.html
[2] http://seclists.org/lists/linux-kernel/2003/May/5842.html
[3] http://seclists.org/lists/linux-kernel/2003/May/5581.html

Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Zwane Mwaikambo]

On Wed, 26 May 2004, Duncan Sands wrote:

> > However I have seen buggy code where spin_lock_irq() was issued with
> > interrupts disabled. [...]
>
> Some time ago I sent a patch to lkml that tests for this [1].
> And guess what - it happens all over the place [2].  Also, the
> scheduler often gets called with interrupts disabled (schedule()
> does spin_lock_irq), but the cases I checked all turned out to be
> OK [3].  Perhaps it is more problematic now?

I'll run with the debug code and audit any suspect ones. The ones
mentioned below all seem ok.

> [1] http://seclists.org/lists/linux-kernel/2003/May/5585.html
> [2] http://seclists.org/lists/linux-kernel/2003/May/5842.html
> [3] http://seclists.org/lists/linux-kernel/2003/May/5581.html

Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Zwane Mwaikambo]

On Wed, 26 May 2004, Zwane Mwaikambo wrote:

> On Wed, 26 May 2004, Duncan Sands wrote:
>
> > > However I have seen buggy code where spin_lock_irq() was issued with
> > > interrupts disabled. [...]
> >
> > Some time ago I sent a patch to lkml that tests for this [1].
> > And guess what - it happens all over the place [2].  Also, the
> > scheduler often gets called with interrupts disabled (schedule()
> > does spin_lock_irq), but the cases I checked all turned out to be
> > OK [3].  Perhaps it is more problematic now?
>
> I'll run with the debug code and audit any suspect ones. The ones
> mentioned below all seem ok.

The oft hit one is triggered from the timer softirq;

Badness in __run_timers at kernel/timer.c:427
 [<c0107705>] dump_stack+0x15/0x20
 [<c012d497>] run_timer_softirq+0x2e7/0x2f0
 [<c0128bc8>] __do_softirq+0xb8/0xc0
 [<c0128c05>] do_softirq+0x35/0x40
 [<c0108fb5>] do_IRQ+0x1a5/0x240

static inline void __run_timers(tvec_base_t *base)
{
	struct timer_list *timer;

	spin_lock_irq(&base->lock); <==
	...
		timer->base = NULL;
		spin_unlock_irq(&base->lock);
		fn(data);
		spin_lock_irq(&base->lock);
	...
	spin_unlock_irq(&base->lock);
}

Most of them would be because the timer irq is setup with SA_INTERRUPT,
this particular one looks safe too, we're on the exit path of irq handling
and there is no critical section to protect. We're also about to
unconditionally enable interrupts again before running the timers. There
also isn't much contention on this lock on most workloads, some
heavy tcp networking load should help that out.

I'll look for others too.

	Zwane