[PATCH] spinlock.h cleanup

[PATCH] spinlock.h cleanup

[Robert Love]

Linus,

Ingo's IRQ rewrite cleaned up include/linux/spinlock.h a bit but it
still resembles a dying albatross.  Some of the defines are redundant,
the coding style used by the remaining defines is likely drug induced,
and there are a couple other issues...

	- cleanup #defines: I do not follow the rationale behind the odd
	  line-wrapped defines at the beginning of the file.  If we have 	  to
use multiple lines, then we might as well do so cleanly and
	  according to normal practice...

	- we do not need to define the spin_lock functions twice, once
	  for CONFIG_PREEMPT and once for !CONFIG_PREEMPT.  Defining
	  them once with the preempt macros will optimize away fine.

	- remove the egcs compiler workarounds: we do not need them

	- other misc. minor cleanup, improved comments, et cetera

Tested on UP, SMP, and preempt.  Object code is unchanged.  Patch is
against 2.5-bk but should apply to 2.5.29.  Please, apply.

	Robert Love

diff -urN linux-2.5.29-bk/include/linux/preempt.h linux/include/linux/preempt.h
--- linux-2.5.29-bk/include/linux/preempt.h	Mon Jul 29 15:59:01 2002
+++ linux/include/linux/preempt.h	Mon Jul 29 16:34:12 2002
@@ -1,6 +1,11 @@
 #ifndef __LINUX_PREEMPT_H
 #define __LINUX_PREEMPT_H
 
+/*
+ * include/linux/preempt.h - macros for accessing and manipulating
+ * preempt_count (used for kernel preemption, interrupt count, etc.)
+ */
+
 #include <linux/config.h>
 
 #define preempt_count() (current_thread_info()->preempt_count)
@@ -37,7 +42,7 @@
 #else
 
 #define preempt_disable()		do { } while (0)
-#define preempt_enable_no_resched()	do {} while(0)
+#define preempt_enable_no_resched()	do { } while (0)
 #define preempt_enable()		do { } while (0)
 #define preempt_check_resched()		do { } while (0)
 
diff -urN linux-2.5.29-bk/include/linux/spinlock.h linux/include/linux/spinlock.h
--- linux-2.5.29-bk/include/linux/spinlock.h	Mon Jul 29 15:59:01 2002
+++ linux/include/linux/spinlock.h	Mon Jul 29 16:34:33 2002
@@ -11,39 +11,80 @@
 #include <asm/system.h>
 
 /*
- * These are the generic versions of the spinlocks and read-write
- * locks..
+ * These are the generic versions of the spinlocks and read-write locks..
  */
-#define spin_lock_irqsave(lock, flags)		do { local_irq_save(flags);       spin_lock(lock); } while (0)
-#define spin_lock_irq(lock)			do { local_irq_disable();         spin_lock(lock); } while (0)
-#define spin_lock_bh(lock)			do { local_bh_disable();          spin_lock(lock); } while (0)
-
-#define read_lock_irqsave(lock, flags)		do { local_irq_save(flags);       read_lock(lock); } while (0)
-#define read_lock_irq(lock)			do { local_irq_disable();         read_lock(lock); } while (0)
-#define read_lock_bh(lock)			do { local_bh_disable();          read_lock(lock); } while (0)
-
-#define write_lock_irqsave(lock, flags)		do { local_irq_save(flags);      write_lock(lock); } while (0)
-#define write_lock_irq(lock)			do { local_irq_disable();        write_lock(lock); } while (0)
-#define write_lock_bh(lock)			do { local_bh_disable();         write_lock(lock); } while (0)
-
-#define spin_unlock_irqrestore(lock, flags)	do { _raw_spin_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
-#define _raw_spin_unlock_irqrestore(lock, flags) do { _raw_spin_unlock(lock);  local_irq_restore(flags); } while (0)
-#define spin_unlock_irq(lock)			do { _raw_spin_unlock(lock);  local_irq_enable(); preempt_enable();       } while (0)
-#define spin_unlock_bh(lock)			do { spin_unlock(lock); local_bh_enable(); } while (0)
-
-#define read_unlock_irqrestore(lock, flags)	do { _raw_read_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
-#define read_unlock_irq(lock)			do { _raw_read_unlock(lock);  local_irq_enable(); preempt_enable(); } while (0)
-#define read_unlock_bh(lock)			do { read_unlock(lock);  local_bh_enable();        } while (0)
-
-#define write_unlock_irqrestore(lock, flags)	do { _raw_write_unlock(lock); local_irq_restore(flags); preempt_enable(); } while (0)
-#define write_unlock_irq(lock)			do { _raw_write_unlock(lock); local_irq_enable(); preempt_enable();       } while (0)
-#define write_unlock_bh(lock)			do { write_unlock(lock); local_bh_enable();        } while (0)
-#define spin_trylock_bh(lock)			({ int __r; local_bh_disable();\
-						__r = spin_trylock(lock);      \
-						if (!__r) local_bh_enable();   \
-						__r; })
+#define spin_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); spin_lock(lock); } while (0)
 
