x86/ticketlock: convert spin loop to C
in [Kernel]
|
Prev: [PATCH 1/2] KVM: SVM: Check for nested vmrun intercept before emulating vmrun
Next: KVM: SVM: Check for nested vmrun intercept before emulating vmrun
From: Peter Zijlstra on 2 Aug 2010 11:10 On Fri, 2010-07-16 at 18:03 -0700, Jeremy Fitzhardinge wrote: > + register union { > + struct __raw_tickets tickets; > + unsigned short slock; > + } inc = { .slock = 1 << TICKET_SHIFT }; register arch_spinlock_t inc = { .tickets = { .head = 1, .tail = 0 } }; >From a quick look you can basically replace all TICKET_SHIFT usage (1 << TICKET_SHIFT) with such a constant. [ Also, does gcc really listen to the register hint these days? ] > + asm volatile (LOCK_PREFIX "xaddw %w0, %1\n" > + : "+Q" (inc), "+m" (lock->slock) : : "memory", "cc"); "+Q" (inc->slock) > + for (;;) { > + if (inc.tickets.head == inc.tickets.tail) > + return; > + cpu_relax(); > + inc.tickets.head = ACCESS_ONCE(lock->tickets.head); > + } > + barrier(); /* make sure nothing creeps before the lock is taken */ > } How will it ever get to that barrier() ? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jeremy Fitzhardinge on 2 Aug 2010 11:20 On 08/02/2010 08:07 AM, Peter Zijlstra wrote: > On Fri, 2010-07-16 at 18:03 -0700, Jeremy Fitzhardinge wrote: >> + register union { >> + struct __raw_tickets tickets; >> + unsigned short slock; >> + } inc = { .slock = 1<< TICKET_SHIFT }; > register arch_spinlock_t inc = { .tickets = { .head = 1, .tail = 0 } }; > > > From a quick look you can basically replace all TICKET_SHIFT usage (1<< > TICKET_SHIFT) with such a constant. Mostly. In the later patch to convert trylock in to C, you need it to construct an argument for cmpxchg (which can only take a scalar, even if it does have a struct packed into it). > [ Also, does gcc really listen to the register hint these days? ] It doesn't make much different in this case. I think the only real effect is that its illegal to take the address of a register variable. >> + asm volatile (LOCK_PREFIX "xaddw %w0, %1\n" >> + : "+Q" (inc), "+m" (lock->slock) : : "memory", "cc"); > "+Q" (inc->slock) > >> + for (;;) { >> + if (inc.tickets.head == inc.tickets.tail) >> + return; >> + cpu_relax(); >> + inc.tickets.head = ACCESS_ONCE(lock->tickets.head); >> + } >> + barrier(); /* make sure nothing creeps before the lock is taken */ >> } > How will it ever get to that barrier() ? The compiler treats this as being: for (;;) { if (inc.tickets.head == inc.tickets.tail) goto out; ... } out: barrier(); } (Which would probably be a reasonable way to clarify the code.) Without the barrier there's a risk of locked-region code being scheduled before the for(;;) loop. J -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jan Beulich on 6 Aug 2010 08:50 >>> On 02.08.10 at 17:17, Jeremy Fitzhardinge <jeremy(a)goop.org> wrote: > On 08/02/2010 08:07 AM, Peter Zijlstra wrote: >> On Fri, 2010-07-16 at 18:03 -0700, Jeremy Fitzhardinge wrote: >>> + asm volatile (LOCK_PREFIX "xaddw %w0, %1\n" >>> + : "+Q" (inc), "+m" (lock->slock) : : "memory", "cc"); >> "+Q" (inc->slock) >> >>> + for (;;) { >>> + if (inc.tickets.head == inc.tickets.tail) >>> + return; >>> + cpu_relax(); >>> + inc.tickets.head = ACCESS_ONCE(lock->tickets.head); >>> + } >>> + barrier(); /* make sure nothing creeps before the lock > is taken */ >>> } >> How will it ever get to that barrier() ? > > The compiler treats this as being: You certainly mean "the compiler currently treats this as being:" - I don't think there's a guarantee it'll always be doing so. > for (;;) { > if (inc.tickets.head == inc.tickets.tail) > goto out; > ... > } > out: barrier(); > } > > (Which would probably be a reasonable way to clarify the code.) I therefore think it needs to be written this way. Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jeremy Fitzhardinge on 6 Aug 2010 11:00 On 08/06/2010 05:43 AM, Jan Beulich wrote: > You certainly mean "the compiler currently treats this as being:" - I > don't think there's a guarantee it'll always be doing so. > >> for (;;) { >> if (inc.tickets.head == inc.tickets.tail) >> goto out; >> ... >> } >> out: barrier(); >> } >> >> (Which would probably be a reasonable way to clarify the code.) > I therefore think it needs to be written this way. Agreed. J -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 6 Aug 2010 16:30
On 08/06/2010 07:53 AM, Jeremy Fitzhardinge wrote: > On 08/06/2010 05:43 AM, Jan Beulich wrote: >> You certainly mean "the compiler currently treats this as being:" - I >> don't think there's a guarantee it'll always be doing so. >> >>> for (;;) { >>> if (inc.tickets.head == inc.tickets.tail) >>> goto out; >>> ... >>> } >>> out: barrier(); >>> } >>> >>> (Which would probably be a reasonable way to clarify the code.) >> I therefore think it needs to be written this way. > > Agreed. > A call/return to an actual out-of-line function is a barrier (and will always be a barrier, as it is the fundamental ABI sequence points), but to an inline function it is not. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |