Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch

Linus,

The change below is causing major problems for me on a dual K7 with CONFIG_PREEMPT enabled (cset -x and rebuilding makes the machine usable again).

This change was merged a couple of days ago so I'm surprised nobody else has reported this. I tried to find where this patch came from but I don't see it on lkml only the bk history.

Note, even with this removed I'm still seeing a few (many actually) "BUG: using smp_processor_id() in preemptible [00000001] code: xxx" messages which I've not seen before --- that might be unrelated but I do see *many* such messages so I'm sure I would have noticed this before or it would have broken something earlier.

Is this specific to the k7 or do other people also see this?

Thanks,

--cw

---

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/01/15 09:40:45-08:00 [email protected]
# [PATCH] Don't busy-lock-loop in preemptable spinlocks
#

#   Paul Mackerras points out that doing the _raw_spin_trylock each time
#   through the loop will generate tons of unnecessary bus traffic.
#   Instead, after we fail to get the lock we should poll it with simple
#   loads until we see that it is clear and then retry the atomic op. 
#   Assuming a reasonable cache design, the loads won't generate any bus
#   traffic until another cpu writes to the cacheline containing the lock.

#
# Agreed.
#
# Signed-off-by: Ingo Molnar <[email protected]>
# Signed-off-by: Linus Torvalds <[email protected]>
#

# kernel/spinlock.c
#   2005/01/14 16:00:00-08:00 [email protected] +8 -6
#   Don't busy-lock-loop in preemptable spinlocks
# 
diff -Nru a/kernel/spinlock.c b/kernel/spinlock.c
--- a/kernel/spinlock.c	2005-01-16 21:43:15 -08:00
+++ b/kernel/spinlock.c	2005-01-16 21:43:15 -08:00
@@ -173,7 +173,7 @@
  * (We do this in a function because inlining it would be excessive.)
  */

-#define BUILD_LOCK_OPS(op, locktype) \
+#define BUILD_LOCK_OPS(op, locktype, is_locked_fn) \
void __lockfunc _##op##_lock(locktype *lock) \
{ \
preempt_disable(); \
@@ -183,7 +183,8 @@
preempt_enable(); \
if (!(lock)->break_lock) \
(lock)->break_lock = 1; \
- cpu_relax(); \
+ while (is_locked_fn(lock) && (lock)->break_lock) \
+ cpu_relax(); \
preempt_disable(); \
} \
} \
@@ -204,7 +205,8 @@
preempt_enable(); \
if (!(lock)->break_lock) \
(lock)->break_lock = 1; \
- cpu_relax(); \
+ while (is_locked_fn(lock) && (lock)->break_lock) \
+ cpu_relax(); \
preempt_disable(); \
} \
return flags; \
@@ -244,9 +246,9 @@

  *         _[spin|read|write]_lock_irqsave()
  *         _[spin|read|write]_lock_bh()

*/
-BUILD_LOCK_OPS(spin, spinlock_t); -BUILD_LOCK_OPS(read, rwlock_t); -BUILD_LOCK_OPS(write, rwlock_t); +BUILD_LOCK_OPS(spin, spinlock_t, spin_is_locked); +BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_locked); +BUILD_LOCK_OPS(write, rwlock_t, spin_is_locked);

#endif /* CONFIG_PREEMPT */

Paul noticed that too so I came up with the patch below.

If it makes sense I can do the other architectures (I'm not sure == 0 is correct everywhere). This is pretty much what I'm using now without problems (it's either correct or it's almost correct and the rwlock_is_write_locked hasn't thus far stuffed anything this boot).

---
Fix how we check for read and write rwlock_t locks.

===== include/asm-i386/spinlock.h 1.16 vs edited =====

@@ -187,6 +187,7 @@ typedef struct {
 #define rwlock_init(x)	do { *(x) = RW_LOCK_UNLOCKED; } while(0)
 
 #define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+#define rwlock_is_write_locked(x) ((x)->lock == 0)
 
 /*
  * On x86, we implement read-write locks as a 32-bit counter

===== kernel/spinlock.c 1.4 vs edited =====

@@ -247,8 +247,8 @@ EXPORT_SYMBOL(_##op##_lock_bh)
  *         _[spin|read|write]_lock_bh()
  */
 BUILD_LOCK_OPS(spin, spinlock_t, spin_is_locked);
-BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_locked);
-BUILD_LOCK_OPS(write, rwlock_t, spin_is_locked);
+BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_write_locked);
+BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
 
 #endif /* CONFIG_PREEMPT */
 

AFAICS on i386 the lock word, although it goes to 0 when write-locked, can then go negative temporarily when another cpu tries to get a read or write lock. So I think this should be

((signed int)(x)->lock <= 0)

(or the equivalent using atomic_read).

Paul.

I think you're right, objdump -d shows[1] _write_lock looks something like:

      lock subl $0x1000000,(%ebx)
      sete   %al
      test   %al,%al
      jne    out;
      lock addl $0x1000000,(%ebx)

out:

so I guess it 2+ CPUs grab a write-lock at once it would indeed be less than zero (the initial value is RW_LOCK_BIAS which is 0x1000000 in this case).

on x86 aligned-reads will be always be atomic AFAIK?

[1] Yes, I'm stupid, trying to grok the twisty-turny-maze of headers and what not made my head hurt and objdump -d seemed easier.

this is not enough - the proper solution should be the patch below, which i sent earlier today as a reply to Paul Mackerras' comments.

Ingo

--
the first fix is that there was no compiler warning on x86 because it uses macros - i fixed this by changing the spinlock field to be '->slock'. (we could also use inline functions to get type protection, i chose this solution because it was the easiest to do.)

the second fix is to split rwlock_is_locked() into two functions:

+/**
+ * read_is_locked - would read_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define read_is_locked(x) (atomic_read((atomic_t *)&(x)->lock) <= 0) +
+/**
+ * write_is_locked - would write_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define write_is_locked(x) ((x)->lock != RW_LOCK_BIAS)

this canonical naming of them also enabled the elimination of the newly added 'is_locked_fn' argument to the BUILD_LOCK_OPS macro.

the third change was to change the other user of rwlock_is_locked(), and to put a migration helper there: architectures that dont have read/write_is_locked defined yet will get a #warning message but the build will succeed. (except if PREEMPT is enabled - there we really need.)

compile and boot-tested on x86, on SMP and UP, PREEMPT and !PREEMPT. Non-x86 architectures should work fine, except PREEMPT+SMP builds which will need the read_is_locked()/write_is_locked() definitions. !PREEMPT+SMP builds will work fine and will produce a #warning.

Ingo

@@ -173,7 +173,7 @@ EXPORT_SYMBOL(_write_lock);
  * (We do this in a function because inlining it would be excessive.)
  */
 
-#define BUILD_LOCK_OPS(op, locktype, is_locked_fn)			\
+#define BUILD_LOCK_OPS(op, locktype)					\
 void __lockfunc _##op##_lock(locktype *lock)				\
 {									\
 	preempt_disable();						\

@@ -183,7 +183,7 @@ void __lockfunc _##op##_lock(locktype *l
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while (is_locked_fn(lock) && (lock)->break_lock)	\
+		while (op##_is_locked(lock) && (lock)->break_lock)	\
 			cpu_relax();					\
 		preempt_disable();					\
 	}								\

@@ -205,7 +205,7 @@ unsigned long __lockfunc _##op##_lock_ir
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while (is_locked_fn(lock) && (lock)->break_lock)	\
+		while (op##_is_locked(lock) && (lock)->break_lock)	\
 			cpu_relax();					\
 		preempt_disable();					\
 	}								\

@@ -246,9 +246,9 @@ EXPORT_SYMBOL(_##op##_lock_bh)
  *         _[spin|read|write]_lock_irqsave()
  *         _[spin|read|write]_lock_bh()
  */
-BUILD_LOCK_OPS(spin, spinlock_t, spin_is_locked);
-BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_locked);
-BUILD_LOCK_OPS(write, rwlock_t, spin_is_locked);
+BUILD_LOCK_OPS(spin, spinlock_t);
+BUILD_LOCK_OPS(read, rwlock_t);
+BUILD_LOCK_OPS(write, rwlock_t);
 
 #endif /* CONFIG_PREEMPT */
 

@@ -15,7 +15,7 @@ asmlinkage int printk(const char * fmt, 
  */
 
 typedef struct {
-	volatile unsigned int lock;
+	volatile unsigned int slock;
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned magic;
 #endif

@@ -43,7 +43,7 @@ typedef struct {
  * We make no fairness assumptions. They have a cost.
  */
 
-#define spin_is_locked(x)	(*(volatile signed char *)(&(x)->lock) <= 0)
+#define spin_is_locked(x)	(*(volatile signed char *)(&(x)->slock) <= 0)
 #define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
 
 #define spin_lock_string \

@@ -83,7 +83,7 @@ typedef struct {
 
 #define spin_unlock_string \
 	"movb $1,%0" \
-		:"=m" (lock->lock) : : "memory"
+		:"=m" (lock->slock) : : "memory"
 
 
 static inline void _raw_spin_unlock(spinlock_t *lock)

@@ -101,7 +101,7 @@ static inline void _raw_spin_unlock(spin
 
 #define spin_unlock_string \
 	"xchgb %b0, %1" \
-		:"=q" (oldval), "=m" (lock->lock) \
+		:"=q" (oldval), "=m" (lock->slock) \
 		:"0" (oldval) : "memory"
 
 static inline void _raw_spin_unlock(spinlock_t *lock)

@@ -123,7 +123,7 @@ static inline int _raw_spin_trylock(spin
 	char oldval;
 	__asm__ __volatile__(
 		"xchgb %b0,%1"
-		:"=q" (oldval), "=m" (lock->lock)
+		:"=q" (oldval), "=m" (lock->slock)
 		:"0" (0) : "memory");
 	return oldval > 0;
 }

@@ -138,7 +138,7 @@ static inline void _raw_spin_lock(spinlo
 #endif
 	__asm__ __volatile__(
 		spin_lock_string
-		:"=m" (lock->lock) : : "memory");
+		:"=m" (lock->slock) : : "memory");
 }
 
 static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)

@@ -151,7 +151,7 @@ static inline void _raw_spin_lock_flags 
 #endif
 	__asm__ __volatile__(
 		spin_lock_string_flags
-		:"=m" (lock->lock) : "r" (flags) : "memory");
+		:"=m" (lock->slock) : "r" (flags) : "memory");
 }
 
 /*

@@ -186,7 +186,17 @@ typedef struct {
 
 #define rwlock_init(x)	do { *(x) = RW_LOCK_UNLOCKED; } while(0)
 
-#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+/**
+ * read_is_locked - would read_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define read_is_locked(x) (atomic_read((atomic_t *)&(x)->lock) <= 0)
+
+/**
+ * write_is_locked - would write_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define write_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
 
 /*
  * On x86, we implement read-write locks as a 32-bit counter

@@ -861,8 +861,12 @@ task_t fastcall *next_thread(const task_
 #ifdef CONFIG_SMP
 	if (!p->sighand)
 		BUG();
+#ifndef write_is_locked
+# warning please implement read_is_locked()/write_is_locked()!
+# define write_is_locked rwlock_is_locked
+#endif
 	if (!spin_is_locked(&p->sighand->siglock) &&
-				!rwlock_is_locked(&tasklist_lock))
+				!write_is_locked(&tasklist_lock))
 		BUG();
 #endif
 	return pid_task(p->pids[PIDTYPE_TGID].pid_list.next, PIDTYPE_TGID);

Hi Ingo

This may fix some archs however ia64 is still broken, with:

kernel/built-in.o(.spinlock.text+0x8b2): In function `sched_init_smp':
kernel/sched.c:650: undefined reference to read_is_locked' kernel/built-in.o(.spinlock.text+0xa52): In function sched_init':
kernel/sched.c:687: undefined reference to read_is_locked' kernel/built-in.o(.spinlock.text+0xcb2): In function schedule':
include/asm/bitops.h:279: undefined reference to write_is_locked' kernel/built-in.o(.spinlock.text+0xe72): In function schedule':
include/linux/sched.h:1122: undefined reference to `write_is_locked'
make: *** [.tmp_vmlinux1] Error 1

include/asm-ia64/spinflock.h needs to define:
read_is_locked(x)
write_is_locked(x)

someone who knows the locking code will need to fill in the blanks.

Darren

--------------------------------------------------
Darren Williams <dsw AT gelato.unsw.edu.au>
Gelato@UNSW <www.gelato.unsw.edu.au>
--------------------------------------------------

On top of Ingo's patch I attempt a solution that failed:
include/asm-ia64/spinlock.h: 1.23 1.24 dsw 05/01/18 10:22:35 (modified, needs delta)

@@ -126,7 +126,8 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
-#define rwlock_is_locked(x) (*(volatile int *) (x) != 0)
+#define read_is_locked(x) (*(volatile int *) (x) > 0)
+#define write_is_locked(x) (*(volatile int *) (x) < 0)

#define _raw_read_lock(rw) \
do {

However this breaks on the simulator with:

Freeing unused kernel memory: 192kB freed
INIT: version 2.78 booting
kernel BUG at kernel/exit.c:870

--------------------------------------------------
Darren Williams <dsw AT gelato.unsw.edu.au>
Gelato@UNSW <www.gelato.unsw.edu.au>
--------------------------------------------------

Here's a patch that adds the missing read_is_locked() and write_is_locked() macros for IA64. When combined with Ingo's patch, I can boot an SMP kernel with CONFIG_PREEMPT on.

However, I feel these macros are misnamed: read_is_locked() returns true if the lock is held for writing; write_is_locked() returns true if the lock is held for reading or writing.

Index: linux-2.6-bklock/include/asm-ia64/spinlock.h
=================================--- linux-2.6-bklock.orig/include/asm-ia64/spinlock.h 2005-01-18 13:46:08.138077857 +1100
+++ linux-2.6-bklock/include/asm-ia64/spinlock.h 2005-01-19 08:58:59.303821753 +1100
@@ -126,8 +126,20 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+
#define rwlock_is_locked(x) (*(volatile int *) (x) != 0)

+/* read_is_locked -- - would read_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define read_is_locked(x) (*(volatile int *) (x) < 0)
+
+/**
+ * write_is_locked - would write_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define write_is_locked(x) (*(volatile int *) (x) != 0)
+
#define _raw_read_lock(rw) \
do { \
rwlock_t *__read_lock_ptr = (rw); \

Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

well, 'read_is_locked()' means: "will a read_lock() succeed" [assuming no races]. Should name it read_trylock_test()/write_trylock_test() perhaps?

Ingo

Ingo> well, 'read_is_locked()' means: "will a read_lock() succeed"

Fail, surely?

Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

yeah ... and with that i proved beyond doubt that the naming is indeed unintuitive :-)

Ingo

Yes. Intuitively read_is_locked() is true when someone has done a read_lock and write_is_locked() is true when someone has done a write lock.

I suggest read_poll(), write_poll(), spin_poll(), which are like {read,write,spin}_trylock but don't do the atomic op to get the lock, that is, they don't change the lock value but return true if the trylock would succeed, assuming no other cpu takes the lock in the meantime.

Regards,
Paul.

I'm not personally convinced *_poll is any clearer really, I would if this is vague prefer longer more obvious names but that's just me.

Because spin_is_locked is used in quite a few places I would leave that one alone for now --- I'm not saying we can't change this name, but it should be a separate issue IMO.

Because rwlock_is_locked isn't used in many places changing that isn't a big deal.

As a compromise I have the following patch in my quilt tree based upon what a few people have said in this thread already. This is again the "-CURRENT bk" tree as of a few minutes ago and seems to be working as expected.

* i386: rename spinlock_t -> lock to slock to catch possible macro abuse problems

* i386, ia64: rename rwlock_is_locked to rwlock_write_locked as this is IMO a better name

* i386, ia64: add rwlock_read_locked (if people are OK with these, I can do the other architectures)

* generic: fix kernel/exit.c to use rwlock_write_locked

* generic: fix kernel/spinlock.c

Comments?

---

 include/asm-i386/spinlock.h |   26 ++++++++++++++++++--------
 include/asm-ia64/spinlock.h |   12 +++++++++++-
 kernel/exit.c               |    2 +-
 kernel/spinlock.c           |    4 ++--
 4 files changed, 32 insertions(+), 12 deletions(-)

=== include/asm-i386/spinlock.h 1.16 vs edited ==Index: cw-current/include/asm-i386/spinlock.h
=================================--- cw-current.orig/include/asm-i386/spinlock.h 2005-01-19 17:37:27.497810394 -0800
+++ cw-current/include/asm-i386/spinlock.h 2005-01-19 17:37:30.044914512 -0800
@@ -15,7 +15,7 @@
*/

typedef struct {
- volatile unsigned int lock;
+ volatile unsigned int slock;
#ifdef CONFIG_DEBUG_SPINLOCK
unsigned magic;
#endif
@@ -43,7 +43,7 @@
* We make no fairness assumptions. They have a cost. */

-#define spin_is_locked(x) (*(volatile signed char *)(&(x)->lock) <= 0)
+#define spin_is_locked(x) (*(volatile signed char *)(&(x)->slock) <= 0)
#define spin_unlock_wait(x) do { barrier(); } while(spin_is_locked(x))

#define spin_lock_string \
@@ -83,7 +83,7 @@

#define spin_unlock_string \
"movb $1,%0" \
- :"=m" (lock->lock) : : "memory"
+ :"=m" (lock->slock) : : "memory"

static inline void _raw_spin_unlock(spinlock_t *lock)
@@ -101,7 +101,7 @@

#define spin_unlock_string \
"xchgb %b0, %1" \
- :"=q" (oldval), "=m" (lock->lock) \
+ :"=q" (oldval), "=m" (lock->slock) \
:"0" (oldval) : "memory"

static inline void _raw_spin_unlock(spinlock_t *lock)
@@ -123,7 +123,7 @@
char oldval;
__asm__ __volatile__(
"xchgb %b0,%1"
- :"=q" (oldval), "=m" (lock->lock)
+ :"=q" (oldval), "=m" (lock->slock)
:"0" (0) : "memory");
return oldval > 0;
}
@@ -138,7 +138,7 @@
#endif
__asm__ __volatile__(
spin_lock_string
- :"=m" (lock->lock) : : "memory");
+ :"=m" (lock->slock) : : "memory");
}

static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)
@@ -151,7 +151,7 @@
#endif
__asm__ __volatile__(
spin_lock_string_flags
- :"=m" (lock->lock) : "r" (flags) : "memory");
+ :"=m" (lock->slock) : "r" (flags) : "memory");
}

/*
@@ -186,7 +186,17 @@

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)

-#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+/**
+ * rwlock_read_locked - would read_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define rwlock_read_locked(x) (atomic_read((atomic_t *)&(x)->lock) <= 0) +
+/**
+ * rwlock_write_locked - would write_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define rwlock_write_locked(x) ((x)->lock != RW_LOCK_BIAS)

/*
* On x86, we implement read-write locks as a 32-bit counter
Index: cw-current/include/asm-ia64/spinlock.h
=================================--- cw-current.orig/include/asm-ia64/spinlock.h 2005-01-19 17:37:27.498810435 -0800
+++ cw-current/include/asm-ia64/spinlock.h 2005-01-19 17:37:30.044914512 -0800
@@ -126,7 +126,17 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
-#define rwlock_is_locked(x) (*(volatile int *) (x) != 0)
+
+/* rwlock_read_locked - would read_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define rwlock_read_locked(x) (*(volatile int *) (x) < 0)
+
+/**
+ * rwlock_write_locked - would write_trylock() fail?
+ * @lock: the rwlock in question.
+ */
+#define rwlock_write_locked(x) (*(volatile int *) (x) != 0)

#define _raw_read_lock(rw) \
do { \
Index: cw-current/kernel/exit.c
=================================--- cw-current.orig/kernel/exit.c 2005-01-19 17:37:27.498810435 -0800
+++ cw-current/kernel/exit.c 2005-01-19 18:14:21.601934388 -0800
@@ -862,7 +862,7 @@
if (!p->sighand)
BUG();
if (!spin_is_locked(&p->sighand->siglock) &&
- !rwlock_is_locked(&tasklist_lock))
+ !rwlock_write_locked(&tasklist_lock))
BUG();
#endif
return pid_task(p->pids[PIDTYPE_TGID].pid_list.next, PIDTYPE_TGID);
Index: cw-current/kernel/spinlock.c
=================================--- cw-current.orig/kernel/spinlock.c 2005-01-19 17:37:27.498810435 -0800
+++ cw-current/kernel/spinlock.c 2005-01-19 17:37:30.048914675 -0800
@@ -247,8 +247,8 @@
* _[spin|read|write]_lock_bh()
*/
BUILD_LOCK_OPS(spin, spinlock_t, spin_is_locked);
-BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_locked); -BUILD_LOCK_OPS(write, rwlock_t, spin_is_locked); +BUILD_LOCK_OPS(read, rwlock_t, rwlock_read_locked); +BUILD_LOCK_OPS(write, rwlock_t, rwlock_write_locked);

#endif /* CONFIG_PREEMPT */

Given the general confusion and the difficulty of defining and understanding the semantics of these predicates. And given that the foo_is_locked() predicates have a history of being used to implement ghastly kludges, how about we simply nuke this statement:

and be done with the whole thing?

I mean, do we really want these things in the kernel anyway? We've never needed them before.

If we reeeealy need the debug check, just do

	BUG_ON(read_trylock(...))

I'm all for killing that. I'll happily send a patch once the dust settles.

It still isn't enough to rid of the rwlock_read_locked and rwlock_write_locked usage in kernel/spinlock.c as those are needed for the cpu_relax() calls so we have to decide on suitable names still...

Oh crap, you're right. There's not much we can do about that.

I have a do-seven-things-at-once patch from Ingo here which touches all this stuff so cannot really go backwards or forwards.

And your patch is a do-four-things-at-once patch. Can you split it up please?

Chris> On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote:

Chris> It still isn't enough to rid of the rwlock_read_locked and Chris> rwlock_write_locked usage in kernel/spinlock.c as those are Chris> needed for the cpu_relax() calls so we have to decide on Chris> suitable names still...

I suggest reversing the sense of the macros, and having read_can_lock() and write_can_lock()

Meaning:
read_can_lock() --- a read_lock() would have succeeded write_can_lock() --- a write_lock() would have succeeded.

IA64 implementation:

#define read_can_lock(x) (*(volatile int *)x >= 0) #define write_can_lock(x) (*(volatile int *)x = 0)

Then use them as

     !read_can_lock(x)

where you want the old semantics. The compiler ought to be smart enough to optimise the boolean ops.

---
Dr Peter Chubb http://www.gelato.unsw.edu.au peterc AT gelato.unsw.edu.au The technical we do immediately, the political takes *forever*

i solved the problem differently in my patch sent to lkml today: i introduced read_trylock_test()/etc. variants which mirror the semantics of the trylock primitives and solve the needs of the PREEMPT branch within kernel/spinlock.c.

Ingo

Yes. This has the advantage of being readable, and the "sense" of the test always being obvious.

We have a sense problem with the "trylock()" cases - some return "it was locked" (semaphores), and some return "I succeeded" (spinlocks), so not only is the sense not immediately obvious from the usage, it's actually _different_ for semaphores and for spinlocks.

So I like "read_can_lock()", since it's also obvious what it returns.

(And yes, we should fix the semaphore trylock return code, dammit.)

Linus

well, this is primarily a problem of the semaphore primitives.

anyway, here's my first patch again, with s/trylock_test/can_lock/.

Ingo

--
it fixes the BUILD_LOCK_OPS() bug by introducing the following 3 new locking primitives:

spin_can_lock(lock)
read_can_lock(lock)
write_can_lock(lock)

this is what is needed by BUILD_LOCK_OPS(): a nonintrusive test to check whether the real (intrusive) trylock op would succeed or not. Semantics and naming is completely symmetric to the trylock counterpart. No changes to exit.c.

build/boot-tested on x86. Architectures that want to support PREEMPT need to add these definitions.

@@ -173,8 +173,8 @@ EXPORT_SYMBOL(_write_lock);
  * (We do this in a function because inlining it would be excessive.)
  */
 