-/* Must define these before including other files, inline functions need them */
+#define spin_lock_irq(lock) \
+	do { local_irq_disable(); spin_lock(lock); } while (0)
+
+#define spin_lock_bh(lock) \
+	do { local_bh_disable(); spin_lock(lock); } while (0)
+
+#define read_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); read_lock(lock); } while (0)
+
+#define read_lock_irq(lock) \
+	do { local_irq_disable(); read_lock(lock); } while (0)
+
+#define read_lock_bh(lock) \
+	do { local_bh_disable(); read_lock(lock); } while (0)
+
+#define write_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); write_lock(lock); } while (0)
+
+#define write_lock_irq(lock) \
+	do { local_irq_disable(); write_lock(lock); } while (0)
+
+#define write_lock_bh(lock) \
+	do { local_bh_disable(); write_lock(lock); } while (0)
+
+#define spin_unlock_irqrestore(lock, flags) do { \
+	_raw_spin_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define _raw_spin_unlock_irqrestore(lock, flags) \
+	do { _raw_spin_unlock(lock); local_irq_restore(flags); } while (0)
+
+#define spin_unlock_irq(lock) do { \
+	_raw_spin_unlock(lock);  local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define spin_unlock_bh(lock) \
+	do { spin_unlock(lock); local_bh_enable(); } while (0)
+
+#define read_unlock_irqrestore(lock, flags) do {\
+	_raw_read_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define read_unlock_irq(lock) do { \
+	_raw_read_unlock(lock); local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define read_unlock_bh(lock) do { \
+	read_unlock(lock); local_bh_enable(); \
+} while (0)
+
+#define write_unlock_irqrestore(lock, flags) do { \
+	_raw_write_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define write_unlock_irq(lock) do { \
+	_raw_write_unlock(lock); local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define write_unlock_bh(lock) \
+	do { write_unlock(lock); local_bh_enable(); } while (0)
+
+#define spin_trylock_bh(lock)	({ int __r; local_bh_disable();\
+				__r = spin_trylock(lock);      \
+				if (!__r) local_bh_enable();   \
+				__r; })
+
+/*
+ * Must define these before including other files, inline functions need them
+ */
 
 #include <linux/stringify.h>
 
@@ -63,8 +104,11 @@
 #ifdef CONFIG_SMP
 #include <asm/spinlock.h>
 
