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

Nicholas Piggin [off-list ref] 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.

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.

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