[PATCH] powerpc/pseries: Use doorbells even if XIVE is available

[Michael Ellerman]

Nicholas Piggin <npiggin at gmail.com> writes:
> KVM supports msgsndp in guests by trapping and emulating the
> instruction, so it was decided to always use XIVE for IPIs if it is
> available. However on PowerVM systems, msgsndp can be used and gives
> better performance. On large systems, high XIVE interrupt rates can
> have sub-linear scaling, and using msgsndp can reduce the load on
> the interrupt controller.
>
> So switch to using core local doorbells even if XIVE is available.
> This reduces performance for KVM guests with an SMT topology by
> about 50% for ping-pong context switching between SMT vCPUs.

You have to take explicit steps to configure KVM in that way with qemu.
eg. "qemu .. -smp 8" will give you 8 SMT1 CPUs by default.

> An option vector (or dt-cpu-ftrs) could be defined to disable msgsndp
> to get KVM performance back.

Qemu/KVM populates /proc/device-tree/hypervisor, so we *could* look at
that. Though adding PowerVM/KVM specific hacks is obviously a very
slippery slope.

> diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
> index 6891710833be..a737a2f87c67 100644
> --- a/arch/powerpc/platforms/pseries/smp.c
> +++ b/arch/powerpc/platforms/pseries/smp.c
> @@ -188,13 +188,14 @@ static int pseries_smp_prepare_cpu(int cpu)
>  	return 0;
>  }
>  
> +static void  (*cause_ipi_offcore)(int cpu) __ro_after_init;
> +
>  static void smp_pseries_cause_ipi(int cpu)

This is static so the name could be more descriptive, it doesn't need
the "smp_pseries" prefix.

>  {
> -	/* POWER9 should not use this handler */
>  	if (doorbell_try_core_ipi(cpu))
>  		return;

Seems like it would be worth making that static inline so we can avoid
the function call overhead.

> -	icp_ops->cause_ipi(cpu);
> +	cause_ipi_offcore(cpu);
>  }
>  
>  static int pseries_cause_nmi_ipi(int cpu)
> @@ -222,10 +223,7 @@ static __init void pSeries_smp_probe_xics(void)
>  {
>  	xics_smp_probe();
>  
> -	if (cpu_has_feature(CPU_FTR_DBELL) && !is_secure_guest())
> -		smp_ops->cause_ipi = smp_pseries_cause_ipi;
> -	else
> -		smp_ops->cause_ipi = icp_ops->cause_ipi;
> +	smp_ops->cause_ipi = icp_ops->cause_ipi;
>  }
>  
>  static __init void pSeries_smp_probe(void)
> @@ -238,6 +236,18 @@ static __init void pSeries_smp_probe(void)

The comment just above here says:

		/*
		 * Don't use P9 doorbells when XIVE is enabled. IPIs
		 * using MMIOs should be faster
		 */
>  		xive_smp_probe();

Which is no longer true.

>  	else
>  		pSeries_smp_probe_xics();

I think you should just fold this in, it would make the logic slightly
easier to follow.

> +	/*
> +	 * KVM emulates doorbells by reading the instruction, which
> +	 * can't be done if the guest is secure. If a secure guest
> +	 * runs under PowerVM, it could use msgsndp but would need a
> +	 * way to distinguish.
> +	 */

It's not clear what it needs to distinguish: That it's running under
PowerVM and therefore *can* use msgsndp even though it's secure.

Also the comment just talks about the is_secure_guest() test, which is
not obvious on first reading.

> +	if (cpu_has_feature(CPU_FTR_DBELL) &&
> +	    cpu_has_feature(CPU_FTR_SMT) && !is_secure_guest()) {
> +		cause_ipi_offcore = smp_ops->cause_ipi;
> +		smp_ops->cause_ipi = smp_pseries_cause_ipi;
> +	}

Because we're at the tail of the function I think this would be clearer
if it used early returns, eg:

	// If the CPU doesn't have doorbells then we must use xics/xive
	if (!cpu_has_feature(CPU_FTR_DBELL))
        	return;

	// If the CPU doesn't have SMT then doorbells don't help us
	if (!cpu_has_feature(CPU_FTR_SMT))
        	return;

	// Secure guests can't use doorbells because ...
	if (!is_secure_guest()
        	return;

	/*
         * Otherwise we want to use doorbells for sibling threads and
         * xics/xive for IPIs off the core, because it performs better
         * on large systems ...
         */
        cause_ipi_offcore = smp_ops->cause_ipi;
	smp_ops->cause_ipi = smp_pseries_cause_ipi;
}


cheers