Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

[Peter Zijlstra]

On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote:
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 523673d..ae938ee 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -52,6 +52,21 @@
>  #define SYNC_IO
>  #endif
>  
> +/* For fixing some spinning issues in a guest.
> + * kernel would check if vcpu is preempted during a spin loop.
> + * we support that.
> + */

If you look around in that file you'll notice that the above comment
style is inconsistent.

Nor is the comment really clarifying things, for one you fail to mention
the problem by its known name. You also forget to explain how this
interface will help. How about something like this:

/*
 * In order to deal with a various lock holder preemption issues provide
 * an interface to see if a vCPU is currently running or not.
 *
 * This allows us to terminate optimistic spin loops and block,
 * analogous to the native optimistic spin heuristic of testing if the
 * lock owner task is running or not.
 */

Also, since you now have a useful comment, which is not architecture
specific, I would place it with the common vcpu_is_preempted()
definition in sched.h.

Hmm?

> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted
> +static inline bool arch_vcpu_is_preempted(int cpu)
> +{
> +	struct lppaca *lp = &lppaca_of(cpu);
> +
> +	if (unlikely(!(lppaca_shared_proc(lp) ||
> +			lppaca_dedicated_proc(lp))))
> +		return false;
> +	return !!(be32_to_cpu(lp->yield_count) & 1);
> +}
> +
>  static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>  {
>  	return lock.slock == 0;
> -- 
> 2.4.11
>