Re: [kvm-unit-tests PATCH] arm: Remove MAX_SMP probe loop

From: Colton Lewis <coltonlewis@google.com>
To: Andrew Jones <andrew.jones@linux.dev>
Cc: alexandru.elisei@arm.com, kvm@vger.kernel.org,
	kvmarm@lists.cs.columbia.edu, maz@kernel.org,
	eric.auger@redhat.com, oliver.upton@linux.dev, reijiw@google.com,
	ricarkol@google.com
Subject: Re: [kvm-unit-tests PATCH] arm: Remove MAX_SMP probe loop
Date: Mon, 09 Jan 2023 21:43:54 +0000	[thread overview]
Message-ID: <gsntr0w3qt7p.fsf@coltonlewis-kvm.c.googlers.com> (raw)
In-Reply-To: <20230109085907.nklhxqz2vrfpengj@orel> (message from Andrew Jones on Mon, 9 Jan 2023 09:59:07 +0100)

Andrew Jones <andrew.jones@linux.dev> writes:

> On Fri, Jan 06, 2023 at 05:37:16PM +0000, Colton Lewis wrote:
>> Andrew Jones <andrew.jones@linux.dev> writes:

>> > > We could cap at 8 for ACCEL=tcg. Even if no one cares, I'm tempted  
>> to do
>> > > it so no one hits the same little landmine as me in the future.

>> > TCG supports up to 255 CPUs. The only reason it'd have a max of 8 is if
>> > you were configuring a GICv2 instead of a GICv3.

>> That makes sense as it was the GICv2 tests failing that led me to this
>> rabbit hole. In that case, it should be completely safe to delete the
>> loop because all the GICv2 tests have ternary condition to cap at 8
>> already.

> How did your gicv2 tests hit the problem?

Mysteriously. QEMU/TCG failed those tests when given -smp 4 but not -smp
8. There's the unresoved question of why they fail then (could be a QEMU
bug). And the second question of why the test was using -smp 4 on some
machines and not others, which is due to the loop I am trying to remove.

>> If we can't delete, the loop logic is still a suboptimal way to do
>> things as qemu reports the max cpus it can take. We could read MAX_SMP
>> from qemu error output.

> Patches welcome, but you'll want to ensure older QEMU also reports the
> number of max CPUs. Basically, either we completely drop the loop,
> which assumes we're no longer concerned with testing kernels older than
> v4.3 and testing shows we always get a working number of CPUs, or we
> change the loop to parsing QEMU's output, but that requires testing
> all versions of QEMU we care about report the error the same way, or
> we leave the loop as is. Alex says the speedup obtained by dropping
> the extra QEMU invocations is noticeable, so that's a vote for doing
> something, but whatever is chosen will need a commit message identifying
> which versions of kernel and/or QEMU are now the oldest ones supported.

Theoretically, I guess we could always have a machine with more CPUs
than QEMU supports. Checking multiple QEMU versions for a specific
message format sounds tedious and flakey. What we really need is a
better way to query QEMU for max cpus and then take min(cpus on machine,
cpus QEMU supports). Any ideas? I sent an email asking the QEMU mailing
list.