Message-ID: <55489D9E.4010003@linux.vnet.ibm.com>
Date: Tue, 05 May 2015 16:08:22 +0530
From: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
Organization: IBM
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7
MIME-Version: 1.0
To: Tahsin Erdogan <tahsin@google.com>
CC: Peter Zijlstra <peterz@infradead.org>, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, x86@kernel.org, Waiman.Long@hp.com,
        borntraeger@de.ibm.com, oleg@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/spinlocks: Fix regression in spinlock contention
 detection
References: <1430799331-20445-1-git-send-email-tahsin@google.com> <20150505091714.GF21418@twins.programming.kicks-ass.net>
In-Reply-To: <20150505091714.GF21418@twins.programming.kicks-ass.net>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2453
Lines: 70

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;
}


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