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

[Raghavendra K T]

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/