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: Jeremy Fitzhardinge on 6 Aug 2010 16:40 On 08/06/2010 01:17 PM, H. Peter Anvin wrote: > 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. Yes. So the goto explicitly puts the barrier into the control flow which should stop the compiler from doing anything unexpected. 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 17:20 On 08/06/2010 01:33 PM, Jeremy Fitzhardinge wrote: > On 08/06/2010 01:17 PM, H. Peter Anvin wrote: >> 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. > > Yes. So the goto explicitly puts the barrier into the control flow which > should stop the compiler from doing anything unexpected. > In this particular case, though, I would somewhat expect the more conventional: while (inc.tickets.head != inc.tickets.tail) { cpu_relax(); inc.tickets.head = ACCESS_ONCE(lock->tickets_head); } -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/
From: Jeremy Fitzhardinge on 6 Aug 2010 18:40
On 08/06/2010 02:09 PM, H. Peter Anvin wrote: > On 08/06/2010 01:33 PM, Jeremy Fitzhardinge wrote: >> On 08/06/2010 01:17 PM, H. Peter Anvin wrote: >>> 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. >> >> Yes. So the goto explicitly puts the barrier into the control flow which >> should stop the compiler from doing anything unexpected. >> > > In this particular case, though, I would somewhat expect the more > conventional: > > while (inc.tickets.head != inc.tickets.tail) { > cpu_relax(); > inc.tickets.head = ACCESS_ONCE(lock->tickets_head); > } Yes, that makes sense for the plain spinlock version. But the full code, including the pv-ticketlock spin timeout, ends up being: static __always_inline void arch_spin_lock(struct arch_spinlock *lock) { register struct __raw_tickets inc; inc = __ticket_spin_claim(lock); for (;;) { unsigned count = SPIN_THRESHOLD; do { if (inc.head == inc.tail) goto out; cpu_relax(); inc.head = ACCESS_ONCE(lock->tickets.head); } while (--count); __ticket_lock_spinning(lock, inc.tail); } out: barrier(); /* make sure nothing creeps before the lock is taken */ } So the goto form is closer to the final form. If it weren't for this, I'd also prefer the while() form. (If you config PARAVIRT_SPINLOCKS=n, then __ticket_lock_spinning() becomes an empty inline, which causes gcc to collapse the whole thing into a simple infinite loop (ie, it eliminates "count" and the inner 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/ |