-#define BUILD_LOCK_OPS(op, locktype, is_locked_fn)			\
-void __lockfunc _##op##_lock(locktype *lock)				\
+#define BUILD_LOCK_OPS(op, locktype)					\
+void __lockfunc _##op##_lock(locktype##_t *lock)			\
 {									\
 	preempt_disable();						\
 	for (;;) {							\

@@ -183,7 +183,7 @@ void __lockfunc _##op##_lock(locktype *l
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while (is_locked_fn(lock) && (lock)->break_lock)	\
+		while (!op##_can_lock(lock) && (lock)->break_lock)	\
 			cpu_relax();					\
 		preempt_disable();					\
 	}								\

@@ -191,7 +191,7 @@ void __lockfunc _##op##_lock(locktype *l
 									\
 EXPORT_SYMBOL(_##op##_lock);						\
 									\
-unsigned long __lockfunc _##op##_lock_irqsave(locktype *lock)		\
+unsigned long __lockfunc _##op##_lock_irqsave(locktype##_t *lock)	\
 {									\
 	unsigned long flags;						\
 									\

@@ -205,7 +205,7 @@ unsigned long __lockfunc _##op##_lock_ir
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while (is_locked_fn(lock) && (lock)->break_lock)	\
+		while (!op##_can_lock(lock) && (lock)->break_lock)	\
 			cpu_relax();					\
 		preempt_disable();					\
 	}								\

@@ -214,14 +214,14 @@ unsigned long __lockfunc _##op##_lock_ir
 									\
 EXPORT_SYMBOL(_##op##_lock_irqsave);					\
 									\
-void __lockfunc _##op##_lock_irq(locktype *lock)			\
+void __lockfunc _##op##_lock_irq(locktype##_t *lock)			\
 {									\
 	_##op##_lock_irqsave(lock);					\
 }									\
 									\
 EXPORT_SYMBOL(_##op##_lock_irq);					\
 									\
-void __lockfunc _##op##_lock_bh(locktype *lock)				\
+void __lockfunc _##op##_lock_bh(locktype##_t *lock)			\
 {									\
 	unsigned long flags;						\
 									\

@@ -246,9 +246,9 @@ EXPORT_SYMBOL(_##op##_lock_bh)
  *         _[spin|read|write]_lock_irqsave()
  *         _[spin|read|write]_lock_bh()
  */
-BUILD_LOCK_OPS(spin, spinlock_t, spin_is_locked);
-BUILD_LOCK_OPS(read, rwlock_t, rwlock_is_locked);
-BUILD_LOCK_OPS(write, rwlock_t, spin_is_locked);
+BUILD_LOCK_OPS(spin, spinlock);
+BUILD_LOCK_OPS(read, rwlock);
+BUILD_LOCK_OPS(write, rwlock);
 
 #endif /* CONFIG_PREEMPT */
 

@@ -584,4 +584,10 @@ static inline int bit_spin_is_locked(int
 #define DEFINE_SPINLOCK(x) spinlock_t x = SPIN_LOCK_UNLOCKED
 #define DEFINE_RWLOCK(x) rwlock_t x = RW_LOCK_UNLOCKED
 
+/**
+ * spin_can_lock - would spin_trylock() succeed?
+ * @lock: the spinlock in question.
+ */
+#define spin_can_lock(lock)		(!spin_is_locked(lock))
+
 #endif /* __LINUX_SPINLOCK_H */

@@ -188,6 +188,18 @@ typedef struct {
 
 #define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
 
+/**
+ * read_can_lock - would read_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+#define read_can_lock(x) (atomic_read((atomic_t *)&(x)->lock) > 0)
+
+/**
+ * write_can_lock - would write_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+#define write_can_lock(x) ((x)->lock = RW_LOCK_BIAS)
+
 /*
  * On x86, we implement read-write locks as a 32-bit counter
  * with the high bit (sign) being the "contended" bit.

[patch respun with s/trylock_test/can_lock/]

--
this patch generalizes the facility of targeted lock-yielding originally implemented for ppc64. This facility enables a virtual CPU to indicate towards the hypervisor which other virtual CPU this CPU is 'blocked on', and hence which CPU the hypervisor should yield the current timeslice to, in order to make progress on releasing the lock.

On physical platforms these functions default to cpu_relax(). Since physical platforms are in the overwhelming majority i've added the two new functions to the new asm-generic/spinlock.h include file - here i hope we can later on move more generic spinlock bits as well.

this patch solves the ppc64/PREEMPT functionality-regression reported by Paul Mackerras. I only tested it on x86, Paul, would you mind to test it on ppc64?

Ingo

@@ -184,7 +184,7 @@ void __lockfunc _##op##_lock(locktype##_
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
 		while (!op##_can_lock(lock) && (lock)->break_lock)	\
-			cpu_relax();					\
+			locktype##_yield(lock);				\
 		preempt_disable();					\
 	}								\
 }									\

@@ -206,7 +206,7 @@ unsigned long __lockfunc _##op##_lock_ir
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
 		while (!op##_can_lock(lock) && (lock)->break_lock)	\
-			cpu_relax();					\
+			locktype##_yield(lock);				\
 		preempt_disable();					\
 	}								\
 	return flags;							\

@@ -23,7 +23,7 @@
 /* waiting for a spinlock... */
 #if defined(CONFIG_PPC_SPLPAR) || defined(CONFIG_PPC_ISERIES)
 
-void __spin_yield(spinlock_t *lock)
+void spinlock_yield(spinlock_t *lock)
 {
 	unsigned int lock_value, holder_cpu, yield_count;
 	struct paca_struct *holder_paca;

@@ -54,7 +54,7 @@ void __spin_yield(spinlock_t *lock)
  * This turns out to be the same for read and write locks, since
  * we only know the holder if it is write-locked.
  */
-void __rw_yield(rwlock_t *rw)
+void rwlock_yield(rwlock_t *rw)
 {
 	int lock_value;
 	unsigned int holder_cpu, yield_count;

@@ -87,7 +87,7 @@ void spin_unlock_wait(spinlock_t *lock)
 	while (lock->lock) {
 		HMT_low();
 		if (SHARED_PROCESSOR)
-			__spin_yield(lock);
+			spinlock_yield(lock);
 	}
 	HMT_medium();
 }

@@ -17,6 +17,8 @@
 #include <asm/intrinsics.h>
 #include <asm/system.h>
 
+#include <asm-generic/spinlock.h>
+
 typedef struct {
 	volatile unsigned int lock;
 #ifdef CONFIG_PREEMPT

@@ -0,0 +1,11 @@
+#ifndef _ASM_GENERIC_SPINLOCK_H
+#define _ASM_GENERIC_SPINLOCK_H
+
+/*
+ * Virtual platforms might use these to
+ * yield to specific virtual CPUs:
+ */
+#define spinlock_yield(lock)	cpu_relax()
+#define rwlock_yield(lock)	cpu_relax()
+
+#endif /* _ASM_GENERIC_SPINLOCK_H */

@@ -206,6 +206,8 @@ typedef struct {
 #define _raw_spin_unlock(lock) do { (void)(lock); } while(0)
 #endif /* CONFIG_DEBUG_SPINLOCK */
 
+#define spinlock_yield(lock)	(void)(lock)
+
 /* RW spinlocks: No debug version */
 
 #if (__GNUC__ > 2)

@@ -224,6 +226,8 @@ typedef struct {
 #define _raw_read_trylock(lock) ({ (void)(lock); (1); })
 #define _raw_write_trylock(lock) ({ (void)(lock); (1); })
 
+#define rwlock_yield(lock)	(void)(lock)
+
 #define _spin_trylock(lock)	({preempt_disable(); _raw_spin_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
 

@@ -7,6 +7,8 @@
 #include <linux/config.h>
 #include <linux/compiler.h>
 
+#include <asm-generic/spinlock.h>
+
 asmlinkage int printk(const char * fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
 

@@ -64,11 +64,11 @@ static __inline__ void _raw_spin_unlock(
 #if defined(CONFIG_PPC_SPLPAR) || defined(CONFIG_PPC_ISERIES)
 /* We only yield to the hypervisor if we are in shared processor mode */
 #define SHARED_PROCESSOR (get_paca()->lppaca.shared_proc)
-extern void __spin_yield(spinlock_t *lock);
-extern void __rw_yield(rwlock_t *lock);
+extern void spinlock_yield(spinlock_t *lock);
+extern void rwlock_yield(rwlock_t *lock);
 #else /* SPLPAR || ISERIES */
-#define __spin_yield(x)	barrier()
-#define __rw_yield(x)	barrier()
+#define spinlock_yield(x)	barrier()
+#define rwlock_yield(x)	barrier()
 #define SHARED_PROCESSOR	0
 #endif
 extern void spin_unlock_wait(spinlock_t *lock);

@@ -109,7 +109,7 @@ static void __inline__ _raw_spin_lock(sp
 		do {
 			HMT_low();
 			if (SHARED_PROCESSOR)
-				__spin_yield(lock);
+				spinlock_yield(lock);
 		} while (likely(lock->lock != 0));
 		HMT_medium();
 	}

@@ -127,7 +127,7 @@ static void __inline__ _raw_spin_lock_fl
 		do {
 			HMT_low();
 			if (SHARED_PROCESSOR)
-				__spin_yield(lock);
+				spinlock_yield(lock);
 		} while (likely(lock->lock != 0));
 		HMT_medium();
 		local_irq_restore(flags_dis);

@@ -201,7 +201,7 @@ static void __inline__ _raw_read_lock(rw
 		do {
 			HMT_low();
 			if (SHARED_PROCESSOR)
-				__rw_yield(rw);
+				rwlock_yield(rw);
 		} while (likely(rw->lock < 0));
 		HMT_medium();
 	}

@@ -258,7 +258,7 @@ static void __inline__ _raw_write_lock(r
 		do {
 			HMT_low();
 			if (SHARED_PROCESSOR)
-				__rw_yield(rw);
+				rwlock_yield(rw);
 		} while (likely(rw->lock != 0));
 		HMT_medium();
 	}

@@ -6,6 +6,8 @@
 #include <asm/page.h>
 #include <linux/config.h>
 
+#include <asm-generic/spinlock.h>
+
 extern int printk(const char * fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
 

[this patch didnt change due to can_lock but i've resent it so that the patch stream is complete.]

--
this patch would have caught the bug in -BK-curr (that patch #1 fixes), via a compiler warning. Test-built/booted on x86.

Ingo

@@ -36,7 +36,10 @@ typedef struct {
 
 #define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 SPINLOCK_MAGIC_INIT }
 
-#define spin_lock_init(x)	do { *(x) = SPIN_LOCK_UNLOCKED; } while(0)
+static inline void spin_lock_init(spinlock_t *lock)
+{
+	*lock = SPIN_LOCK_UNLOCKED;
+}
 
 /*
  * Simple spin lock operations.  There are two variants, one clears IRQ's

@@ -45,8 +48,17 @@ typedef struct {
  * We make no fairness assumptions. They have a cost.
  */
 
-#define spin_is_locked(x)	(*(volatile signed char *)(&(x)->lock) <= 0)
-#define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
+static inline int spin_is_locked(spinlock_t *lock)
+{
+	return *(volatile signed char *)(&lock->lock) <= 0;
+}
+
+static inline void spin_unlock_wait(spinlock_t *lock)
+{
+	do {
+		barrier();
+	} while (spin_is_locked(lock));
+}
 
 #define spin_lock_string \
 	"\n1:\t" \

[patch respun with s/trylock_test/can_lock/]
--

turn x86 rwlock macros into inline functions, to get stricter type-checking. Test-built/booted on x86. (patch comes after all previous spinlock patches.)

Ingo

@@ -198,21 +198,33 @@ typedef struct {
 
 #define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }
 
-#define rwlock_init(x)	do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+static inline void rwlock_init(rwlock_t *rw)
+{
+	*rw = RW_LOCK_UNLOCKED;
+}
 
-#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+static inline int rwlock_is_locked(rwlock_t *rw)
+{
+	return rw->lock != RW_LOCK_BIAS;
+}
 
 /**
  * read_can_lock - would read_trylock() succeed?
  * @lock: the rwlock in question.
  */
-#define read_can_lock(x) (atomic_read((atomic_t *)&(x)->lock) > 0)
+static inline int read_can_lock(rwlock_t *rw)
+{
+	return atomic_read((atomic_t *)&rw->lock) > 0;
+}
 
 /**
  * write_can_lock - would write_trylock() succeed?
  * @lock: the rwlock in question.
  */
-#define write_can_lock(x) ((x)->lock = RW_LOCK_BIAS)
+static inline int write_can_lock(rwlock_t *rw)
+{
+	return atomic_read((atomic_t *)&rw->lock) = RW_LOCK_BIAS;
+}
 
 /*
  * On x86, we implement read-write locks as a 32-bit counter

@@ -241,8 +253,16 @@ static inline void _raw_write_lock(rwloc
 	__build_write_lock(rw, "__write_lock_failed");
 }
 
-#define _raw_read_unlock(rw)		asm volatile("lock ; incl %0" :"=m" ((rw)->lock) : : "memory")
-#define _raw_write_unlock(rw)	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ",%0":"=m" ((rw)->lock) : : "memory")
+static inline void _raw_read_unlock(rwlock_t *rw)
+{
+	asm volatile("lock ; incl %0" :"=m" (rw->lock) : : "memory");
+}
+
+static inline void _raw_write_unlock(rwlock_t *rw)
+{
+	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR
+				",%0":"=m" (rw->lock) : : "memory");
+}
 
 static inline int _raw_read_trylock(rwlock_t *lock)
 {

[this patch didnt change due to can_lock but i've resent it so that the patch stream is complete. this concludes my current spinlock patches.] --

cleanup: remove stale semicolon from linux/spinlock.h and stale space from asm-i386/spinlock.h.

Ingo

@@ -202,7 +202,7 @@ typedef struct {
 #define _raw_spin_lock(lock)	do { (void)(lock); } while(0)
 #define spin_is_locked(lock)	((void)(lock), 0)
 #define _raw_spin_trylock(lock)	(((void)(lock), 1))
-#define spin_unlock_wait(lock)	(void)(lock);
+#define spin_unlock_wait(lock)	(void)(lock)
 #define _raw_spin_unlock(lock) do { (void)(lock); } while(0)
 #endif /* CONFIG_DEBUG_SPINLOCK */
 

@@ -92,7 +92,7 @@ static inline void spin_unlock_wait(spin
  * (except on PPro SMP or if we are using OOSTORE)
  * (PPro errata 66, 92)
  */
- 
+
 #if !defined(CONFIG_X86_OOSTORE) && !defined(CONFIG_X86_PPRO_FENCE)
 
 #define spin_unlock_string \

I don't want to break all the other architectures. Or at least not most of them. Especially since I was hoping to do a -pre2 soon (well, like today, but I guess that's out..) and make the 2.6.11 cycle shorter than 2.6.10.

So I'd like to now _first_ get

for at least most architectures (ie for me at a minimum that is x86, x86-64, ia64 and ppc64 - and obviously the "always true" cases for the UP version).

Ok?

Also, I've already made sure that I can't apply any half-measures by mistake by undoing the mess that it was before, and making sure that any patches I get have to be "clean slate".

That said, I like how just the _renaming_ of the thing (and making them all consistent) made your BUILD_LOCK_OPS() helper macro much simpler. So I'm convinced that this is the right solution - I just want to not screw up other architectures.

I can do ppc64 myself, can others fix the other architectures (Ingo, shouldn't the UP case have the read/write_can_lock() cases too? And wouldn't you agree that it makes more sense to have the rwlock test variants in asm/rwlock.h?):

Linus

You are right about UP, and the patch below adds the UP variants. It's analogous to the existing wrapping concept that UP 'spinlocks' are always unlocked on UP. (spin_can_lock() is already properly defined on UP too.)

Ingo

@@ -228,6 +228,9 @@ typedef struct {
 
 #define rwlock_yield(lock)	(void)(lock)
 
+#define read_can_lock(lock)	(((void)(lock), 1))
+#define write_can_lock(lock)	(((void)(lock), 1))
+
 #define _spin_trylock(lock)	({preempt_disable(); _raw_spin_trylock(lock) ? \
 				1 : ({preempt_enable(); 0;});})
 

Looking closer, it _looks_ like the spinlock debug case never had a "spin_is_locked()" define at all. Or am I blind? Maybe UP doesn't want/need it after all?

Linus

i remember frequently breaking the UP build wrt. spin_is_locked() when redoing all the spinlock primitives for PREEMPT_RT.

looking closer, here's the debug variant it appears:

/* without debugging, spin_is_locked on UP always says
* FALSE. --> printk if already locked. */

 #define spin_is_locked(x) \
	({ \
	 	CHECK_LOCK(x); \
		if ((x)->lock&&(x)->babble) { \
			(x)->babble--; \
			printk("%s:%d: spin_is_locked(%s:%p) already locked by %s/%d\n", \
					__FILE__,__LINE__, (x)->module, \
					(x), (x)->owner, (x)->oline); \
		} \
		0; \
	})

(the comment is misleading a bit, this _is_ the debug branch. The nondebug branch has a spin_is_locked() definition too.)

Ingo

this patch, against BK-curr, implements a nonintrusive spin-polling loop for the SMP+PREEMPT spinlock/rwlock variants, using the new *_can_lock() primitives. (The patch also adds *_can_lock() to the UP branch of spinlock.h, for completeness.)

build- and boot-tested on x86 SMP+PREEMPT and SMP+!PREEMPT.

Ingo

@@ -174,7 +174,7 @@ EXPORT_SYMBOL(_write_lock);
  */
 
 #define BUILD_LOCK_OPS(op, locktype)					\
-void __lockfunc _##op##_lock(locktype *lock)				\
+void __lockfunc _##op##_lock(locktype##_t *lock)			\
 {									\
 	preempt_disable();						\
 	for (;;) {							\

@@ -183,14 +183,15 @@ void __lockfunc _##op##_lock(locktype *l
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		cpu_relax();						\
+		while (!op##_can_lock(lock) && (lock)->break_lock)	\
+			cpu_relax();					\
 		preempt_disable();					\
 	}								\
 }									\
 									\
 EXPORT_SYMBOL(_##op##_lock);						\
 									\
-unsigned long __lockfunc _##op##_lock_irqsave(locktype *lock)		\
+unsigned long __lockfunc _##op##_lock_irqsave(locktype##_t *lock)	\
 {									\
 	unsigned long flags;						\
 									\

@@ -204,7 +205,8 @@ unsigned long __lockfunc _##op##_lock_ir
 		preempt_enable();					\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		cpu_relax();						\
+		while (!op##_can_lock(lock) && (lock)->break_lock)	\
+			cpu_relax();					\
 		preempt_disable();					\
 	}								\
 	return flags;							\

@@ -212,14 +214,14 @@ unsigned long __lockfunc _##op##_lock_ir
 									\
 EXPORT_SYMBOL(_##op##_lock_irqsave);					\
 									\
-void __lockfunc _##op##_lock_irq(locktype *lock)			\
+void __lockfunc _##op##_lock_irq(locktype##_t *lock)			\
 {									\
 	_##op##_lock_irqsave(lock);					\
 }									\
 									\
 EXPORT_SYMBOL(_##op##_lock_irq);					\
 									\
-void __lockfunc _##op##_lock_bh(locktype *lock)				\
+void __lockfunc _##op##_lock_bh(locktype##_t *lock)			\
 {									\
 	unsigned long flags;						\
 									\

@@ -244,9 +246,9 @@ EXPORT_SYMBOL(_##op##_lock_bh)
  *         _[spin|read|write]_lock_irqsave()
  *         _[spin|read|write]_lock_bh()
  */
-BUILD_LOCK_OPS(spin, spinlock_t);
-BUILD_LOCK_OPS(read, rwlock_t);
-BUILD_LOCK_OPS(write, rwlock_t);
+BUILD_LOCK_OPS(spin, spinlock);
+BUILD_LOCK_OPS(read, rwlock);
+BUILD_LOCK_OPS(write, rwlock);
 
 #endif /* CONFIG_PREEMPT */
 

@@ -221,6 +221,8 @@ typedef struct {
 #define _raw_read_unlock(lock)	do { (void)(lock); } while(0)
 #define _raw_write_lock(lock)	do { (void)(lock); } while(0)
 #define _raw_write_unlock(lock)	do { (void)(lock); } while(0)
+#define read_can_lock(lock)	(((void)(lock), 1))
+#define write_can_lock(lock)	(((void)(lock), 1))
 #define _raw_read_trylock(lock) ({ (void)(lock); (1); })
 #define _raw_write_trylock(lock) ({ (void)(lock); (1); })
 

this x86 patch, against BK-curr, renames spinlock_t's 'lock' field to 'slock', to protect against spinlock_t/rwlock_t type mismatches.

build- and boot-tested on x86 SMP+PREEMPT and SMP+!PREEMPT.

Ingo

@@ -15,7 +15,7 @@ asmlinkage int printk(const char * fmt, 
  */
 
 typedef struct {
-	volatile unsigned int lock;
+	volatile unsigned int slock;
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned magic;
 #endif

@@ -43,7 +43,7 @@ typedef struct {
  * We make no fairness assumptions. They have a cost.
  */
 
-#define spin_is_locked(x)	(*(volatile signed char *)(&(x)->lock) <= 0)
+#define spin_is_locked(x)	(*(volatile signed char *)(&(x)->slock) <= 0)
 #define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
 
 #define spin_lock_string \

@@ -83,7 +83,7 @@ typedef struct {
 
 #define spin_unlock_string \
 	"movb $1,%0" \
-		:"=m" (lock->lock) : : "memory"
+		:"=m" (lock->slock) : : "memory"
 
 
 static inline void _raw_spin_unlock(spinlock_t *lock)

@@ -101,7 +101,7 @@ static inline void _raw_spin_unlock(spin
 
 #define spin_unlock_string \
 	"xchgb %b0, %1" \
-		:"=q" (oldval), "=m" (lock->lock) \
+		:"=q" (oldval), "=m" (lock->slock) \
 		:"0" (oldval) : "memory"
 
 static inline void _raw_spin_unlock(spinlock_t *lock)

@@ -123,7 +123,7 @@ static inline int _raw_spin_trylock(spin
 	char oldval;
 	__asm__ __volatile__(
 		"xchgb %b0,%1"
-		:"=q" (oldval), "=m" (lock->lock)
+		:"=q" (oldval), "=m" (lock->slock)
 		:"0" (0) : "memory");
 	return oldval > 0;
 }

@@ -138,7 +138,7 @@ static inline void _raw_spin_lock(spinlo
 #endif
 	__asm__ __volatile__(
 		spin_lock_string
-		:"=m" (lock->lock) : : "memory");
+		:"=m" (lock->slock) : : "memory");
 }
 
 static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)

@@ -151,7 +151,7 @@ static inline void _raw_spin_lock_flags 
 #endif
 	__asm__ __volatile__(
 		spin_lock_string_flags
-		:"=m" (lock->lock) : "r" (flags) : "memory");
+		:"=m" (lock->slock) : "r" (flags) : "memory");
 }
 
 /*

Btw, I think I've now merged everything to bring us back to where we wanted to be - can people verify that the architecture they care about has all the right "read_can_lock()" etc infrastructure (and preferably that it _works_ too ;), and that I've not missed of incorrectly ignored some patches in this thread?

Linus

this one adds it to x64. (untested at the moment) This patch assumes that we are nuking rwlock_is_locked and that there is at least a s/rwlock_is_locked/!write_can_lock/ done to kernel/exit.c.

Ingo

@@ -161,7 +161,23 @@ typedef struct {
 
 #define rwlock_init(x)	do { *(x) = RW_LOCK_UNLOCKED; } while(0)
 
-#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+/**
+ * read_can_lock - would read_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+static inline int read_can_lock(rwlock_t *rw)
+{
+	return rw->lock > 0;
+}
+
+/**
+ * write_can_lock - would write_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+static inline int write_can_lock(rwlock_t *rw)
+{
+	return rw->lock = RW_LOCK_BIAS;
+}
 
 /*
  * On x86, we implement read-write locks as a 32-bit counter

with this patch the x64 SMP+PREEMPT kernel builds & boots fine on an UP x64 box. (this is not a full test but better than nothing.) [the other 8 spinlock patches were all applied as well.]

Ingo

(untested) ia64 version below. Differs from x86/x64.

Ingo

@@ -128,8 +128,27 @@ typedef struct {
 #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
 
 #define rwlock_init(x)		do { *(x) = RW_LOCK_UNLOCKED; } while(0)
-#define rwlock_is_locked(x)	(*(volatile int *) (x) != 0)
 
+/**
+ * read_can_lock - would read_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+static inline int read_can_lock(rwlock_t *rw)
+{
+	return *(volatile int *)rw >= 0;
+}
+
+/**
+ * write_can_lock - would write_trylock() succeed?
+ * @lock: the rwlock in question.
+ */
+static inline int write_can_lock(rwlock_t *rw)
+{
+	return *(volatile int *)rw = 0;
+}
+ 
+ /*
+  * On x86, we implement read-write locks as a 32-bit counter
 #define _raw_read_lock(rw)								\
 do {											\
 	rwlock_t *__read_lock_ptr = (rw);						\

if we remove the debugging check from exit.c then the only thing that might break in an architecture is SMP+PREEMPT, which is rarely used outside of the x86-ish architectures.

Ingo

How about I just kill it now, so that it just doesn't exist, and the dust (from all the other things) can settle where it will?

In fact, I think I will remove the whole "rwlock_is_locked()" thing and the only user, since it's all clearly broken, and regardless of what we do it will be something else. That will at least fix the current problem, and only leave us doing too many bus accesses when BKL_PREEMPT is enabled.

Linus

in the 5-patch stream i just sent there's no need to touch exit.c, and the debugging check didnt hurt. But if you remove it from spinlock.h now then i'll probably have to regenerate the 5 patches again :-| We can:

- nuke it afterwards

- or can leave it alone as-is (it did catch a couple of bugs in the past)

- or can change the rwlock_is_locked() to !write_can_lock() and remove rwlock_is_locked() [!write_can_lock() is a perfect replacement for it].

i'd prefer #3.

Ingo

I actually much prefer the "read_can_lock()" suggestion by Peter.

Also, why this:

	+#define rwlock_read_locked(x) (atomic_read((atomic_t *)&(x)->lock) <= 0)

what the _heck_ is that "atomic_read((atomic_t *)&(x)->lock)", and why is it not just a "(int)(x)->lock" instead?

So I think it would be much better as

	#define read_can_lock(x) ((int)(x)->lock > 0)

which seems simple and straightforward.

And it probably should be in <asm-i386/rwlock.h>, since that is where the actual implementation is, and <asm-i386/spinlock.h> doesn't really have any clue what the rules are, and shouldn't act like it has.

Linus

right. Replace patch #4 with:

@@ -198,21 +198,33 @@ typedef struct {
 
 #define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }
 
-#define rwlock_init(x)	do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+static inline void rwlock_init(rwlock_t *rw)
+{
+	*rw = RW_LOCK_UNLOCKED;
+}
 
-#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)
+static inline int rwlock_is_locked(rwlock_t *rw)
+{
+	return rw->lock != RW_LOCK_BIAS;
+}
 
 /**
  * read_can_lock - would read_trylock() succeed?
  * @lock: the rwlock in question.
  */
-#define read_can_lock(x) (atomic_read((atomic_t *)&(x)->lock) > 0)
+static inline int read_can_lock(rwlock_t *rw)
+{
+	return rw->lock > 0;
+}
 
 /**
  * write_can_lock - would write_trylock() succeed?
  * @lock: the rwlock in question.
  */
-#define write_can_lock(x) ((x)->lock = RW_LOCK_BIAS)
+static inline int write_can_lock(rwlock_t *rw)
+{
+	return rw->lock = RW_LOCK_BIAS;
+}
 
 /*
  * On x86, we implement read-write locks as a 32-bit counter

@@ -241,8 +253,16 @@ static inline void _raw_write_lock(rwloc
 	__build_write_lock(rw, "__write_lock_failed");
 }
 
-#define _raw_read_unlock(rw)		asm volatile("lock ; incl %0" :"=m" ((rw)->lock) : : "memory")
-#define _raw_write_unlock(rw)	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ",%0":"=m" ((rw)->lock) : : "memory")
+static inline void _raw_read_unlock(rwlock_t *rw)
+{
+	asm volatile("lock ; incl %0" :"=m" (rw->lock) : : "memory");
+}
+
+static inline void _raw_write_unlock(rwlock_t *rw)
+{
+	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR
+				",%0":"=m" (rw->lock) : : "memory");
+}
 
 static inline int _raw_read_trylock(rwlock_t *lock)
 {

indeed. Another solution would be to make the type signed - patch below. (like ppc64 does)

(i sent the original lock/slock renaming patch yesterday, and i think Chris simply reworked&resent that one.)

Ingo

@@ -179,7 +179,7 @@ static inline void _raw_spin_lock_flags 
  * read-locks.
  */
 typedef struct {
-	volatile unsigned int lock;
+	volatile signed int lock;
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned magic;
 #endif

historically spinlock.h had the full implementation of both spinlock
variants: spinlocks and rwlocks. (hey, you implemented it first and put it there! :-) Then came Ben's rwsems that wanted pieces of rw-spinlocks, so rwlock.h was created with the shared bits.

one thing i was thinking about was to move most but the assembly to asm-generic/spinlock.h. Almost every architecture shares the spinlock type definitions and shares most of the non-assembly functions.

Ingo

Erm...those names sound way too much like existing interfaces. Perhaps check_read_lock()/check_write_lock() ?

If they don't get used too much, the longer name should be ok.

grant

Actually, it is unrelated. Proposed fix:

---

It seems logical that __get_cpu_var should use __smp_processor_id() rather than smp_processor_id(). Noticed when __get_cpu_var was making lots of noise with CONFIG_DEBUG_PREEMPT=y

===== include/asm-generic/percpu.h 1.10 vs edited =====

@@ -13,7 +13,7 @@ extern unsigned long __per_cpu_offset[NR
 
 /* var is in discarded region: offset to particular copy we want */
 #define per_cpu(var, cpu) (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
-#define __get_cpu_var(var) per_cpu(var, smp_processor_id())
+#define __get_cpu_var(var) per_cpu(var, __smp_processor_id())
 
 /* A macro to avoid #include hell... */
 #define percpu_modcopy(pcpudst, src, size)			\

no ... normally you should only use __get_cpu_var() if you know that you are in a non-preempt case. It's a __ internal function for a reason. Where did it trigger?

Ingo

XFS has statistics which are 'per cpu' but doesn't use per_cpu variables, __get_cpu_var(foo)++ is used (it doesn't have to be preempt safe since it's just stats and who cares if they are a bit wrong).