Nicholas Piggin <npiggin@xxxxxxxxx> 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