-#elif !defined(spin_lock_init) /* !SMP and spin_lock_init not previously
-                                  defined (e.g. by including asm/spinlock.h */
+/*
+ * !CONFIG_SMP and spin_lock_init not previously defined
+ * (e.g. by including include/asm/spinlock.h)
+ */
+#elif !defined(spin_lock_init)
 
 #ifndef CONFIG_PREEMPT
 # define atomic_dec_and_lock(atomic,lock) atomic_dec_and_test(atomic)
@@ -73,16 +117,9 @@
 
 /*
  * Your basic spinlocks, allowing only a single CPU anywhere
- *
- * Most gcc versions have a nasty bug with empty initializers.
  */
-#if (__GNUC__ > 2)
-  typedef struct { } spinlock_t;
+typedef struct { } spinlock_t;
 # define SPIN_LOCK_UNLOCKED (spinlock_t) { }
-#else
-  typedef struct { int gcc_is_buggy; } spinlock_t;
-# define SPIN_LOCK_UNLOCKED (spinlock_t) { 0 }
-#endif
 
 #define spin_lock_init(lock)	do { (void)(lock); } while(0)
 #define _raw_spin_lock(lock)	(void)(lock) /* Not "unused variable". */
@@ -100,16 +137,9 @@
  * can "mix" irq-safe locks - any writer needs to get a
  * irq-safe write-lock, but readers can get non-irqsafe
  * read-locks.
- *
- * Most gcc versions have a nasty bug with empty initializers.
  */
-#if (__GNUC__ > 2)
-  typedef struct { } rwlock_t;
-  #define RW_LOCK_UNLOCKED (rwlock_t) { }
-#else
-  typedef struct { int gcc_is_buggy; } rwlock_t;
-  #define RW_LOCK_UNLOCKED (rwlock_t) { 0 }
-#endif
+typedef struct { } rwlock_t;
+#define RW_LOCK_UNLOCKED (rwlock_t) { }
 
 #define rwlock_init(lock)	do { } while(0)
 #define _raw_read_lock(lock)	(void)(lock) /* Not "unused variable". */
@@ -119,8 +149,6 @@
 
 #endif /* !SMP */
 
-#ifdef CONFIG_PREEMPT
-
 #define spin_lock(lock)	\
 do { \
 	preempt_disable(); \
@@ -129,6 +157,7 @@
 
 #define spin_trylock(lock)	({preempt_disable(); _raw_spin_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
+
 #define spin_unlock(lock) \
 do { \
 	_raw_spin_unlock(lock); \
@@ -142,19 +171,6 @@
 #define write_trylock(lock)	({preempt_disable();_raw_write_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
 
-#else
-
-#define spin_lock(lock)			_raw_spin_lock(lock)
-#define spin_trylock(lock)		_raw_spin_trylock(lock)
-#define spin_unlock(lock)		_raw_spin_unlock(lock)
-
-#define read_lock(lock)			_raw_read_lock(lock)
-#define read_unlock(lock)		_raw_read_unlock(lock)
-#define write_lock(lock)		_raw_write_lock(lock)
-#define write_unlock(lock)		_raw_write_unlock(lock)
-#define write_trylock(lock)		_raw_write_trylock(lock)
-#endif
-
 /* "lock on reference count zero" */
 #ifndef ATOMIC_DEC_AND_LOCK
 #include <asm/atomic.h>

Re: [PATCH] spinlock.h cleanup

[Linus Torvalds]

On 29 Jul 2002, Robert Love wrote:
> 
> Tested on UP, SMP, and preempt.  Object code is unchanged.  Patch is
> against 2.5-bk but should apply to 2.5.29.  Please, apply.

Hmm.. Why did you remove the gcc workaround? Are all gcc's > 2.95 known to 
be ok wrt empty initializers?

		Linus

Re: [PATCH] spinlock.h cleanup

[Robert Love]

On Mon, 2002-07-29 at 17:26, Linus Torvalds wrote:

> Hmm.. Why did you remove the gcc workaround? Are all gcc's > 2.95 known to 
> be ok wrt empty initializers?

I hope so, we only check for gcc < 2.0.

If I recall correctly, the fix was for older egcs compilers.

	Robert Love

Re: [PATCH] spinlock.h cleanup

[Robert Love]

On Mon, 2002-07-29 at 17:30, Robert Love wrote:

> On Mon, 2002-07-29 at 17:26, Linus Torvalds wrote:
> 
> > Hmm.. Why did you remove the gcc workaround? Are all gcc's > 2.95 known to 
> > be ok wrt empty initializers?
> 
> If I recall correctly, the fix was for older egcs compilers.

To better answer your question, I just checked and indeed it seems all
gcc's >= 2.95 are OK.

	Robert Love

Re: [PATCH] spinlock.h cleanup

[Robert Love]

On Mon, 2002-07-29 at 17:33, Robert Love wrote:

> To better answer your question, I just checked and indeed it seems all
> gcc's >= 2.95 are OK.

I was informed by Andrew Morton that he uses egcs... normally I would
prefer to abandon an old compiler, but Andrew is an immediate exception
in my book :)

I do not know if egcs 1.1.2 has the bug or not.  For Andrew's sake, and
architectures that still recommend egcs-1.1.2, attached is a version of
the patch that keeps the compiler workaround.

We can either merge the original and see who screams or merge this
now... either is fine with me.  If the former, I will be prepared to
send of a patch to restore the workaround.

	Robert Love

diff -urN linux-2.5.29-bk/include/linux/preempt.h linux/include/linux/preempt.h
--- linux-2.5.29-bk/include/linux/preempt.h	Mon Jul 29 15:59:01 2002
+++ linux/include/linux/preempt.h	Mon Jul 29 16:34:12 2002
@@ -1,6 +1,11 @@
 #ifndef __LINUX_PREEMPT_H
 #define __LINUX_PREEMPT_H
 
+/*
+ * include/linux/preempt.h - macros for accessing and manipulating
+ * preempt_count (used for kernel preemption, interrupt count, etc.)
+ */
+
 #include <linux/config.h>
 
 #define preempt_count() (current_thread_info()->preempt_count)
@@ -37,7 +42,7 @@
 #else
 
 #define preempt_disable()		do { } while (0)
-#define preempt_enable_no_resched()	do {} while(0)
+#define preempt_enable_no_resched()	do { } while (0)
 #define preempt_enable()		do { } while (0)
 #define preempt_check_resched()		do { } while (0)
 
diff -urN linux-2.5.29-bk/include/linux/spinlock.h linux/include/linux/spinlock.h
--- linux-2.5.29-bk/include/linux/spinlock.h	Mon Jul 29 15:59:01 2002
+++ linux/include/linux/spinlock.h	Mon Jul 29 16:34:33 2002
@@ -11,39 +11,80 @@
 #include <asm/system.h>
 
 /*
- * These are the generic versions of the spinlocks and read-write
- * locks..
+ * These are the generic versions of the spinlocks and read-write locks..
  */
-#define spin_lock_irqsave(lock, flags)		do { local_irq_save(flags);       spin_lock(lock); } while (0)
-#define spin_lock_irq(lock)			do { local_irq_disable();         spin_lock(lock); } while (0)
-#define spin_lock_bh(lock)			do { local_bh_disable();          spin_lock(lock); } while (0)
-
-#define read_lock_irqsave(lock, flags)		do { local_irq_save(flags);       read_lock(lock); } while (0)
-#define read_lock_irq(lock)			do { local_irq_disable();         read_lock(lock); } while (0)
-#define read_lock_bh(lock)			do { local_bh_disable();          read_lock(lock); } while (0)
-
-#define write_lock_irqsave(lock, flags)		do { local_irq_save(flags);      write_lock(lock); } while (0)
-#define write_lock_irq(lock)			do { local_irq_disable();        write_lock(lock); } while (0)
-#define write_lock_bh(lock)			do { local_bh_disable();         write_lock(lock); } while (0)
-
-#define spin_unlock_irqrestore(lock, flags)	do { _raw_spin_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
-#define _raw_spin_unlock_irqrestore(lock, flags) do { _raw_spin_unlock(lock);  local_irq_restore(flags); } while (0)
-#define spin_unlock_irq(lock)			do { _raw_spin_unlock(lock);  local_irq_enable(); preempt_enable();       } while (0)
-#define spin_unlock_bh(lock)			do { spin_unlock(lock); local_bh_enable(); } while (0)
-
-#define read_unlock_irqrestore(lock, flags)	do { _raw_read_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
-#define read_unlock_irq(lock)			do { _raw_read_unlock(lock);  local_irq_enable(); preempt_enable(); } while (0)
-#define read_unlock_bh(lock)			do { read_unlock(lock);  local_bh_enable();        } while (0)
-
-#define write_unlock_irqrestore(lock, flags)	do { _raw_write_unlock(lock); local_irq_restore(flags); preempt_enable(); } while (0)
-#define write_unlock_irq(lock)			do { _raw_write_unlock(lock); local_irq_enable(); preempt_enable();       } while (0)
-#define write_unlock_bh(lock)			do { write_unlock(lock); local_bh_enable();        } while (0)
-#define spin_trylock_bh(lock)			({ int __r; local_bh_disable();\
-						__r = spin_trylock(lock);      \
-						if (!__r) local_bh_enable();   \
-						__r; })
+#define spin_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); spin_lock(lock); } while (0)
 
