Re: [PATCH 1/3] powerpc/spinlocks: Refactor SHARED_PROCESSOR

[Thiago Jung Bauermann <bauerman@linux.ibm.com>]

Christopher M Riedl <cmr@informatik.wtf> writes:

>> On July 30, 2019 at 4:31 PM Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>>
>>
>>
>> Christopher M. Riedl <cmr@informatik.wtf> writes:
>>
>> > Determining if a processor is in shared processor mode is not a constant
>> > so don't hide it behind a #define.
>> >
>> > Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
>> > ---
>> >  arch/powerpc/include/asm/spinlock.h | 21 +++++++++++++++------
>> >  1 file changed, 15 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
>> > index a47f827bc5f1..8631b0b4e109 100644
>> > --- a/arch/powerpc/include/asm/spinlock.h
>> > +++ b/arch/powerpc/include/asm/spinlock.h
>> > @@ -101,15 +101,24 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
>> >
>> >  #if defined(CONFIG_PPC_SPLPAR)
>> >  /* We only yield to the hypervisor if we are in shared processor mode */
>> > -#define SHARED_PROCESSOR (lppaca_shared_proc(local_paca->lppaca_ptr))
>> >  extern void __spin_yield(arch_spinlock_t *lock);
>> >  extern void __rw_yield(arch_rwlock_t *lock);
>> >  #else /* SPLPAR */
>> >  #define __spin_yield(x)	barrier()
>> >  #define __rw_yield(x)	barrier()
>> > -#define SHARED_PROCESSOR	0
>> >  #endif
>> >
>> > +static inline bool is_shared_processor(void)
>> > +{
>> > +/* Only server processors have an lppaca struct */
>> > +#ifdef CONFIG_PPC_BOOK3S
>> > +	return (IS_ENABLED(CONFIG_PPC_SPLPAR) &&
>> > +		lppaca_shared_proc(local_paca->lppaca_ptr));
>> > +#else
>> > +	return false;
>> > +#endif
>> > +}
>> > +
>>
>> CONFIG_PPC_SPLPAR depends on CONFIG_PPC_PSERIES, which depends on
>> CONFIG_PPC_BOOK3S so the #ifdef above is unnecessary:
>>
>> if CONFIG_PPC_BOOK3S is unset then CONFIG_PPC_SPLPAR will be unset as
>> well and the return expression should short-circuit to false.
>>
>
> Agreed, but the #ifdef is necessary to compile platforms which include
> this header but do not implement lppaca_shared_proc(...) and friends.
> I can reword the comment if that helps.

Ah, indeed. Yes, if you could mention that in the commit I think it
would help. These #ifdefs are becoming démodé so it's good to know why
they're there.

Another alternative is to provide a dummy lppaca_shared_proc() which
always returns false when CONFIG_PPC_BOOK3S isn't set (just mentioning
it, I don't have a preference).

--
Thiago Jung Bauermann
IBM Linux Technology Center