* Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch @ 2005-01-17 5:50 Chris Wedgwood 2005-01-17 7:09 ` Andrew Morton 2005-01-17 7:38 ` [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() Chris Wedgwood 0 siblings, 2 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-17 5:50 UTC (permalink / raw) To: Linus Torvalds, Ingo Molnar; +Cc: cw, Benjamin Herrenschmidt, LKML 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 mingo@elte.hu # [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 <mingo@elte.hu> # Signed-off-by: Linus Torvalds <torvalds@osdl.org> # # kernel/spinlock.c # 2005/01/14 16:00:00-08:00 mingo@elte.hu +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 */ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 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 14:33 ` Ingo Molnar 2005-01-17 7:38 ` [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() Chris Wedgwood 1 sibling, 2 replies; 48+ messages in thread From: Andrew Morton @ 2005-01-17 7:09 UTC (permalink / raw) To: Chris Wedgwood; +Cc: torvalds, mingo, cw, benh, linux-kernel Chris Wedgwood <cw@f00f.org> 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? ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-17 7:09 ` Andrew Morton @ 2005-01-17 7:33 ` Chris Wedgwood 2005-01-17 7:50 ` Paul Mackerras 2005-01-17 14:33 ` Ingo Molnar 1 sibling, 1 reply; 48+ messages in thread From: Chris Wedgwood @ 2005-01-17 7:33 UTC (permalink / raw) To: Andrew Morton; +Cc: torvalds, mingo, benh, linux-kernel, Paul Mackerras 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 <cw@f00f.org> ===== 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 */ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-17 7:33 ` Chris Wedgwood @ 2005-01-17 7:50 ` Paul Mackerras 2005-01-17 8:00 ` Chris Wedgwood 0 siblings, 1 reply; 48+ messages in thread From: Paul Mackerras @ 2005-01-17 7:50 UTC (permalink / raw) To: Chris Wedgwood; +Cc: Andrew Morton, torvalds, mingo, benh, linux-kernel 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-17 7:50 ` Paul Mackerras @ 2005-01-17 8:00 ` Chris Wedgwood 0 siblings, 0 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-17 8:00 UTC (permalink / raw) To: Paul Mackerras; +Cc: Andrew Morton, torvalds, mingo, benh, linux-kernel 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-17 7:09 ` Andrew Morton 2005-01-17 7:33 ` Chris Wedgwood @ 2005-01-17 14:33 ` Ingo Molnar 2005-01-18 1:47 ` Darren Williams 1 sibling, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-17 14:33 UTC (permalink / raw) To: Andrew Morton; +Cc: Chris Wedgwood, torvalds, benh, linux-kernel * 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); ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-17 14:33 ` Ingo Molnar @ 2005-01-18 1:47 ` Darren Williams 2005-01-18 4:28 ` Darren Williams 2005-01-19 0:14 ` Peter Chubb 0 siblings, 2 replies; 48+ messages in thread From: Darren Williams @ 2005-01-18 1:47 UTC (permalink / raw) To: Ingo Molnar Cc: Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux Hi Ingo On Mon, 17 Jan 2005, Ingo Molnar wrote: > > * 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 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 <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); > > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > 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 <www.gelato.unsw.edu.au> -------------------------------------------------- ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 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 1 sibling, 1 reply; 48+ messages in thread From: Darren Williams @ 2005-01-18 4:28 UTC (permalink / raw) To: Ingo Molnar, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux On Tue, 18 Jan 2005, Darren Williams wrote: > Hi Ingo > > On Mon, 17 Jan 2005, Ingo Molnar wrote: > > > > > * 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 > 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 <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); > > > > - > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > 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 <www.gelato.unsw.edu.au> > -------------------------------------------------- > - > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -------------------------------------------------- Darren Williams <dsw AT gelato.unsw.edu.au> Gelato@UNSW <www.gelato.unsw.edu.au> -------------------------------------------------- ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-18 4:28 ` Darren Williams @ 2005-01-18 7:08 ` Chris Wedgwood 0 siblings, 0 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-18 7:08 UTC (permalink / raw) To: Ingo Molnar, Andrew Morton, torvalds, benh, linux-kernel, Ia64 Linux 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-18 1:47 ` Darren Williams 2005-01-18 4:28 ` Darren Williams @ 2005-01-19 0:14 ` Peter Chubb 2005-01-19 8:04 ` Ingo Molnar 1 sibling, 1 reply; 48+ messages in thread From: Peter Chubb @ 2005-01-19 0:14 UTC (permalink / raw) To: Tony Luck Cc: Darren Williams, Ingo Molnar, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux 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 <peterc@gelato.unsw.edu.au> 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* ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-19 0:14 ` Peter Chubb @ 2005-01-19 8:04 ` Ingo Molnar 2005-01-19 9:18 ` Peter Chubb 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-19 8:04 UTC (permalink / raw) To: Peter Chubb Cc: Tony Luck, Darren Williams, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux, Christoph Hellwig * Peter Chubb <peterc@gelato.unsw.edu.au> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-19 8:04 ` Ingo Molnar @ 2005-01-19 9:18 ` Peter Chubb 2005-01-19 9:20 ` Ingo Molnar 0 siblings, 1 reply; 48+ messages in thread From: Peter Chubb @ 2005-01-19 9:18 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux, Christoph Hellwig >>>>> "Ingo" == Ingo Molnar <mingo@elte.hu> writes: Ingo> * Peter Chubb <peterc@gelato.unsw.edu.au> 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* ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 2005-01-19 9:18 ` Peter Chubb @ 2005-01-19 9:20 ` Ingo Molnar 2005-01-19 21:43 ` Paul Mackerras 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-19 9:20 UTC (permalink / raw) To: Peter Chubb Cc: Tony Luck, Darren Williams, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux, Christoph Hellwig * Peter Chubb <peterc@gelato.unsw.edu.au> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 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 5:49 ` Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch Grant Grundler 0 siblings, 2 replies; 48+ messages in thread From: Paul Mackerras @ 2005-01-19 21:43 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux, Christoph Hellwig Ingo Molnar writes: > * Peter Chubb <peterc@gelato.unsw.edu.au> 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-19 21:43 ` Paul Mackerras @ 2005-01-20 2:34 ` Chris Wedgwood 2005-01-20 3:01 ` Andrew Morton 2005-01-20 16:18 ` Linus Torvalds 2005-01-20 5:49 ` Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch Grant Grundler 1 sibling, 2 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-20 2:34 UTC (permalink / raw) To: Paul Mackerras Cc: linux-kernel, Ingo Molnar, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, torvalds, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes 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 */ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 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 16:18 ` Linus Torvalds 1 sibling, 1 reply; 48+ messages in thread From: Andrew Morton @ 2005-01-20 3:01 UTC (permalink / raw) To: Chris Wedgwood Cc: paulus, linux-kernel, mingo, peterc, tony.luck, dsw, torvalds, benh, linux-ia64, hch, wli, jbarnes 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 <cw@f00f.org> 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(...)) ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 3:01 ` Andrew Morton @ 2005-01-20 3:18 ` Chris Wedgwood 2005-01-20 3:33 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-20 3:18 UTC (permalink / raw) To: Andrew Morton Cc: paulus, linux-kernel, mingo, peterc, tony.luck, dsw, torvalds, benh, linux-ia64, hch, wli, jbarnes On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote: > ... how about we simply nuke this statement: > > Chris Wedgwood <cw@f00f.org> 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... ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 3:18 ` Chris Wedgwood @ 2005-01-20 3:33 ` Andrew Morton 2005-01-20 8:59 ` Peter Chubb 2005-01-20 16:05 ` [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] Linus Torvalds 2 siblings, 0 replies; 48+ messages in thread From: Andrew Morton @ 2005-01-20 3:33 UTC (permalink / raw) To: Chris Wedgwood Cc: paulus, linux-kernel, mingo, peterc, tony.luck, dsw, torvalds, benh, linux-ia64, hch, wli, jbarnes Chris Wedgwood <cw@f00f.org> wrote: > > On Wed, Jan 19, 2005 at 07:01:04PM -0800, Andrew Morton wrote: > > > ... how about we simply nuke this statement: > > > > Chris Wedgwood <cw@f00f.org> 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? ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 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:05 ` [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] Linus Torvalds 2 siblings, 2 replies; 48+ messages in thread From: Peter Chubb @ 2005-01-20 8:59 UTC (permalink / raw) To: Chris Wedgwood Cc: Andrew Morton, paulus, linux-kernel, mingo, peterc, tony.luck, dsw, torvalds, benh, linux-ia64, hch, wli, jbarnes >>>>> "Chris" == Chris Wedgwood <cw@f00f.org> 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* ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 8:59 ` Peter Chubb @ 2005-01-20 13:04 ` Ingo Molnar 2005-01-20 15:51 ` Linus Torvalds 1 sibling, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 13:04 UTC (permalink / raw) To: Peter Chubb Cc: Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, torvalds, benh, linux-ia64, hch, wli, jbarnes * Peter Chubb <peterc@gelato.unsw.edu.au> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 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 1 sibling, 1 reply; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 15:51 UTC (permalink / raw) To: Peter Chubb Cc: Chris Wedgwood, Andrew Morton, paulus, linux-kernel, mingo, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch 1/3] spinlock fix #1, *_can_lock() primitives 2005-01-20 15:51 ` Linus Torvalds @ 2005-01-20 16:08 ` Ingo Molnar 2005-01-20 16:11 ` [patch 2/3] spinlock fix #2: generalize [spin|rw]lock yielding Ingo Molnar 2005-01-20 16:31 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Linus Torvalds 0 siblings, 2 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:08 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 <mingo@elte.hu> --- 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch 2/3] spinlock fix #2: generalize [spin|rw]lock yielding 2005-01-20 16:08 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar @ 2005-01-20 16:11 ` Ingo Molnar 2005-01-20 16:12 ` [patch 3/3] spinlock fix #3: type-checking spinlock primitives, x86 Ingo Molnar 2005-01-20 16:31 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Linus Torvalds 1 sibling, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:11 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes [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 <mingo@elte.hu> --- 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))); ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch 3/3] spinlock fix #3: type-checking spinlock primitives, x86 2005-01-20 16:11 ` [patch 2/3] spinlock fix #2: generalize [spin|rw]lock yielding Ingo Molnar @ 2005-01-20 16:12 ` Ingo Molnar 2005-01-20 16:14 ` [patch] stricter type-checking rwlock " Ingo Molnar 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:12 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes [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 <mingo@elte.hu> --- 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" \ ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch] stricter type-checking rwlock primitives, x86 2005-01-20 16:12 ` [patch 3/3] spinlock fix #3: type-checking spinlock primitives, x86 Ingo Molnar @ 2005-01-20 16:14 ` Ingo Molnar 2005-01-20 16:16 ` [patch] minor spinlock cleanups Ingo Molnar 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:14 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes [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 <mingo@elte.hu> --- 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) { ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch] minor spinlock cleanups 2005-01-20 16:14 ` [patch] stricter type-checking rwlock " Ingo Molnar @ 2005-01-20 16:16 ` Ingo Molnar 0 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:16 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes [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 <mingo@elte.hu> --- 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 \ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 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:31 ` Linus Torvalds 2005-01-20 16:40 ` Ingo Molnar ` (3 more replies) 1 sibling, 4 replies; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 16:31 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 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 16:44 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar ` (2 subsequent siblings) 3 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:40 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 <mingo@elte.hu> --- 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;});}) ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 2005-01-20 16:40 ` Ingo Molnar @ 2005-01-20 17:48 ` Linus Torvalds 2005-01-20 17:53 ` Ingo Molnar 0 siblings, 1 reply; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 17:48 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 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 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 17:53 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c 2005-01-20 17:53 ` Ingo Molnar @ 2005-01-20 18:22 ` 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 0 siblings, 2 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 18:22 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 <mingo@elte.hu> --- 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); }) ^ permalink raw reply [flat|nested] 48+ messages in thread
* [patch, BK-curr] rename 'lock' to 'slock' in asm-i386/spinlock.h 2005-01-20 18:22 ` [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c Ingo Molnar @ 2005-01-20 18:25 ` Ingo Molnar 2005-01-20 23:45 ` [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c Linus Torvalds 1 sibling, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 18:25 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 <mingo@elte.hu> --- 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"); } /* ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch, BK-curr] nonintrusive spin-polling loop in kernel/spinlock.c 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 ` Linus Torvalds 1 sibling, 0 replies; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 23:45 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 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 16:44 ` Ingo Molnar 2005-01-20 16:59 ` Ingo Molnar 2005-01-20 16:47 ` Ingo Molnar 2005-01-20 16:57 ` Ingo Molnar 3 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:44 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 <mingo@elte.hu> --- 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 2005-01-20 16:44 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar @ 2005-01-20 16:59 ` Ingo Molnar 0 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:59 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Ingo Molnar <mingo@elte.hu> wrote: > * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 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 16:44 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Ingo Molnar @ 2005-01-20 16:47 ` Ingo Molnar 2005-01-20 16:57 ` Ingo Molnar 3 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:47 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 <mingo@elte.hu> --- 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); \ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [patch 1/3] spinlock fix #1, *_can_lock() primitives 2005-01-20 16:31 ` [patch 1/3] spinlock fix #1, *_can_lock() primitives Linus Torvalds ` (2 preceding siblings ...) 2005-01-20 16:47 ` Ingo Molnar @ 2005-01-20 16:57 ` Ingo Molnar 3 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:57 UTC (permalink / raw) To: Linus Torvalds Cc: Peter Chubb, Chris Wedgwood, Andrew Morton, paulus, linux-kernel, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 3:18 ` Chris Wedgwood 2005-01-20 3:33 ` Andrew Morton 2005-01-20 8:59 ` Peter Chubb @ 2005-01-20 16:05 ` Linus Torvalds 2005-01-20 16:20 ` Ingo Molnar 2 siblings, 1 reply; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 16:05 UTC (permalink / raw) To: Chris Wedgwood Cc: Andrew Morton, paulus, linux-kernel, mingo, peterc, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes 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 <cw@f00f.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 16:05 ` [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] Linus Torvalds @ 2005-01-20 16:20 ` Ingo Molnar 0 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:20 UTC (permalink / raw) To: Linus Torvalds Cc: Chris Wedgwood, Andrew Morton, paulus, linux-kernel, peterc, tony.luck, dsw, benh, linux-ia64, hch, wli, jbarnes * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 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 16:18 ` Linus Torvalds 2005-01-20 16:23 ` Ingo Molnar 2005-01-20 16:28 ` Ingo Molnar 1 sibling, 2 replies; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 16:18 UTC (permalink / raw) To: Chris Wedgwood Cc: Paul Mackerras, linux-kernel, Ingo Molnar, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 16:18 ` Linus Torvalds @ 2005-01-20 16:23 ` Ingo Molnar 2005-01-20 17:30 ` Linus Torvalds 2005-01-20 16:28 ` Ingo Molnar 1 sibling, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:23 UTC (permalink / raw) To: Linus Torvalds Cc: Chris Wedgwood, Paul Mackerras, linux-kernel, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes * Linus Torvalds <torvalds@osdl.org> 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) { ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 16:23 ` Ingo Molnar @ 2005-01-20 17:30 ` Linus Torvalds 2005-01-20 17:38 ` Ingo Molnar 0 siblings, 1 reply; 48+ messages in thread From: Linus Torvalds @ 2005-01-20 17:30 UTC (permalink / raw) To: Ingo Molnar Cc: Chris Wedgwood, Paul Mackerras, linux-kernel, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 17:30 ` Linus Torvalds @ 2005-01-20 17:38 ` Ingo Molnar 0 siblings, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 17:38 UTC (permalink / raw) To: Linus Torvalds Cc: Chris Wedgwood, Paul Mackerras, linux-kernel, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH RFC] 'spinlock/rwlock fixes' V3 [1/1] 2005-01-20 16:18 ` Linus Torvalds 2005-01-20 16:23 ` Ingo Molnar @ 2005-01-20 16:28 ` Ingo Molnar 1 sibling, 0 replies; 48+ messages in thread From: Ingo Molnar @ 2005-01-20 16:28 UTC (permalink / raw) To: Linus Torvalds Cc: Chris Wedgwood, Paul Mackerras, linux-kernel, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Benjamin Herrenschmidt, Ia64 Linux, Christoph Hellwig, William Lee Irwin III, Jesse Barnes * Linus Torvalds <torvalds@osdl.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: Horrible regression with -CURRENT from "Don't busy-lock-loop in preemptable spinlocks" patch 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 5:49 ` Grant Grundler 1 sibling, 0 replies; 48+ messages in thread From: Grant Grundler @ 2005-01-20 5:49 UTC (permalink / raw) To: Paul Mackerras Cc: Ingo Molnar, Peter Chubb, Tony Luck, Darren Williams, Andrew Morton, Chris Wedgwood, torvalds, benh, linux-kernel, Ia64 Linux, Christoph Hellwig 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() 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:38 ` Chris Wedgwood 2005-01-17 14:40 ` Ingo Molnar 1 sibling, 1 reply; 48+ messages in thread From: Chris Wedgwood @ 2005-01-17 7:38 UTC (permalink / raw) To: Andrew Morton, Linus Torvalds, Ingo Molnar Cc: Benjamin Herrenschmidt, LKML, Paul Mackerras 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 <cw@f00f.org> ===== 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) \ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() 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 0 siblings, 1 reply; 48+ messages in thread From: Ingo Molnar @ 2005-01-17 14:40 UTC (permalink / raw) To: Chris Wedgwood Cc: Andrew Morton, Linus Torvalds, Benjamin Herrenschmidt, LKML, Paul Mackerras * Chris Wedgwood <cw@f00f.org> 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH] __get_cpu_var should use __smp_processor_id() not smp_processor_id() 2005-01-17 14:40 ` Ingo Molnar @ 2005-01-17 18:53 ` Chris Wedgwood 0 siblings, 0 replies; 48+ messages in thread From: Chris Wedgwood @ 2005-01-17 18:53 UTC (permalink / raw) To: Ingo Molnar Cc: Andrew Morton, Linus Torvalds, Benjamin Herrenschmidt, LKML, Paul Mackerras, Christoph Hellwig 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). ^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2005-01-20 23:46 UTC | newest] Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).