Excerpts from Waiman Long's message of July 6, 2020 5:00 am:
> On 7/3/20 3:35 AM, Nicholas Piggin wrote:
>> Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
>> ---
>> arch/powerpc/include/asm/paravirt.h | 28 ++++++++++
>> arch/powerpc/include/asm/qspinlock.h | 55 +++++++++++++++++++
>> arch/powerpc/include/asm/qspinlock_paravirt.h | 5 ++
>> arch/powerpc/platforms/pseries/Kconfig | 5 ++
>> arch/powerpc/platforms/pseries/setup.c | 6 +-
>> include/asm-generic/qspinlock.h | 2 +
>> 6 files changed, 100 insertions(+), 1 deletion(-)
>> create mode 100644 arch/powerpc/include/asm/qspinlock_paravirt.h
>>
>> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
>> index 7a8546660a63..f2d51f929cf5 100644
>> --- a/arch/powerpc/include/asm/paravirt.h
>> +++ b/arch/powerpc/include/asm/paravirt.h
>> @@ -29,6 +29,16 @@ static inline void yield_to_preempted(int cpu, u32 yield_count)
>> {
>> plpar_hcall_norets(H_CONFER, get_hard_smp_processor_id(cpu), yield_count);
>> }
>> +
>> +static inline void prod_cpu(int cpu)
>> +{
>> + plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
>> +}
>> +
>> +static inline void yield_to_any(void)
>> +{
>> + plpar_hcall_norets(H_CONFER, -1, 0);
>> +}
>> #else
>> static inline bool is_shared_processor(void)
>> {
>> @@ -45,6 +55,19 @@ static inline void yield_to_preempted(int cpu, u32 yield_count)
>> {
>> ___bad_yield_to_preempted(); /* This would be a bug */
>> }
>> +
>> +extern void ___bad_yield_to_any(void);
>> +static inline void yield_to_any(void)
>> +{
>> + ___bad_yield_to_any(); /* This would be a bug */
>> +}
>> +
>> +extern void ___bad_prod_cpu(void);
>> +static inline void prod_cpu(int cpu)
>> +{
>> + ___bad_prod_cpu(); /* This would be a bug */
>> +}
>> +
>> #endif
>>
>> #define vcpu_is_preempted vcpu_is_preempted
>> @@ -57,5 +80,10 @@ static inline bool vcpu_is_preempted(int cpu)
>> return false;
>> }
>>
>> +static inline bool pv_is_native_spin_unlock(void)
>> +{
>> + return !is_shared_processor();
>> +}
>> +
>> #endif /* __KERNEL__ */
>> #endif /* __ASM_PARAVIRT_H */
>> diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h
>> index c49e33e24edd..0960a0de2467 100644
>> --- a/arch/powerpc/include/asm/qspinlock.h
>> +++ b/arch/powerpc/include/asm/qspinlock.h
>> @@ -3,9 +3,36 @@
>> #define _ASM_POWERPC_QSPINLOCK_H
>>
>> #include <asm-generic/qspinlock_types.h>
>> +#include <asm/paravirt.h>
>>
>> #define _Q_PENDING_LOOPS (1 << 9) /* not tuned */
>>
>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>> +extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>> +extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>> +
>> +static __always_inline void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>> +{
>> + if (!is_shared_processor())
>> + native_queued_spin_lock_slowpath(lock, val);
>> + else
>> + __pv_queued_spin_lock_slowpath(lock, val);
>> +}
>
> In a previous mail, I said that:
Hey, yeah I read that right after sending the series out. Thanks for the
thorough review.
> You may need to match the use of __pv_queued_spin_lock_slowpath() with
> the corresponding __pv_queued_spin_unlock(), e.g.
>
> #define queued_spin_unlock queued_spin_unlock
> static inline queued_spin_unlock(struct qspinlock *lock)
> {
> if (!is_shared_processor())
> smp_store_release(&lock->locked, 0);
> else
> __pv_queued_spin_unlock(lock);
> }
>
> Otherwise, pv_kick() will never be called.
>
> Maybe PowerPC HMT is different that the shared cpus can still process
> instruction, though slower, that cpu kicking like what was done in kvm
> is not really necessary. If that is the case, I think we should document
> that.
It does stop dispatch, but it will wake up by itself after all other
vCPUs have had a chance to dispatch. I will re-test with the fix in
place and see if there's any significant performance differences.
Thanks,
Nick
