[PATCH v2 3/3] powerpc/spinlocks: Fix oops in shared-processor spinlocks

Michael Ellerman mpe at ellerman.id.au
Tue Aug 6 22:14:27 AEST 2019


Christopher M Riedl <cmr at informatik.wtf> writes:
>> On August 2, 2019 at 6:38 AM Michael Ellerman <mpe at ellerman.id.au> wrote:
>> "Christopher M. Riedl" <cmr at informatik.wtf> writes:
>> > diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
>> > index 0a8270183770..6aed8a83b180 100644
>> > --- a/arch/powerpc/include/asm/spinlock.h
>> > +++ b/arch/powerpc/include/asm/spinlock.h
>> > @@ -124,6 +122,22 @@ static inline bool is_shared_processor(void)
>> >  #endif
>> >  }
>> >  
>> > +static inline void spin_yield(arch_spinlock_t *lock)
>> > +{
>> > +	if (is_shared_processor())
>> > +		splpar_spin_yield(lock);
>> > +	else
>> > +		barrier();
>> > +}
>> ...
>> >  static inline void arch_spin_lock(arch_spinlock_t *lock)
>> >  {
>> >  	while (1) {
>> > @@ -132,7 +146,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>> >  		do {
>> >  			HMT_low();
>> >  			if (is_shared_processor())
>> > -				__spin_yield(lock);
>> > +				spin_yield(lock);
>> 
>> This leaves us with a double test of is_shared_processor() doesn't it?
>
> Yep, and that's no good. Hmm, executing the barrier() in the non-shared-processor
> case probably hurts performance here?

It's only a "compiler barrier", so it shouldn't generate any code.

But it does have the effect of telling the compiler it can't optimise
across that barrier, which can be important.

In those spin loops all we're doing is checking lock->slock which is
already marked volatile in the definition of arch_spinlock_t, so the
extra barrier shouldn't really make any difference.

But still the current code doesn't have a barrier() there, so we should
make sure we don't introduce one as part of this refactor.

So I think you just want to change the call to spin_yield() above to
splpar_spin_yield(), which avoids the double check, and also avoids the
barrier() in the SPLPAR=n case.

And then arch_spin_relax() calls spin_yield() etc.

cheers


More information about the Linuxppc-dev mailing list