Skip to contentSkip to conversation

Who owns those locks ?

PATCH Zoltan Menyhart 3 participants 8 messages May 12, 2004 – Jun 8, 2004

lore
Share & tools
lore

Zoltan Menyhart

author
#0 lore raw

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.

The current task pointers are identity mapped memory addresses. I shift them to the right by 12 bits (these bits are always 0-s). In that way, addresses up to 16 Tbytes can fit into the lock word.

Interrupt handlers use the current task pointers as IDs, too. An interrupt handler has to free all the locks it has taken, therefore using the same ID as the task pre-empted uses, is not confusing. Locks which are taken with / without interrupt disabling form two distinct sets.
If you are back into the pre-empted task and should there is a "left over" lock with the ID of the task => you've got a hint ;-)

In debug mode, you can check if the lock is yours before setting it free:

#define spin_is_mine(x) ((x)->lock == (__u32)((__u64) current >> 12))

Good luck.

Zoltán Menyhárt

include/asm-ia64/spinlock.h +15 -8
@@ -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)
#1 lore raw
Quoted 5 lines

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.

We already do this on ppc64.

Anton

#2 lore raw
In reply to Zoltan Menyhart
Quoted 7 lines · Zoltan Menyhart

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.

author
#3 lore raw
Quoted 5 lines · Bjorn Helgaas
Quoted 3 lines · Zoltan Menyhart

Got a dead lock ?
No idea how you got there ?

Quoted 8 lines
Quoted 4 lines

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)

Zoltán Menyhárt

include/asm-ia64/spinlock.h +16 -9
@@ -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 {
arch/ia64/kernel/head.S +2 -1
@@ -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
#4 lore raw
Quoted 10 lines · Zoltan Menyhart
Quoted 9 lines · Bjorn Helgaas
Quoted 7 lines · Zoltan Menyhart

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.

Quoted 6 lines
Quoted 3 lines

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.

author
#5 lore raw
Quoted 4 lines · Bjorn Helgaas

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

Quoted 8 lines

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).

Sure, I did not want to make an error like saying:
"640 K ought to be enough for everyone".

I'm afraid, there is no perfect solution.

- We do not want to change the lock size to 64 bits, do we ? -- Couple of new alignment problems.

- You keep my code, it is correct for a memory size up to 16 Tbytes.

- You shift by PAGE_SHIFT, rather than by 12 (using page size of 16 Kbytes) => up to 64 Tbytes.
-- Not that much human readable lock values.

- You move to PAGE_SIZE = 64 K, you get human readable lock values up to 256 Tbytes.

- You could store the "PID | miraculous bit" (to avoid PID = 0 problem).
-- Somewhat longer code.

I expect the main stream IA64 kernel to move to PAGE_SIZE = 64 K by the time there will be machines with more than 16 Tbytes of memory (as the processors have got just a very limited number of translation look aside buffer entries, and the ever growing application / memory sizes result in higher TLB miss rate unless the page size increases).

Regards,

Zoltán

#6 lore raw
Quoted 1 line · Zoltan Menyhart

- You keep my code, it is correct for a memory size up to 16 Tbytes.

Many if not most large machines have sparse address spaces, so you may have memory at an address that will cause a problem even if the actual amount of memory is much smaller.

The main point is that I wouldn't want a time bomb that will silently fail when somebody happens to boot on such a machine. Whether that's avoided by a "miraculous" bit, throwing away problem pages at boot-time, avoiding task allocation at specific addresses, etc., is secondary.

author
#7 lore raw
Quoted 8 lines · Bjorn Helgaas
Quoted 1 line · Zoltan Menyhart

- You keep my code, it is correct for a memory size up to 16 Tbytes.

Many if not most large machines have sparse address spaces, so you may have memory at an address that will cause a problem even if the actual amount of memory is much smaller.

Quoted 5 lines

The main point is that I wouldn't want a time bomb that will silently fail when somebody happens to boot on such a machine. Whether that's avoided by a "miraculous" bit, throwing away problem pages at boot-time, avoiding task allocation at specific addresses, etc., is secondary.

I see.
No use to make it too much complicated.
There is always the option CONFIG_DEBUG_SPINLOCK.

On our no-more-than-512-Gbyte-machine, this small stuff "saved my life" twice.
I just wanted to share it...

Regards,

Zoltán