Re: Serious problem with ticket spinlocks on ia64

[Petr Tesarik <ptesarik@suse.cz>]

Hi all,

On Monday 06 of September 2010 16:47:01 Petr Tesarik wrote:
>[...]
> To sum it up:
>
>[...]
> 2. So far, the problem has been observed only after the spinlock value
> changes to zero.

I thought about this point for a while, and then I decided to test this with 
brute force. Why not simply skip the zero? If I shift the head position to 
the right within the lock, I can iterate over odd numbers only. 
Unfortunately, the ia64 platform does not have a fetchadd4 variant with an 
increment of 2, so I had to reduce the size of the head/tail to 14 bits, but 
that's still sufficient for all today's machines. Anyway, I do NOT propose 
this as a solution, rather as a proof of concept.

Anyway, after applying the following patch, the test case provided by Hedi has 
been running for a few hours already. Now, I'm confident this is a hardware 
bug.

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

diff --git a/arch/ia64/include/asm/spinlock.h 
b/arch/ia64/include/asm/spinlock.h
index f0816ac..01be28e 100644
--- a/arch/ia64/include/asm/spinlock.h
+++ b/arch/ia64/include/asm/spinlock.h
@@ -26,23 +26,28 @@
  * The pad bits in the middle are used to prevent the next_ticket number
  * overflowing into the now_serving number.
  *
- *   31             17  16    15  14                    0
+ *   31             18  17    16  15               2  1 0
  *  +----------------------------------------------------+
- *  |  now_serving     | padding |   next_ticket         |
+ *  |  now_serving     | padding |   next_ticket     | - |
  *  +----------------------------------------------------+
  */
 
-#define TICKET_SHIFT	17
-#define TICKET_BITS	15
+#define TICKET_HSHIFT	2
+#define TICKET_TSHIFT	18
+#define TICKET_BITS	14
 #define	TICKET_MASK	((1 << TICKET_BITS) - 1)
 
+#define __ticket_spin_is_unlocked(ticket, serve) \
+	(!((((serve) >> (TICKET_TSHIFT - TICKET_HSHIFT)) ^ (ticket)) \
+	   & (TICKET_MASK << TICKET_HSHIFT)))
+
 static __always_inline void __ticket_spin_lock(arch_spinlock_t *lock)
 {
 	int	*p = (int *)&lock->lock, ticket, serve;
 
-	ticket = ia64_fetchadd(1, p, acq);
+	ticket = ia64_fetchadd(1 << TICKET_HSHIFT, p, acq);
 
-	if (!(((ticket >> TICKET_SHIFT) ^ ticket) & TICKET_MASK))
+	if (__ticket_spin_is_unlocked(ticket, ticket))
 		return;
 
 	ia64_invala();
@@ -50,7 +55,7 @@ static __always_inline void 
__ticket_spin_lock(arch_spinlock_t *lock)
 	for (;;) {
 		asm volatile ("ld4.c.nc %0=[%1]" : "=r"(serve) : "r"(p) : "memory");
 
-		if (!(((serve >> TICKET_SHIFT) ^ ticket) & TICKET_MASK))
+		if (__ticket_spin_is_unlocked(ticket, serve))
 			return;
 		cpu_relax();
 	}
@@ -60,8 +65,8 @@ static __always_inline int 
__ticket_spin_trylock(arch_spinlock_t *lock)
 {
 	int tmp = ACCESS_ONCE(lock->lock);
 
-	if (!(((tmp >> TICKET_SHIFT) ^ tmp) & TICKET_MASK))
-		return ia64_cmpxchg(acq, &lock->lock, tmp, tmp + 1, sizeof (tmp)) = tmp;
+	if (__ticket_spin_is_unlocked(tmp, tmp))
+		return ia64_cmpxchg(acq, &lock->lock, tmp, tmp + (1 << TICKET_HSHIFT), 
sizeof (tmp)) = tmp;
 	return 0;
 }
 
@@ -70,7 +75,7 @@ static __always_inline void 
__ticket_spin_unlock(arch_spinlock_t *lock)
 	unsigned short	*p = (unsigned short *)&lock->lock + 1, tmp;
 
 	asm volatile ("ld2.bias %0=[%1]" : "=r"(tmp) : "r"(p));
-	ACCESS_ONCE(*p) = (tmp + 2) & ~1;
+	ACCESS_ONCE(*p) = (tmp + (1 << (TICKET_TSHIFT - 16))) & ~1;
 }
 
 static __always_inline void __ticket_spin_unlock_wait(arch_spinlock_t *lock)
@@ -81,7 +86,7 @@ static __always_inline void 
__ticket_spin_unlock_wait(arch_spinlock_t *lock)
 
 	for (;;) {
 		asm volatile ("ld4.c.nc %0=[%1]" : "=r"(ticket) : "r"(p) : "memory");
-		if (!(((ticket >> TICKET_SHIFT) ^ ticket) & TICKET_MASK))
+		if (__ticket_spin_is_unlocked(ticket, ticket))
 			return;
 		cpu_relax();
 	}
@@ -91,14 +96,14 @@ static inline int __ticket_spin_is_locked(arch_spinlock_t 
*lock)
 {
 	long tmp = ACCESS_ONCE(lock->lock);
 
-	return !!(((tmp >> TICKET_SHIFT) ^ tmp) & TICKET_MASK);
+	return !__ticket_spin_is_unlocked(tmp, tmp);
 }
 
 static inline int __ticket_spin_is_contended(arch_spinlock_t *lock)
 {
 	long tmp = ACCESS_ONCE(lock->lock);
 
-	return ((tmp - (tmp >> TICKET_SHIFT)) & TICKET_MASK) > 1;
+	return (((tmp >> TICKET_HSHIFT) - (tmp >> TICKET_TSHIFT)) & TICKET_MASK) > 
1;
 }
 
 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
diff --git a/arch/ia64/include/asm/spinlock_types.h 
b/arch/ia64/include/asm/spinlock_types.h
index e2b42a5..4275cd3 100644
--- a/arch/ia64/include/asm/spinlock_types.h
+++ b/arch/ia64/include/asm/spinlock_types.h
@@ -9,7 +9,7 @@ typedef struct {
 	volatile unsigned int lock;
 } arch_spinlock_t;
 
-#define __ARCH_SPIN_LOCK_UNLOCKED	{ 0 }
+#define __ARCH_SPIN_LOCK_UNLOCKED	{ 1 }
 
 typedef struct {
 	volatile unsigned int read_counter	: 31;