generic__raw_read_trylock Considered Harmful

generic__raw_read_trylock Considered Harmful

[Matthew Wilcox]

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.

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 31473db..6ed1e54 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -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):
  */
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index bfd6ad9..c9f1541 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -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();

generic__raw_read_trylock Considered Harmful

[Matthew Wilcox]

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.

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 31473db..6ed1e54 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -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):
  */
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index bfd6ad9..c9f1541 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -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();

Re: generic__raw_read_trylock Considered Harmful

[Russell King]

On Thu, Aug 31, 2006 at 07:42:55AM -0600, Matthew Wilcox wrote:
> 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:

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

Re: generic__raw_read_trylock Considered Harmful

[Russell King]

On Thu, Aug 31, 2006 at 07:42:55AM -0600, Matthew Wilcox wrote:
> 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:

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

Re: generic__raw_read_trylock Considered Harmful

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 31 Aug 2006 07:42:55 -0600

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

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...

Re: generic__raw_read_trylock Considered Harmful

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 31 Aug 2006 07:42:55 -0600

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

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...

Re: generic__raw_read_trylock Considered Harmful

[Matthew Wilcox]

On Tue, Sep 19, 2006 at 03:53:17PM -0700, David Miller wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> > NOTE to architecture maintainers: generic_raw_read_trylock() is
> > completely unfit for use and will cause lockups if used in interrupt
> > context.
> 
> 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;h=513b38677e9bb51d32f4cb401105bf828dfcd685
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=93d8d8dff74923dc9eb511c96ee3ccb918666049

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

Re: generic__raw_read_trylock Considered Harmful

[Matthew Wilcox]

On Tue, Sep 19, 2006 at 03:53:17PM -0700, David Miller wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> > NOTE to architecture maintainers: generic_raw_read_trylock() is
> > completely unfit for use and will cause lockups if used in interrupt
> > context.
> 
> 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 ;-)