Re: [patch 0/8] Allow inlined spinlocks again V5

[Heiko Carstens <heiko.carstens@de.ibm.com>]

On Sat, Aug 29, 2009 at 01:16:42PM +0200, Ingo Molnar wrote:
> * Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > This patch set allows to have inlined spinlocks again.
> > 
> > V5: simplify ifdefs as pointed out by Linus. Fix architecture compile
> >     breakages caused by this change.
> > 
> > Linus, Ingo, do you still have objections?
> 
> Ok, this looks pretty clean.
> 
> Two small comments. One (small) worry is that the configuration 
> space of this construct:
> 
> +#define __spin_lock_is_small
> [...]
> 
> Is 2^28.
> 
> Could we perhaps shape this in a form that makes it 'formally' a 
> manually tuned inlining decision - where we already accept such kind 
> of per function decisions and know how to handle them?
> 
> I.e. rename it to something like:
> 
>   #define __always_inline__write_unlock_irqrestore
> 
> That way it fits into our existing forced-inlining attributes 
> (visually and name-space wise), which means the addition of 'inline' 
> or '__forced_inline', or the removal of such attributes.

Ok, I'll wait for more comments and post an updated version.

> The other comment i have: you dont seem to have preserved the 
> current auto-inlining of spin_lock() we did before, have you? 
> Without that this patchset causes a (small) performance regression 
> on every architectures but s390.

I assume you're talking of spin_unlock() and not spin_lock()?

It still gets inlined (spinlock.h):

/*
 * We inline the unlock functions in the nondebug case:
 */
#if defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT) || \
	!defined(CONFIG_SMP)
# define spin_unlock(lock)              _spin_unlock(lock)
[...]
#else
# define spin_unlock(lock) \
	do {__raw_spin_unlock(&(lock)->raw_lock); __release(lock); } while (0)
[...]

Or in other words, specifying __spin_lock_is_small causes only that the
out-of-line variant doesn't get generated anymore.
It gets inlined regardless (if config options allow).

Now this could be simplified/cleanup by doing something like this:

/*
 * We inline the unlock functions in the nondebug case:
 */
#if defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT) || \
	!defined(CONFIG_SMP)
# define spin_unlock(lock)              _spin_unlock(lock)
[...]
#else
# define spin_unlock(lock) \
	do {__raw_spin_unlock(&(lock)->raw_lock); __release(lock); } while (0)
[...]

However it sucks a bit that we have two different ways to force inlining.
So we could and probably should simplify this. The patch below (untested)
would do that:

---
 include/linux/spinlock.h         |   46 +++++----------------------------------
 include/linux/spinlock_api_smp.h |   12 ++++++++++
 2 files changed, 18 insertions(+), 40 deletions(-)

Index: linux-2.6/include/linux/spinlock.h
===================================================================
--- linux-2.6.orig/include/linux/spinlock.h
+++ linux-2.6/include/linux/spinlock.h
@@ -259,50 +259,16 @@ static inline void smp_mb__after_lock(vo
 
 #define spin_lock_irq(lock)		_spin_lock_irq(lock)
 #define spin_lock_bh(lock)		_spin_lock_bh(lock)
-
 #define read_lock_irq(lock)		_read_lock_irq(lock)
 #define read_lock_bh(lock)		_read_lock_bh(lock)
-
 #define write_lock_irq(lock)		_write_lock_irq(lock)
 #define write_lock_bh(lock)		_write_lock_bh(lock)
-
-/*
- * We inline the unlock functions in the nondebug case:
- */
-#if defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT) || \
-	!defined(CONFIG_SMP)
-# define spin_unlock(lock)		_spin_unlock(lock)
-# define read_unlock(lock)		_read_unlock(lock)
-# define write_unlock(lock)		_write_unlock(lock)
-# define spin_unlock_irq(lock)		_spin_unlock_irq(lock)
-# define read_unlock_irq(lock)		_read_unlock_irq(lock)
-# define write_unlock_irq(lock)		_write_unlock_irq(lock)
-#else
-# define spin_unlock(lock) \
-    do {__raw_spin_unlock(&(lock)->raw_lock); __release(lock); } while (0)
-# define read_unlock(lock) \
-    do {__raw_read_unlock(&(lock)->raw_lock); __release(lock); } while (0)
-# define write_unlock(lock) \
-    do {__raw_write_unlock(&(lock)->raw_lock); __release(lock); } while (0)
-# define spin_unlock_irq(lock)			\
-do {						\
-	__raw_spin_unlock(&(lock)->raw_lock);	\
-	__release(lock);			\
-	local_irq_enable();			\
-} while (0)
-# define read_unlock_irq(lock)			\
-do {						\
-	__raw_read_unlock(&(lock)->raw_lock);	\
-	__release(lock);			\
-	local_irq_enable();			\
-} while (0)
-# define write_unlock_irq(lock)			\
-do {						\
-	__raw_write_unlock(&(lock)->raw_lock);	\
-	__release(lock);			\
-	local_irq_enable();			\
-} while (0)
-#endif
+#define spin_unlock(lock)		_spin_unlock(lock)
+#define read_unlock(lock)		_read_unlock(lock)
+#define write_unlock(lock)		_write_unlock(lock)
+#define spin_unlock_irq(lock)		_spin_unlock_irq(lock)
+#define read_unlock_irq(lock)		_read_unlock_irq(lock)
+#define write_unlock_irq(lock)		_write_unlock_irq(lock)
 
 #define spin_unlock_irqrestore(lock, flags)		\
 	do {						\
Index: linux-2.6/include/linux/spinlock_api_smp.h
===================================================================
--- linux-2.6.orig/include/linux/spinlock_api_smp.h
+++ linux-2.6/include/linux/spinlock_api_smp.h
@@ -60,6 +60,18 @@ void __lockfunc _read_unlock_irqrestore(
 void __lockfunc _write_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
 							__releases(lock);
 
+/*
+ * We inline the unlock functions in the nondebug case:
+ */
+#if !defined(CONFIG_DEBUG_SPINLOCK) && !defined(CONFIG_PREEMPT)
+#define __spin_unlock_is_small
+#define __read_unlock_is_small
+#define __write_unlock_is_small
+#define __spin_unlock_irq_is_small
+#define __read_unlock_irq_is_small
+#define __write_unlock_irq_is_small
+#endif
+
 #ifndef CONFIG_DEBUG_SPINLOCK
 #ifndef CONFIG_GENERIC_LOCKBREAK