Re: [PATCH] x86/spinlocks: Fix regression in spinlock contention detection

From: Raghavendra K T
Date: Tue May 05 2015 - 06:34:10 EST


On 05/05/2015 02:47 PM, Peter Zijlstra wrote:
On Mon, May 04, 2015 at 09:15:31PM -0700, Tahsin Erdogan wrote:
A spinlock is regarded as contended when there is at least one waiter.
Currently, the code that checks whether there are any waiters rely on
tail value being greater than head. However, this is not true if tail
reaches the max value and wraps back to zero, so arch_spin_is_contended()
incorrectly returns 0 (not contended) when tail is smaller than head.

The original code (before regression) handled this case by casting the
(tail - head) to an unsigned value. This change simply restores that
behavior.

Fixes: d6abfdb20223 ("x86/spinlocks/paravirt: Fix memory corruption on
unlock")
Signed-off-by: Tahsin Erdogan <tahsin@xxxxxxxxxx>
---
arch/x86/include/asm/spinlock.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
index cf87de3..64b6117 100644
--- a/arch/x86/include/asm/spinlock.h
+++ b/arch/x86/include/asm/spinlock.h
@@ -169,7 +169,7 @@ static inline int arch_spin_is_contended(arch_spinlock_t *lock)
struct __raw_tickets tmp = READ_ONCE(lock->tickets);

tmp.head &= ~TICKET_SLOWPATH_FLAG;
- return (tmp.tail - tmp.head) > TICKET_LOCK_INC;
+ return (__ticket_t)(tmp.tail - tmp.head) > TICKET_LOCK_INC;

I'm not seeing it, everything in that expression is of __ticket_t type
(tail, head and TICKET_LOCK_INC), nothing should cause it to be cast to
another type due to conversion rules.

Or does - always cast to a signed type? Lemme go grab the C rules again.

I'm not seeing it.. Please explain better, iow. your changelog fails to
properly explain the problem.


Same from me here.

Did you really see a regression?

I am not seeing two unsigned value subtraction resulting in a negative
value.

================
#include <stdio.h>

#define LOCK_INC 2

int main()
{
unsigned int head = 32700, tail=2;

if ((tail - head) > LOCK_INC)
printf(" tail - head > LOCK_INC \n");
else
printf(" tail - head < LOCK_INC \n");

return 0;
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/