Skip to contentSkip to conversation

spin_lock_irq: Enable interrupts while spinning -- preperatory patch

RFC PATCH In review Ravikiran G Thirumalai 2 participants 4 messages Jan 3, 2007 Locking primitives

lore
Share & tools
lore

Ravikiran G Thirumalai

author
#0 lore raw

RFC PATCH 1/2 spin_lock_irq: Enable interrupts while spinning -- preperatory patch

There seems to be no good reason for spin_lock_irq to disable interrupts while spinning. Zwane Mwaikambo had an implementation couple of years ago, and the only objection seemed to be concerns about buggy code using spin_lock_irq whilst interrupts disabled
http://lkml.org/lkml/2004/5/26/87
That shouldn't be a concern anymore. Besides, spin_lock_irqsave now enables interrupts while spinning.

As to the motivation, on a Sun x4600 8 socket 16 core x86_64 NUMA box, we notice softlockups and quite a few lost timer ticks under extreme memory pressure. The reason turned out to be that zone->lru_lock tends to get heavily contended, and NUMA nodes try to grab the locks using spin_lock_irq. Instrumentation showed us that interrupt hold offs can last for a few seconds, and even the main timer interrupts get held off for long -- which is not good. Enabling interrupts for spinlocks while spinning made the machine responsive and the softlockups/lost ticks went away.

Although the scenario above was an extreme condition (very high memory pressure), I guess it still makes sense to enable interrupts while spinning for a lock.

The following patches do just that. The first patch is preparatory in nature and the second one changes the x86_64 implementation of spin_lock_irq. Patch passed overnight runs of kernbench and dbench on 4 way x86_64 smp.

Comments?

Thanks,
Kiran

Preparatory patch to enable interrupts while spinning with spinlock irqs. Any arch which needs this feature just has to implement __raw_spin_lock_irq

Signed-off by: Pravin B. Shelar <[email protected]> Signed-off by: Ravikiran Thirumalai <[email protected]> Signed-off by: Shai Fultheim <[email protected]>

include/asm-alpha/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-alpha/spinlock.h
===================================================================
@@ -13,6 +13,7 @@
  */
 
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_is_locked(x)	((x)->lock != 0)
 #define __raw_spin_unlock_wait(x) \
 		do { cpu_relax(); } while ((x)->lock)
include/asm-arm/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-arm/spinlock.h
===================================================================
@@ -22,6 +22,7 @@
 	do { while (__raw_spin_is_locked(lock)) cpu_relax(); } while (0)
 
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock, flags) __raw_spin_lock(lock)
 
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
include/asm-cris/arch-v32/spinlock.h +1 -1
Index: linux-2.6.20-rc1/include/asm-cris/arch-v32/spinlock.h
===================================================================
@@ -36,7 +36,7 @@ static inline void _raw_spin_lock_flags 
 {
   _raw_spin_lock(lock);
 }
-
+#define __raw_spin_lock_irq(lock) _raw_spin_lock(lock)
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
include/asm-i386/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-i386/spinlock.h
===================================================================
@@ -82,6 +82,7 @@ static inline void __raw_spin_lock_flags
 	 	  CLI_STI_INPUT_ARGS
 		: "memory" CLI_STI_CLOBBERS);
 }
+# define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #endif
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)
include/asm-ia64/spinlock.h +2 -1
Index: linux-2.6.20-rc1/include/asm-ia64/spinlock.h
===================================================================
@@ -87,7 +87,7 @@ __raw_spin_lock_flags (raw_spinlock_t *l
 }
 
 #define __raw_spin_lock(lock) __raw_spin_lock_flags(lock, 0)
