2004-06-07 08:59:07

by Zoltan Menyhart

[permalink] [raw]
Subject: Re: Who owns those locks ?

--- 2.6.5.ref/include/asm-ia64/spinlock.h Sun Apr 4 05:36:17 2004
+++ 2.6.5.new/include/asm-ia64/spinlock.h Wed May 12 13:17:50 2004
@@ -45,7 +45,8 @@
asm volatile ("{\n\t"
" mov ar.ccv = r0\n\t"
" mov r28 = ip\n\t"
- " mov r30 = 1;;\n\t"
+ /* " mov r30 = 1;;\n\t" */
+ " shr.u r30 = r13, 12;;\n\t" /* Current task pointer */
"}\n\t"
"cmpxchg4.acq r30 = [%1], r30, ar.ccv\n\t"
"movl r29 = ia64_spinlock_contention_pre3_4;;\n\t"
@@ -57,7 +58,8 @@
asm volatile ("{\n\t"
" mov ar.ccv = r0\n\t"
" mov r28 = ip\n\t"
- " mov r30 = 1;;\n\t"
+ /* " mov r30 = 1;;\n\t" */
+ " shr.u r30 = r13, 12;;\n\t" /* Current task pointer */
"}\n\t"
"cmpxchg4.acq r30 = [%1], r30, ar.ccv;;\n\t"
"cmp4.ne p14, p0 = r30, r0\n"
@@ -68,7 +70,8 @@
# ifdef CONFIG_ITANIUM
/* don't use brl on Itanium... */
/* mis-declare, so we get the entry-point, not it's function descriptor: */
- asm volatile ("mov r30 = 1\n\t"
+ asm volatile (/* " mov r30 = 1;;\n\t" */
+ " shr.u r30 = r13, 12;;\n\t" /* Current task pointer */
"mov ar.ccv = r0;;\n\t"
"cmpxchg4.acq r30 = [%0], r30, ar.ccv\n\t"
"movl r29 = ia64_spinlock_contention;;\n\t"
@@ -77,7 +80,8 @@
"(p14) br.call.spnt.many b6 = b6"
: "=r"(ptr) : "r"(ptr) : IA64_SPINLOCK_CLOBBERS);
# else
- asm volatile ("mov r30 = 1\n\t"
+ asm volatile (/* " mov r30 = 1;;\n\t" */
+ " shr.u r30 = r13, 12;;\n\t" /* Current task pointer */
"mov ar.ccv = r0;;\n\t"
"cmpxchg4.acq r30 = [%0], r30, ar.ccv;;\n\t"
"cmp4.ne p14, p0 = r30, r0\n\t"
@@ -89,14 +93,17 @@
#else /* !ASM_SUPPORTED */
# define _raw_spin_lock(x) \
do { \
- __u32 *ia64_spinlock_ptr = (__u32 *) (x); \
- __u64 ia64_spinlock_val; \
- ia64_spinlock_val = ia64_cmpxchg4_acq(ia64_spinlock_ptr, 1, 0); \
+ __u32 *ia64_spinlock_ptr = (__u32 *) (x); \
+ __u64 ia64_spinlock_val; \
+ __u32 new_spinlock_val = (__u32)((__u64) current >> 12); \
+ \
+ ia64_spinlock_val = ia64_cmpxchg4_acq(ia64_spinlock_ptr, new_spinlock_val, 0); \
if (unlikely(ia64_spinlock_val)) { \
do { \
while (*ia64_spinlock_ptr) \
ia64_barrier(); \
- ia64_spinlock_val = ia64_cmpxchg4_acq(ia64_spinlock_ptr, 1, 0); \
+ ia64_spinlock_val = ia64_cmpxchg4_acq(ia64_spinlock_ptr, \
+ new_spinlock_val, 0); \
} while (ia64_spinlock_val); \
} \
} while (0)
@@ -104,7 +111,7 @@

#define spin_is_locked(x) ((x)->lock != 0)
#define _raw_spin_unlock(x) do { barrier(); ((spinlock_t *) x)->lock = 0; } while (0)
-#define _raw_spin_trylock(x) (cmpxchg_acq(&(x)->lock, 0, 1) == 0)
+#define _raw_spin_trylock(x) (cmpxchg_acq(&(x)->lock, 0, (__u32)((__u64) current >> 12)) == 0)
#define spin_unlock_wait(x) do { barrier(); } while ((x)->lock)

typedef struct {
--- 2.6.5.ref/arch/ia64/kernel/head.S Wed Apr 21 16:18:26 2004
+++ 2.6.5.new/arch/ia64/kernel/head.S Wed May 12 16:31:33 2004
@@ -917,7 +917,8 @@
ld4 r30=[r31] // don't use ld4.bias; if it's contended, we won't write the word
;;
cmp4.ne p14,p0=r30,r0
- mov r30 = 1
+// mov r30 = 1
+ shr.u r30 = r13, 12 // Current task pointer
(p14) br.cond.sptk.few .wait
;;
cmpxchg4.acq r30=[r31], r30, ar.ccv


Attachments:
l (3.28 kB)

2004-06-07 15:07:10

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: Who owns those locks ?

On Monday 07 June 2004 2:58 am, Zoltan Menyhart wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 12 May 2004 3:56 am, Zoltan Menyhart wrote:
> > > Got a dead lock ?
> > > No idea how you got there ?
> > >
> > > Why don't you put the ID of the owner of the lock in the lock word ?
> > > Here is your patch for IA-64.
> > > Doesn't cost any additional instruction, you can have it in your
> > > "production" kernel, too.
> >
> > Whatever happened with this patch? I really like the idea, but
> > it seems like it died on the vine. Maybe it's time to clean it
> > up, pull all the bits together, and repost it.
>
> Here you are.
> (I'm still playing with 2.6.5)

There are a couple issues I was thinking of when
I wrote "clean it up, pull the bits together...":

1) Tony Luck's question about what happens when
"shr.u r30 = r13, 12" yields zero in the 32-bit
lock value. I'm not the 2.6 maintainer, but I'd
sure like to see some solution for this. It would
be a nightmare to debug a system where one random
task didn't release locks correctly. Since other
arches use a trick like this, I'm hoping they've
figured out something we can copy (I haven't looked).

2) A nit-pick, but for things like this:

- " mov r30 = 1;;\n\t"
+ /* " mov r30 = 1;;\n\t" */
+ " shr.u r30 = r13, 12;;\n\t" /* Current task pointer */

just remove the old line, don't comment it out.

Thanks for collecting the bits. I looked at it last week, got
derailed trying to figure out how the pre-gcc-3.3 lock contention
code worked (a comment there would have been helpful), and got
distracted.