-/* Must define these before including other files, inline functions need them */
+#define spin_lock_irq(lock) \
+	do { local_irq_disable(); spin_lock(lock); } while (0)
+
+#define spin_lock_bh(lock) \
+	do { local_bh_disable(); spin_lock(lock); } while (0)
+
+#define read_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); read_lock(lock); } while (0)
+
+#define read_lock_irq(lock) \
+	do { local_irq_disable(); read_lock(lock); } while (0)
+
+#define read_lock_bh(lock) \
+	do { local_bh_disable(); read_lock(lock); } while (0)
+
+#define write_lock_irqsave(lock, flags) \
+	do { local_irq_save(flags); write_lock(lock); } while (0)
+
+#define write_lock_irq(lock) \
+	do { local_irq_disable(); write_lock(lock); } while (0)
+
+#define write_lock_bh(lock) \
+	do { local_bh_disable(); write_lock(lock); } while (0)
+
+#define spin_unlock_irqrestore(lock, flags) do { \
+	_raw_spin_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define _raw_spin_unlock_irqrestore(lock, flags) \
+	do { _raw_spin_unlock(lock); local_irq_restore(flags); } while (0)
+
+#define spin_unlock_irq(lock) do { \
+	_raw_spin_unlock(lock);  local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define spin_unlock_bh(lock) \
+	do { spin_unlock(lock); local_bh_enable(); } while (0)
+
+#define read_unlock_irqrestore(lock, flags) do {\
+	_raw_read_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define read_unlock_irq(lock) do { \
+	_raw_read_unlock(lock); local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define read_unlock_bh(lock) do { \
+	read_unlock(lock); local_bh_enable(); \
+} while (0)
+
+#define write_unlock_irqrestore(lock, flags) do { \
+	_raw_write_unlock(lock); local_irq_restore(flags); preempt_enable(); \
+} while (0)
+
+#define write_unlock_irq(lock) do { \
+	_raw_write_unlock(lock); local_irq_enable(); preempt_enable(); \
+} while (0)
+
+#define write_unlock_bh(lock) \
+	do { write_unlock(lock); local_bh_enable(); } while (0)
+
+#define spin_trylock_bh(lock)	({ int __r; local_bh_disable();\
+				__r = spin_trylock(lock);      \
+				if (!__r) local_bh_enable();   \
+				__r; })
+
+/*
+ * Must define these before including other files, inline functions need them
+ */
 
 #include <linux/stringify.h>
 