-
+# define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 /* Unlock by doing an ordered store and releasing the cacheline with nta */
 static inline void __raw_spin_unlock(raw_spinlock_t *x) {
 	barrier();

@@ -96,6 +96,7 @@ static inline void __raw_spin_unlock(raw
 
 #else /* !ASM_SUPPORTED */
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+# define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 # define __raw_spin_lock(x)								\
 do {											\
 	__u32 *ia64_spinlock_ptr = (__u32 *) (x);					\
include/asm-m32r/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-m32r/spinlock.h
===================================================================
@@ -26,6 +26,7 @@
 
 #define __raw_spin_is_locked(x)		(*(volatile int *)(&(x)->slock) <= 0)
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_unlock_wait(x) \
 		do { cpu_relax(); } while (__raw_spin_is_locked(x))
include/asm-mips/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-mips/spinlock.h
===================================================================
@@ -18,6 +18,7 @@
 
 #define __raw_spin_is_locked(x)       ((x)->lock != 0)
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_unlock_wait(x) \
 	do { cpu_relax(); } while ((x)->lock)
include/asm-parisc/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-parisc/spinlock.h
===================================================================
@@ -12,6 +12,7 @@ static inline int __raw_spin_is_locked(r
 }
 
 #define __raw_spin_lock(lock) __raw_spin_lock_flags(lock, 0)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_unlock_wait(x) \
 		do { cpu_relax(); } while (__raw_spin_is_locked(x))
include/asm-powerpc/spinlock.h +2 -0
Index: linux-2.6.20-rc1/include/asm-powerpc/spinlock.h
===================================================================
@@ -138,6 +138,8 @@ static void __inline__ __raw_spin_lock_f
 	}
 }
 
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
+
 static __inline__ void __raw_spin_unlock(raw_spinlock_t *lock)
 {
 	SYNC_IO;
include/asm-ppc/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-ppc/spinlock.h
===================================================================
@@ -13,6 +13,7 @@
 #define __raw_spin_unlock_wait(lock) \
 	do { while (__raw_spin_is_locked(lock)) cpu_relax(); } while (0)
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
include/asm-s390/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-s390/spinlock.h
===================================================================
@@ -54,6 +54,7 @@ _raw_compare_and_swap(volatile unsigned 
 
 #define __raw_spin_is_locked(x) ((x)->owner_cpu != 0)
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_unlock_wait(lock) \
 	do { while (__raw_spin_is_locked(lock)) \
 		 _raw_spin_relax(lock); } while (0)
include/asm-sh/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-sh/spinlock.h
===================================================================
@@ -18,6 +18,7 @@
 
 #define __raw_spin_is_locked(x)	((x)->lock != 0)
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #define __raw_spin_unlock_wait(x) \
 	do { cpu_relax(); } while (__raw_spin_is_locked(x))
include/asm-sparc/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-sparc/spinlock.h
===================================================================
@@ -179,6 +179,7 @@ static inline int __read_trylock(raw_rwl
 #define __raw_write_unlock(rw)	do { (rw)->lock = 0; } while(0)
 
 #define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 
 #define _raw_spin_relax(lock)	cpu_relax()
 #define _raw_read_relax(lock)	cpu_relax()
include/asm-sparc64/spinlock.h +2 -0
Index: linux-2.6.20-rc1/include/asm-sparc64/spinlock.h
===================================================================
@@ -103,6 +103,8 @@ static inline void __raw_spin_lock_flags
 	: "memory");
 }
 
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
+
 /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
 
 static void inline __read_lock(raw_rwlock_t *lock)
include/asm-x86_64/spinlock.h +1 -0
Index: linux-2.6.20-rc1/include/asm-x86_64/spinlock.h
===================================================================
@@ -63,6 +63,7 @@ static inline void __raw_spin_lock_flags
 		"5:\n\t"
 		: "+m" (lock->slock) : "r" ((unsigned)flags) : "memory");
 }
+#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
 #endif
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)
include/linux/spinlock.h +2 -0
Index: linux-2.6.20-rc1/include/linux/spinlock.h
===================================================================
@@ -138,6 +138,7 @@ do {								\
 #ifdef CONFIG_DEBUG_SPINLOCK
  extern void _raw_spin_lock(spinlock_t *lock);
 #define _raw_spin_lock_flags(lock, flags) _raw_spin_lock(lock)
+#define _raw_spin_lock_irq(lock) _raw_spin_lock(lock)
  extern int _raw_spin_trylock(spinlock_t *lock);
  extern void _raw_spin_unlock(spinlock_t *lock);
  extern void _raw_read_lock(rwlock_t *lock);

@@ -148,6 +149,7 @@ do {								\
  extern void _raw_write_unlock(rwlock_t *lock);
 #else
 # define _raw_spin_lock(lock)		__raw_spin_lock(&(lock)->raw_lock)
+# define _raw_spin_lock_irq(lock) 	__raw_spin_lock_irq(&(lock)->raw_lock)
 # define _raw_spin_lock_flags(lock, flags) \
 		__raw_spin_lock_flags(&(lock)->raw_lock, *(flags))
 # define _raw_spin_trylock(lock)	__raw_spin_trylock(&(lock)->raw_lock)
kernel/spinlock.c +5 -1
Index: linux-2.6.20-rc1/kernel/spinlock.c
===================================================================
@@ -102,7 +102,11 @@ void __lockfunc _spin_lock_irq(spinlock_
 	local_irq_disable();
 	preempt_disable();
 	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
-	_raw_spin_lock(lock);
+#ifdef CONFIG_PROVE_LOCKING
+        _raw_spin_lock(lock);
+#else
+        _raw_spin_lock_irq(lock);
+#endif
 }
 EXPORT_SYMBOL(_spin_lock_irq);
author
#1 lore raw

RFC PATCH 2/2 spin_lock_irq: Enable interrupts while spinning -- x86_64 implementation

Implement interrupt enabling while spinning for lock for spin_lock_irq

Signed-off by: Pravin B. Shelar <[email protected]> Signed-off by: Ravikiran Thirumalai <[email protected]> Signed-off by: Shai Fultheim <[email protected]>

include/asm-x86_64/spinlock.h +15 -1
Index: linux-2.6.20-rc1/include/asm-x86_64/spinlock.h
===================================================================
@@ -63,7 +63,21 @@ static inline void __raw_spin_lock_flags
 		"5:\n\t"
 		: "+m" (lock->slock) : "r" ((unsigned)flags) : "memory");
 }
