generic__raw_read_trylock Considered Harmful

Back in January 2005, Ingo wrote:

NOTE to architecture maintainers: generic_raw_read_trylock() is a crude version that should be replaced with the proper arch-optimized version ASAP.

He didn't really phrase that strongly enough. It should have read:

NOTE to architecture maintainers: generic_raw_read_trylock() is completely unfit for use and will cause lockups if used in interrupt context.

I propose we delete this from the tree, turning a rather nasty and hard to track down runtime failure into a simple to fix buildtime failure on the following architectures:

include/asm-arm/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock) include/asm-m32r/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock) include/asm-mips/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock) include/asm-parisc/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock) include/asm-sh/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock) include/asm-sparc/spinlock.h:#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)

MIPS is already fixed out of tree. ARM, M32R, SH and SPARC need to be fixed. We're redoing the PA-RISC implementation right now, which is how I noticed this, er, minor problem.

index 31473db..6ed1e54 100644
@@ -77,8 +77,6 @@ #define __lockfunc fastcall __attribute_
  */
 #include <linux/spinlock_types.h>
 
-extern int __lockfunc generic__raw_read_trylock(raw_rwlock_t *lock);
-
 /*
  * Pull the __raw*() functions/declarations (UP-nondebug doesnt need them):
  */

index bfd6ad9..c9f1541 100644
@@ -16,17 +16,6 @@ #include <linux/interrupt.h>
 #include <linux/debug_locks.h>
 #include <linux/module.h>
 
-/*
- * Generic declaration of the raw read_trylock() function,
- * architectures are supposed to optimize this:
- */
-int __lockfunc generic__raw_read_trylock(raw_rwlock_t *lock)
-{
-	__raw_read_lock(lock);
-	return 1;
-}
-EXPORT_SYMBOL(generic__raw_read_trylock);
-
 int __lockfunc _spin_trylock(spinlock_t *lock)
 {
 	preempt_disable();

Thanks, I've just committed a fix for ARM.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

From: Matthew Wilcox <[email protected]>
Date: Thu, 31 Aug 2006 07:42:55 -0600

I was about to go an fix SPARC but then looked in the tree for actual users, and I see one, and only one, in the lockdep code which tries to grab the tasklist_lock.

That's it (well.. there's one strange IA64 specific case in the MCA code too).

You say you were fixing up PARISC and actually hit this problem, was it exactly this lockdep case? I really doubt it...

That's not quite what I said; I was chasing some problems on parisc where we were getting read_lock deadlocks, and spotted this problem. It's not beyond the bounds of possibility that we'll have a real user for this function in the future.

The real bug was almost certainly elsewhere:

http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;hQ3b38677e9bb51d32f4cb401105bf828dfcd685
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h“d8d8dff74923dc9eb511c96ee3ccb918666049

Essentially, a fix for one rather subtle bug introduced a somewhat less subtle one (which nevertheless went unfound for, er, 11 months ;-)