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

[Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>]

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@google.com>
>> ---
>>   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;
}