From: Ingo Molnar <mingo@elte.hu>
To: Andrew Morton <akpm@osdl.org>
Cc: Chris Wedgwood <cw@f00f.org>,
torvalds@osdl.org, benh@kernel.crashing.org,
linux-kernel@vger.kernel.org
Subject: Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch
Date: Mon, 17 Jan 2005 15:33:01 +0100 [thread overview]
Message-ID: <20050117143301.GA10341@elte.hu> (raw)
In-Reply-To: <20050116230922.7274f9a2.akpm@osdl.org>
* Andrew Morton <akpm@osdl.org> 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 <mingo@elte.hu>
--- 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);
next prev parent reply other threads:[~2005-01-17 14:33 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-01-17 5:50 Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch Chris Wedgwood
2005-01-17 7:09 ` Andrew Morton
2005-01-17 7:33 ` Chris Wedgwood
2005-01-17 7:50 ` Paul Mackerras
2005-01-17 8:00 ` Chris Wedgwood
2005-01-17 14:33 ` Ingo Molnar [this message]
2005-01-18 1:47 ` Darren Williams
2005-01-18 4:28 ` Darren Williams
2005-01-18 7:08 ` Chris Wedgwood
2005-01-19 0:14 ` Peter Chubb
2005-01-19 8:04 ` Ingo Molnar
2005-01-19 9:18 ` Peter Chubb
2005-01-19 9:20 ` Ingo Molnar
2005-01-19 21:43 ` Paul Mackerras
2005-01-20 2:34 ` [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] Chris Wedgwood
2005-01-20 3:01 ` Andrew Morton
2005-01-20 3:18 ` Chris Wedgwood
2005-01-20 3:33 ` Andrew Morton
2005-01-20 8:59 ` Peter Chubb
2005-01-20 13:04 ` Ingo Molnar
2005-01-20 15:51 ` Linus Torvalds
2005-01-20 16:08 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar
2005-01-20 16:11 ` [patch 2/3] spinlock fix #2: generalize [spin|rw]lock yielding Ingo Molnar
2005-01-20 16:12 ` [patch 3/3] spinlock fix #3: type-checking spinlock primitives, x86 Ingo Molnar
2005-01-20 16:14 ` [patch] stricter type-checking rwlock " Ingo Molnar
2005-01-20 16:16 ` [patch] minor spinlock cleanups Ingo Molnar
2005-01-20 16:31 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Linus Torvalds
2005-01-20 16:40 ` Ingo Molnar
2005-01-20 17:48 ` Linus Torvalds
2005-01-20 17:53 ` Ingo Molnar
2005-01-20 18:22 ` [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c Ingo Molnar
2005-01-20 18:25 ` [patch, BK-curr] rename 'lock' to 'slock' in asm-i386/spinlock.h Ingo Molnar
2005-01-20 23:45 ` [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c Linus Torvalds
2005-01-20 16:44 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar
2005-01-20 16:59 ` Ingo Molnar
2005-01-20 16:47 ` Ingo Molnar
2005-01-20 16:57 ` Ingo Molnar
2005-01-20 16:05 ` [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] Linus Torvalds
2005-01-20 16:20 ` Ingo Molnar
2005-01-20 16:18 ` Linus Torvalds
2005-01-20 16:23 ` Ingo Molnar
2005-01-20 17:30 ` Linus Torvalds
2005-01-20 17:38 ` Ingo Molnar
2005-01-20 16:28 ` Ingo Molnar
2005-01-20 5:49 ` Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch Grant Grundler
2005-01-17 7:38 ` [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() Chris Wedgwood
2005-01-17 14:40 ` Ingo Molnar
2005-01-17 18:53 ` Chris Wedgwood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20050117143301.GA10341@elte.hu \
--to=mingo@elte.hu \
--cc=akpm@osdl.org \
--cc=benh@kernel.crashing.org \
--cc=cw@f00f.org \
--cc=linux-kernel@vger.kernel.org \
--cc=torvalds@osdl.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).