-#define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
+static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
+{
+	asm volatile(
+		"\n1:\t"
+		LOCK_PREFIX " ; decl %0\n\t"
+		"jns 2f\n"
+		"sti\n"		/* Enable interrupts during spin */
+		"3:\n"
+		"rep;nop\n\t"
+		"cmpl $0,%0\n\t"
+		"jle 3b\n\t"
+		"cli\n"
+		"jmp 1b\n"
+		"2:\t" : "+m" (lock->slock) : : "memory");
+}
 #endif
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)
#2 lore raw
In reply to Ravikiran G Thirumalai
Quoted 3 lines · On Tue, 2 Jan 2007 23:59:23 -0800 Ravikiran G Thirumalai <[email protected]>…

The following patches do just that. The first patch is preparatory in nature and the second one changes the x86_64 implementation of spin_lock_irq. Patch passed overnight runs of kernbench and dbench on 4 way x86_64 smp.

The end result of this is, I think, that i386 enables irqs while spinning in spin_lock_irqsave() but not while spinning in spin_lock_irq(). And x86_64 does the opposite.

Odd, isn't it?

author
#3 lore raw
Quoted 10 lines · Andrew Morton
Quoted 3 lines · On Tue, 2 Jan 2007 23:59:23 -0800 Ravikiran G Thirumalai <[email protected]>…

The following patches do just that. The first patch is preparatory in nature and the second one changes the x86_64 implementation of spin_lock_irq. Patch passed overnight runs of kernbench and dbench on 4 way x86_64 smp.

The end result of this is, I think, that i386 enables irqs while spinning in spin_lock_irqsave() but not while spinning in spin_lock_irq(). And x86_64 does the opposite.

No, right now we have on mainline (non PREEMPT case);

			i386				x86_64

-----------------------------------------------------------------------------
spin_lock_irq cli when spin cli when spin
spin_lock_irqsave spin with intr enabled spin with intr enabled

The posted patchset changed this to:

			i386				x86_64

-----------------------------------------------------------------------------
spin_lock_irq cli when spin spin with intr enabled
spin_lock_irqsave spin with intr enabled spin with intr enabled

Quoted 2 lines

Odd, isn't it?

Well we just implemented the x86_64 part. Here goes the i386 part as well for spin_lock_irq.

i386: Enable interrupts while spinning for a lock with spin_lock_irq

Signed-off-by Ravikiran Thirumalai <[email protected]>
include/asm-i386/spinlock.h +16 -1
Index: linux-2.6.20-rc1/include/asm-i386/spinlock.h
===================================================================
@@ -82,7 +82,22 @@ static inline void __raw_spin_lock_flags
 	 	  CLI_STI_INPUT_ARGS
 		: "memory" CLI_STI_CLOBBERS);
 }
-# define __raw_spin_lock_irq(lock) __raw_spin_lock(lock)
+
+static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
+{
+	asm volatile("\n1:\t"
+		     LOCK_PREFIX " ; decb %0\n\t"
+		     "jns 3f\n"
+		     STI_STRING "\n"
+		     "2:\t"
+		     "rep;nop\n\t"
+		     "cmpb $0,%0\n\t"
+		     "jle 2b\n\t"
+		     CLI_STRING "\n"
+		     "jmp 1b\n"
+		     "3:\n\t"
+		     : "+m" (lock->slock) : : "memory");
+}
 #endif
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)