@@ -63,8 +104,11 @@
 #ifdef CONFIG_SMP
 #include <asm/spinlock.h>
 
-#elif !defined(spin_lock_init) /* !SMP and spin_lock_init not previously
-                                  defined (e.g. by including asm/spinlock.h */
+/*
+ * !CONFIG_SMP and spin_lock_init not previously defined
+ * (e.g. by including include/asm/spinlock.h)
+ */
+#elif !defined(spin_lock_init)
 
 #ifndef CONFIG_PREEMPT
 # define atomic_dec_and_lock(atomic,lock) atomic_dec_and_test(atomic)
@@ -119,8 +149,6 @@
 
 #endif /* !SMP */
 
-#ifdef CONFIG_PREEMPT
-
 #define spin_lock(lock)	\
 do { \
 	preempt_disable(); \
@@ -129,6 +157,7 @@
 
 #define spin_trylock(lock)	({preempt_disable(); _raw_spin_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
+
 #define spin_unlock(lock) \
 do { \
 	_raw_spin_unlock(lock); \
@@ -142,19 +171,6 @@
 #define write_trylock(lock)	({preempt_disable();_raw_write_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
 
-#else
-
-#define spin_lock(lock)			_raw_spin_lock(lock)
-#define spin_trylock(lock)		_raw_spin_trylock(lock)
-#define spin_unlock(lock)		_raw_spin_unlock(lock)
-
-#define read_lock(lock)			_raw_read_lock(lock)
-#define read_unlock(lock)		_raw_read_unlock(lock)
-#define write_lock(lock)		_raw_write_lock(lock)
-#define write_unlock(lock)		_raw_write_unlock(lock)
-#define write_trylock(lock)		_raw_write_trylock(lock)
-#endif
-
 /* "lock on reference count zero" */
 #ifndef ATOMIC_DEC_AND_LOCK
 #include <asm/atomic.h>

Re: [PATCH] spinlock.h cleanup

[David S. Miller]

   From: Robert Love <rml@tech9.net>
   Date: 29 Jul 2002 17:33:39 -0700

   On Mon, 2002-07-29 at 17:30, Robert Love wrote:
   
   > On Mon, 2002-07-29 at 17:26, Linus Torvalds wrote:
   > 
   > > Hmm.. Why did you remove the gcc workaround? Are all gcc's > 2.95 known to 
   > > be ok wrt empty initializers?
   > 
   > If I recall correctly, the fix was for older egcs compilers.
   
   To better answer your question, I just checked and indeed it seems all
   gcc's >= 2.95 are OK.

Some platforms (sparc64) are still using things like egcs-2.92.x
vintage compilers as their main supported kernel build compiler.

init/main.c allows 2.91 or greater to pass so that should be the rule
enforced kernel wide.

I don't remember when the empty initializer thing was fixed.

Re: [PATCH] spinlock.h cleanup

[Linus Torvalds]

On 29 Jul 2002, Robert Love wrote:
>
> I hope so, we only check for gcc < 2.0.

Check that again. The old check was for "(__GNUC__ > 2)", ie it would only
trigger for gcc-3 and up.

> If I recall correctly, the fix was for older egcs compilers.

I've got this memory of fairly recent gcc's messing up on sparc, for
example.

		Linus

Re: [PATCH] spinlock.h cleanup

[Arnaldo Carvalho de Melo]

Em Mon, Jul 29, 2002 at 06:46:40PM -0700, Linus Torvalds escreveu:
> On 29 Jul 2002, Robert Love wrote:
> > If I recall correctly, the fix was for older egcs compilers.
> 
> I've got this memory of fairly recent gcc's messing up on sparc, for
> example.

And IIRC the current supported compiled for Sparc32 is an old hacked egcs.

- Arnaldo

Re: [PATCH] spinlock.h cleanup

[Robert Love]

On Mon, 2002-07-29 at 18:46, Linus Torvalds wrote:

> Check that again. The old check was for "(__GNUC__ > 2)", ie it would only
> trigger for gcc-3 and up.

Err, right.

> > If I recall correctly, the fix was for older egcs compilers.
> 
> I've got this memory of fairly recent gcc's messing up on sparc, for
> example.

The problem is indeed fixed in 2.95... the problem is not with those
compilers on sparc but that sparc64 uses an old compiler (DaveM
recommends 2.91.66 for sparc64).

Consequently, I sent you a rediff of the patch - this time without
removing the compiler workaround.  It should be acceptable.

	Robert Love