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 */
Chris Wedgwood <[email protected]> wrote:
>
> 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).
>
> ...
> +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);
If you replace the last line with
BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
does it help?
On Sun, Jan 16, 2005 at 11:09:22PM -0800, Andrew Morton wrote:
> If you replace the last line with
>
> BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
>
> does it help?
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.
Signed-off-by: Chris Wedgwood <[email protected]>
===== include/asm-i386/spinlock.h 1.16 vs edited =====
--- 1.16/include/asm-i386/spinlock.h 2005-01-07 21:43:58 -08:00
+++ edited/include/asm-i386/spinlock.h 2005-01-16 23:23:50 -08:00
@@ -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 =====
--- 1.4/kernel/spinlock.c 2005-01-14 16:00:00 -08:00
+++ edited/kernel/spinlock.c 2005-01-16 23:25:11 -08:00
@@ -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 */
On Sun, Jan 16, 2005 at 09:50:44PM -0800, Chris Wedgwood wrote:
> 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.
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
Signed-off-by: Chris Wedgwood <[email protected]>
===== include/asm-generic/percpu.h 1.10 vs edited =====
--- 1.10/include/asm-generic/percpu.h 2004-01-18 22:28:34 -08:00
+++ edited/include/asm-generic/percpu.h 2005-01-16 22:32:07 -08:00
@@ -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) \
Chris Wedgwood writes:
> +#define rwlock_is_write_locked(x) ((x)->lock == 0)
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.
On Mon, Jan 17, 2005 at 06:50:57PM +1100, Paul Mackerras wrote:
> 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)
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).
> (or the equivalent using atomic_read).
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.
* Andrew Morton <[email protected]> wrote:
> > +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);
>
> If you replace the last line with
>
> BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
>
> does it help?
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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/kernel/spinlock.c.orig
+++ linux/kernel/spinlock.c
@@ -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 */
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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
--- linux/kernel/exit.c.orig
+++ linux/kernel/exit.c
@@ -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);
* Chris Wedgwood <[email protected]> wrote:
> 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
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
On Mon, Jan 17, 2005 at 03:40:16PM +0100, Ingo Molnar wrote:
> 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?
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).
Hi Ingo
On Mon, 17 Jan 2005, Ingo Molnar wrote:
>
> * Andrew Morton <[email protected]> wrote:
>
> > > +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);
> >
> > If you replace the last line with
> >
> > BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
> >
> > does it help?
>
> 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
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
>
> Signed-off-by: Ingo Molnar <[email protected]>
>
> --- linux/kernel/spinlock.c.orig
> +++ linux/kernel/spinlock.c
> @@ -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 */
>
> --- linux/include/asm-i386/spinlock.h.orig
> +++ linux/include/asm-i386/spinlock.h
> @@ -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
> --- linux/kernel/exit.c.orig
> +++ linux/kernel/exit.c
> @@ -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);
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--------------------------------------------------
Darren Williams <dsw AT gelato.unsw.edu.au>
Gelato@UNSW <http://www.gelato.unsw.edu.au>
--------------------------------------------------
On Tue, 18 Jan 2005, Darren Williams wrote:
> Hi Ingo
>
> On Mon, 17 Jan 2005, Ingo Molnar wrote:
>
> >
> > * Andrew Morton <[email protected]> wrote:
> >
> > > > +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);
> > >
> > > If you replace the last line with
> > >
> > > BUILD_LOCK_OPS(write, rwlock_t, rwlock_is_locked);
> > >
> > > does it help?
> >
> > 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
> 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.
>
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
>
> >
> > Signed-off-by: Ingo Molnar <[email protected]>
> >
> > --- linux/kernel/spinlock.c.orig
> > +++ linux/kernel/spinlock.c
> > @@ -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 */
> >
> > --- linux/include/asm-i386/spinlock.h.orig
> > +++ linux/include/asm-i386/spinlock.h
> > @@ -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
> > --- linux/kernel/exit.c.orig
> > +++ linux/kernel/exit.c
> > @@ -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);
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> --------------------------------------------------
> Darren Williams <dsw AT gelato.unsw.edu.au>
> Gelato@UNSW <http://www.gelato.unsw.edu.au>
> --------------------------------------------------
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--------------------------------------------------
Darren Williams <dsw AT gelato.unsw.edu.au>
Gelato@UNSW <http://www.gelato.unsw.edu.au>
--------------------------------------------------
On Tue, Jan 18, 2005 at 03:28:58PM +1100, Darren Williams wrote:
> On top of Ingo's patch I attempt a solution that failed:
> +#define read_is_locked(x) (*(volatile int *) (x) > 0)
> +#define write_is_locked(x) (*(volatile int *) (x) < 0)
how about something like:
#define read_is_locked(x) (*(volatile int *) (x) != 0)
#define write_is_locked(x) (*(volatile int *) (x) & (1<<31))
I'm not masking the write-bit for read_is_locked here, I'm not sure is
we should?
--cw
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.
Signed-off-by: Peter Chubb <[email protected]>
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*
* Peter Chubb <[email protected]> wrote:
> 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.
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" == Ingo Molnar <[email protected]> writes:
Ingo> * Peter Chubb <[email protected]> wrote:
>> 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.
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*
* Peter Chubb <[email protected]> wrote:
> >> 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.
>
> Ingo> well, 'read_is_locked()' means: "will a read_lock() succeed"
>
> Fail, surely?
yeah ... and with that i proved beyond doubt that the naming is indeed
unintuitive :-)
Ingo
Ingo Molnar writes:
> * Peter Chubb <[email protected]> wrote:
>
> > >> 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.
> >
> > Ingo> well, 'read_is_locked()' means: "will a read_lock() succeed"
> >
> > Fail, surely?
>
> yeah ... and with that i proved beyond doubt that the naming is indeed
> unintuitive :-)
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.
On Thu, Jan 20, 2005 at 08:43:30AM +1100, Paul Mackerras wrote:
> 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.
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:
Chris Wedgwood <[email protected]> wrote:
>
> if (!spin_is_locked(&p->sighand->siglock) &&
> - !rwlock_is_locked(&tasklist_lock))
> + !rwlock_write_locked(&tasklist_lock))
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(...))
On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote:
> ... how about we simply nuke this statement:
>
> Chris Wedgwood <[email protected]> wrote:
> >
> > if (!spin_is_locked(&p->sighand->siglock) &&
> > - !rwlock_is_locked(&tasklist_lock))
> > + !rwlock_write_locked(&tasklist_lock))
>
> and be done with the whole thing?
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...
Chris Wedgwood <[email protected]> wrote:
>
> On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote:
>
> > ... how about we simply nuke this statement:
> >
> > Chris Wedgwood <[email protected]> wrote:
> > >
> > > if (!spin_is_locked(&p->sighand->siglock) &&
> > > - !rwlock_is_locked(&tasklist_lock))
> > > + !rwlock_write_locked(&tasklist_lock))
> >
> > and be done with the whole thing?
>
> 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?
On Thu, Jan 20, 2005 at 08:43:30AM +1100, Paul Mackerras wrote:
> I suggest read_poll(), write_poll(), spin_poll(),...
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
>>>>> "Chris" == Chris Wedgwood <[email protected]> writes:
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*
* Peter Chubb <[email protected]> wrote:
> 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.
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
On Thu, 20 Jan 2005, Peter Chubb wrote:
>
> 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.
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
On Wed, 19 Jan 2005, Chris Wedgwood wrote:
>
> On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote:
>
> > ... how about we simply nuke this statement:
> >
> > Chris Wedgwood <[email protected]> wrote:
> > >
> > > if (!spin_is_locked(&p->sighand->siglock) &&
> > > - !rwlock_is_locked(&tasklist_lock))
> > > + !rwlock_write_locked(&tasklist_lock))
> >
> > and be done with the whole thing?
>
> I'm all for killing that. I'll happily send a patch once the dust
> settles.
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
* Linus Torvalds <[email protected]> wrote:
> 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.
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.
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/kernel/spinlock.c.orig
+++ linux/kernel/spinlock.c
@@ -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 */
--- linux/include/linux/spinlock.h.orig
+++ linux/include/linux/spinlock.h
@@ -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 */
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/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.
[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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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/]
--
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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/kernel/spinlock.c.orig
+++ linux/kernel/spinlock.c
@@ -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; \
--- linux/arch/ppc64/lib/locks.c.orig
+++ linux/arch/ppc64/lib/locks.c
@@ -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();
}
--- linux/include/asm-ia64/spinlock.h.orig
+++ linux/include/asm-ia64/spinlock.h
@@ -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
--- linux/include/asm-generic/spinlock.h.orig
+++ linux/include/asm-generic/spinlock.h
@@ -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 */
--- linux/include/linux/spinlock.h.orig
+++ linux/include/linux/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;});})
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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)));
--- linux/include/asm-ppc64/spinlock.h.orig
+++ linux/include/asm-ppc64/spinlock.h
@@ -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();
}
--- linux/include/asm-x86_64/spinlock.h.orig
+++ linux/include/asm-x86_64/spinlock.h
@@ -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)));
On Wed, 19 Jan 2005, Chris Wedgwood wrote:
>
> * i386, ia64: rename rwlock_is_locked to rwlock_write_locked as this
> is IMO a better name
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
* Linus Torvalds <[email protected]> wrote:
> 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.
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
* Linus Torvalds <[email protected]> wrote:
> 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.
right. Replace patch #4 with:
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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)
{
[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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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)
{
* Linus Torvalds <[email protected]> wrote:
> 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.
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
On Thu, 20 Jan 2005, Ingo Molnar wrote:
>
> anyway, here's my first patch again, with s/trylock_test/can_lock/.
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
> spin_can_lock(lock)
> read_can_lock(lock)
> write_can_lock(lock)
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
> --- linux/include/linux/spinlock.h.orig
> +++ linux/include/linux/spinlock.h
> @@ -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 */
> --- linux/include/asm-i386/spinlock.h.orig
> +++ linux/include/asm-i386/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.
>
* Linus Torvalds <[email protected]> wrote:
> 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?):
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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/asm-x86_64/spinlock.h.orig
+++ linux/include/asm-x86_64/spinlock.h
@@ -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
* Linus Torvalds <[email protected]> wrote:
> 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?):
(untested) ia64 version below. Differs from x86/x64.
Ingo
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/asm-ia64/spinlock.h.orig
+++ linux/include/asm-ia64/spinlock.h
@@ -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); \
* Linus Torvalds <[email protected]> wrote:
> 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.
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
* Linus Torvalds <[email protected]> wrote:
> 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?):
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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/linux/spinlock.h.orig
+++ linux/include/linux/spinlock.h
@@ -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;});})
* Ingo Molnar <[email protected]> wrote:
> * Linus Torvalds <[email protected]> wrote:
>
> > 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?):
>
> this one adds it to x64. (untested at the moment) [...]
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
[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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/linux/spinlock.h.orig
+++ linux/include/linux/spinlock.h
@@ -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 */
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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 \
On Thu, 20 Jan 2005, Ingo Molnar wrote:
>
> right. Replace patch #4 with:
>
> /**
> * 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;
> +}
No, it does need the cast to signed, otherwise it will return true for the
case where somebody holds the write-lock _and_ there's a pending reader
waiting too (the write-lock will make it zero, the pending reader will
wrap around and make it negative, but since "lock" is "unsigned", it will
look like a large value to "read_can_lock".
I also think I'd prefer to do the things as macros, and do the type-safety
by just renaming the "lock" field like Chris did. We had an issue with gcc
being very slow recently, and that was due to some inline functions in
header files: gcc does a lot of work on an inline function regardless of
whether it is used or not, and the spinlock header file is included pretty
much _everywhere_...
Clearly inline functions are "nicer", but they do come with a cost.
Linus
* Linus Torvalds <[email protected]> wrote:
> > +static inline int read_can_lock(rwlock_t *rw)
> > +{
> > + return rw->lock > 0;
> > +}
>
> No, it does need the cast to signed, otherwise it will return true for
> the case where somebody holds the write-lock _and_ there's a pending
> reader waiting too (the write-lock will make it zero, the pending
> reader will wrap around and make it negative, but since "lock" is
> "unsigned", it will look like a large value to "read_can_lock".
indeed. Another solution would be to make the type signed - patch below.
(like ppc64 does)
> I also think I'd prefer to do the things as macros, and do the
> type-safety by just renaming the "lock" field like Chris did. [...]
(i sent the original lock/slock renaming patch yesterday, and i think
Chris simply reworked&resent that one.)
Ingo
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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
On Thu, 20 Jan 2005, Ingo Molnar wrote:
>
> 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.)
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
* Linus Torvalds <[email protected]> wrote:
> On Thu, 20 Jan 2005, Ingo Molnar wrote:
> >
> > 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.)
>
> 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?
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 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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/include/asm-i386/spinlock.h.orig
+++ linux/include/asm-i386/spinlock.h
@@ -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");
}
/*
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
Signed-off-by: Ingo Molnar <[email protected]>
--- linux/kernel/spinlock.c.orig
+++ linux/kernel/spinlock.c
@@ -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 */
--- linux/include/linux/spinlock.h.orig
+++ linux/include/linux/spinlock.h
@@ -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); })
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