[BRLOCKS]: Delete atomic version, is buggy and deadlock prone.

ChangeSet 1.1576.1.3, 2005/02/16 12:57:35-08:00, [email protected]

	[BRLOCKS]: Delete atomic version, is buggy and deadlock prone.
	
	There were two versions of the big-reader lock implementation.
	
	1) One using per-cpu reader locks, and a singular write lock.
	   Predominantly enabled on x86 and it's brothers.
	
	2) One using non-atomic per-cpu counter, and a single write lock.
	   This is what all other platforms were using.
	
	#1 is unfortunately buggy.  brlocks were meant to provide a
	high performance implementation of rwlock_t locks when it
	is known that the lock is taken %99 of the time by readers
	and that writers are thus rare.
	
	The #2 version of the implementation does this faithfully.
	In particular, when writers try to get in, they back off
	on grabbing the writer lock if readers are present, then
	try again.  #1 does not work this way, it works by taking
	each of the per-cpu reader locks, then the singular write
	lock.
	
	One of several consequences of #1's behavior is that if
	there is a code path where a brlock is taken as a reader
	recursively, a writer can deadlock a reader between the
	two recursive read lock acquires and thus deadlock the entire
	system.
	
	The main core kernel usage of brlocks, networking, does
	exactly this, it takes the NETPROTO brlock recursively as
	a writer.  Specifically, this occurs in the netfilter
	nf_hook() path.
	
	As a result, we need to remove the atomic based brlock
	implementation (#1 above) and use only the non-atomic
	variant which does faithfully implement rwlock_t compatible
	semantics.
	
	Thanks to Alexey Kuznetsov for discovering this problem, and
	to Ingo Molnar for valuable feedback.
	
	Signed-off-by: David S. Miller <[email protected]>



 include/linux/brlock.h |   46 ----------------------------------------------
 lib/brlock.c           |   25 -------------------------
 2 files changed, 71 deletions(-)


diff -Nru a/include/linux/brlock.h b/include/linux/brlock.h
--- a/include/linux/brlock.h	2005-02-19 07:07:54 -08:00
+++ b/include/linux/brlock.h	2005-02-19 07:07:54 -08:00
@@ -18,16 +18,6 @@
  * Registry idea and naming [ crutial! :-) ] by:
  *
  *                 David S. Miller <[email protected]>
- *
- * David has an implementation that doesn't use atomic operations in
- * the read branch via memory ordering tricks - i guess we need to
- * split this up into a per-arch thing? The atomicity issue is a
- * secondary item in profiles, at least on x86 platforms.
- *
- * The atomic op version overhead is indeed a big deal on
- * load-locked/store-conditional cpus (ALPHA/MIPS/PPC) and
- * compare-and-swap cpus (Sparc64).  So we control which
- * implementation to use with a __BRLOCK_USE_ATOMICS define. -DaveM
  */
 
 /* Register bigreader lock indices here. */
@@ -45,17 +35,7 @@
 #include <linux/cache.h>
 #include <linux/spinlock.h>
 
-#if defined(__i386__) || defined(__ia64__) || defined(__x86_64__)
-#define __BRLOCK_USE_ATOMICS
-#else
-#undef __BRLOCK_USE_ATOMICS
-#endif
-
-#ifdef __BRLOCK_USE_ATOMICS
-typedef rwlock_t	brlock_read_lock_t;
-#else
 typedef unsigned int	brlock_read_lock_t;
-#endif
 
 /*
  * align last allocated index to the next cacheline:
@@ -65,39 +45,14 @@
 
 extern brlock_read_lock_t __brlock_array[NR_CPUS][__BR_IDX_MAX];
 
-#ifndef __BRLOCK_USE_ATOMICS
 struct br_wrlock {
 	spinlock_t lock;
 } __attribute__ ((__aligned__(SMP_CACHE_BYTES)));
 
 extern struct br_wrlock __br_write_locks[__BR_IDX_MAX];
-#endif
 
 extern void __br_lock_usage_bug (void);
 
-#ifdef __BRLOCK_USE_ATOMICS
-
-static inline void br_read_lock (enum brlock_indices idx)
-{
-	/*
-	 * This causes a link-time bug message if an
-	 * invalid index is used:
-	 */
-	if (idx >= __BR_END)
-		__br_lock_usage_bug();
-
-	read_lock(&__brlock_array[smp_processor_id()][idx]);
-}
-
-static inline void br_read_unlock (enum brlock_indices idx)
-{
-	if (idx >= __BR_END)
-		__br_lock_usage_bug();
-
-	read_unlock(&__brlock_array[smp_processor_id()][idx]);
-}
-
-#else /* ! __BRLOCK_USE_ATOMICS */
 static inline void br_read_lock (enum brlock_indices idx)
 {
 	unsigned int *ctr;
@@ -149,7 +104,6 @@
 	wmb();
 	(*ctr)--;
 }
-#endif /* __BRLOCK_USE_ATOMICS */
 
 /* write path not inlined - it's rare and larger */
 
diff -Nru a/lib/brlock.c b/lib/brlock.c
--- a/lib/brlock.c	2005-02-19 07:07:54 -08:00
+++ b/lib/brlock.c	2005-02-19 07:07:54 -08:00
@@ -15,29 +15,6 @@
 #include <linux/sched.h>
 #include <linux/brlock.h>
 
-#ifdef __BRLOCK_USE_ATOMICS
-
-brlock_read_lock_t __brlock_array[NR_CPUS][__BR_IDX_MAX] =
-   { [0 ... NR_CPUS-1] = { [0 ... __BR_IDX_MAX-1] = RW_LOCK_UNLOCKED } };
-
-void fastcall __br_write_lock (enum brlock_indices idx)
-{
-	int i;
-
-	for (i = 0; i < smp_num_cpus; i++)
-		write_lock(&__brlock_array[cpu_logical_map(i)][idx]);
-}
-
-void fastcall __br_write_unlock (enum brlock_indices idx)
-{
-	int i;
-
-	for (i = 0; i < smp_num_cpus; i++)
-		write_unlock(&__brlock_array[cpu_logical_map(i)][idx]);
-}
-
-#else /* ! __BRLOCK_USE_ATOMICS */
-
 brlock_read_lock_t __brlock_array[NR_CPUS][__BR_IDX_MAX] =
    { [0 ... NR_CPUS-1] = { [0 ... __BR_IDX_MAX-1] = 0 } };
 
@@ -63,7 +40,5 @@
 {
 	spin_unlock(&__br_write_locks[idx].lock);
 }
-
-#endif /* __BRLOCK_USE_ATOMICS */
 
 #endif /* CONFIG_SMP */