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

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

[Colton Lewis]

This loop logic is broken for machines with a number of CPUs that
isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
6. This can, in rare circumstances, lead to different test results
depending only on the number of CPUs the machine has.

The loop is safe to remove with no side effects. It has an explanitory
comment explaining that it only applies to kernels <=v4.3 on arm and
suggestion deletion when it becomes tiresome to maintain.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---
 scripts/runtime.bash | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index f8794e9..18a8dd7 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -183,17 +183,3 @@ function run()
 
     return $ret
 }
-
-#
-# Probe for MAX_SMP, in case it's less than the number of host cpus.
-#
-# This probing currently only works for ARM, as x86 bails on another
-# error first. Also, this probing isn't necessary for any ARM hosts
-# running kernels later than v4.3, i.e. those including ef748917b52
-# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
-# point when maintaining the while loop gets too tiresome, we can
-# just remove it...
-while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
-		|& grep -qi 'exceeds max CPUs'; do
-	MAX_SMP=$((MAX_SMP >> 1))
-done
-- 
2.39.0.314.g84b9a713c41-goog

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

[Colton Lewis]

This loop logic is broken for machines with a number of CPUs that
isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
6. This can, in rare circumstances, lead to different test results
depending only on the number of CPUs the machine has.

The loop is safe to remove with no side effects. It has an explanitory
comment explaining that it only applies to kernels <=v4.3 on arm and
suggestion deletion when it becomes tiresome to maintain.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---
 scripts/runtime.bash | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index f8794e9..18a8dd7 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -183,17 +183,3 @@ function run()
 
     return $ret
 }
-
-#
-# Probe for MAX_SMP, in case it's less than the number of host cpus.
-#
-# This probing currently only works for ARM, as x86 bails on another
-# error first. Also, this probing isn't necessary for any ARM hosts
-# running kernels later than v4.3, i.e. those including ef748917b52
-# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
-# point when maintaining the while loop gets too tiresome, we can
-# just remove it...
-while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
-		|& grep -qi 'exceeds max CPUs'; do
-	MAX_SMP=$((MAX_SMP >> 1))
-done
-- 
2.39.0.314.g84b9a713c41-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Alexandru Elisei]

Hi,

On Mon, Dec 19, 2022 at 06:52:50PM +0000, Colton Lewis wrote:
> This loop logic is broken for machines with a number of CPUs that
> isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
> but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
> 6. This can, in rare circumstances, lead to different test results
> depending only on the number of CPUs the machine has.

I do think removing the loop is a good idea. I tested this patch, and when
running a single test, it makes the run 3 seconds faster on an Cortex-A53
for me - selftest-smp and selftest-vectors-kernel went from taking 12s to
9s. Doesn't make much of a difference when running all the tests (those
take for me 5m10s without the patch, 5m6.5s with the patch), but when
running a single test the 25% speed improvement is noticeable.

Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores on
a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
so the body of the loop should never execute. I also tried it on a 6 core
machine, and MAX_SMP was 6, not 3.

Am I missing something?

Thanks,
Alex

> 
> The loop is safe to remove with no side effects. It has an explanitory
> comment explaining that it only applies to kernels <=v4.3 on arm and
> suggestion deletion when it becomes tiresome to maintain.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  scripts/runtime.bash | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index f8794e9..18a8dd7 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -183,17 +183,3 @@ function run()
>  
>      return $ret
>  }
> -
> -#
> -# Probe for MAX_SMP, in case it's less than the number of host cpus.
> -#
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

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

[Alexandru Elisei]

Hi,

On Mon, Dec 19, 2022 at 06:52:50PM +0000, Colton Lewis wrote:
> This loop logic is broken for machines with a number of CPUs that
> isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
> but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
> 6. This can, in rare circumstances, lead to different test results
> depending only on the number of CPUs the machine has.

I do think removing the loop is a good idea. I tested this patch, and when
running a single test, it makes the run 3 seconds faster on an Cortex-A53
for me - selftest-smp and selftest-vectors-kernel went from taking 12s to
9s. Doesn't make much of a difference when running all the tests (those
take for me 5m10s without the patch, 5m6.5s with the patch), but when
running a single test the 25% speed improvement is noticeable.

Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores on
a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
so the body of the loop should never execute. I also tried it on a 6 core
machine, and MAX_SMP was 6, not 3.

Am I missing something?

Thanks,
Alex

> 
> The loop is safe to remove with no side effects. It has an explanitory
> comment explaining that it only applies to kernels <=v4.3 on arm and
> suggestion deletion when it becomes tiresome to maintain.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  scripts/runtime.bash | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index f8794e9..18a8dd7 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -183,17 +183,3 @@ function run()
>  
>      return $ret
>  }
> -
> -#
> -# Probe for MAX_SMP, in case it's less than the number of host cpus.
> -#
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Colton Lewis]

Alexandru Elisei <alexandru.elisei@arm.com> writes:

> Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores  
> on
> a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
> so the body of the loop should never execute. I also tried it on a 6 core
> machine, and MAX_SMP was 6, not 3.

> Am I missing something?

To be clear, 12 cores was a simplified example I did not directly
verify. What happened to me was 152 cores being cut down to 4. I was
confused why one machine was running a test with 4 cores when my other
machines were running with 8 and traced it to that loop. In effect the
loop was doing MAX_SMP=floor(MAX_SMP / 2) until MAX_SMP <= 8. I printed
the iterations and MAX_SMP followed the sequence 152->76->38->19->9->4.

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

[Colton Lewis]

Alexandru Elisei <alexandru.elisei@arm.com> writes:

> Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores  
> on
> a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
> so the body of the loop should never execute. I also tried it on a 6 core
> machine, and MAX_SMP was 6, not 3.

> Am I missing something?

To be clear, 12 cores was a simplified example I did not directly
verify. What happened to me was 152 cores being cut down to 4. I was
confused why one machine was running a test with 4 cores when my other
machines were running with 8 and traced it to that loop. In effect the
loop was doing MAX_SMP=floor(MAX_SMP / 2) until MAX_SMP <= 8. I printed
the iterations and MAX_SMP followed the sequence 152->76->38->19->9->4.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Mon, Dec 19, 2022 at 06:52:50PM +0000, Colton Lewis wrote:
> This loop logic is broken for machines with a number of CPUs that
> isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
> but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
                                                                    ^ 1
> 6. This can, in rare circumstances, lead to different test results
> depending only on the number of CPUs the machine has.
> 
> The loop is safe to remove with no side effects. It has an explanitory
> comment explaining that it only applies to kernels <=v4.3 on arm and
> suggestion deletion when it becomes tiresome to maintain.

Removing this loop is safe if the body of the loop is not expected to
ever run, i.e. we're through testing kernels older than v4.3. But, if
MAX_SMP is getting reduced, as stated above, then that implies

 $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP

is resulting in a "exceeds max CPUs" error. If that's the case, then
the tests which configure $MAX_SMP cpus should be failing as well.

IOW, the loop should never do anything except be a pointless extra
invocation of QEMU. If it does do something, then we should understand
why.

Thanks,
drew

> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  scripts/runtime.bash | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index f8794e9..18a8dd7 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -183,17 +183,3 @@ function run()
>  
>      return $ret
>  }
> -
> -#
> -# Probe for MAX_SMP, in case it's less than the number of host cpus.
> -#
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

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

[Andrew Jones]

On Mon, Dec 19, 2022 at 06:52:50PM +0000, Colton Lewis wrote:
> This loop logic is broken for machines with a number of CPUs that
> isn't a power of two. A machine with 8 CPUs will test with MAX_SMP=8
> but a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 2 ==
                                                                    ^ 1
> 6. This can, in rare circumstances, lead to different test results
> depending only on the number of CPUs the machine has.
> 
> The loop is safe to remove with no side effects. It has an explanitory
> comment explaining that it only applies to kernels <=v4.3 on arm and
> suggestion deletion when it becomes tiresome to maintain.

Removing this loop is safe if the body of the loop is not expected to
ever run, i.e. we're through testing kernels older than v4.3. But, if
MAX_SMP is getting reduced, as stated above, then that implies

 $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP

is resulting in a "exceeds max CPUs" error. If that's the case, then
the tests which configure $MAX_SMP cpus should be failing as well.

IOW, the loop should never do anything except be a pointless extra
invocation of QEMU. If it does do something, then we should understand
why.

Thanks,
drew

> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  scripts/runtime.bash | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index f8794e9..18a8dd7 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -183,17 +183,3 @@ function run()
>  
>      return $ret
>  }
> -
> -#
> -# Probe for MAX_SMP, in case it's less than the number of host cpus.
> -#
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Tue, Dec 20, 2022 at 04:32:00PM +0000, Colton Lewis wrote:
> Alexandru Elisei <alexandru.elisei@arm.com> writes:
> 
> > Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores
> > on
> > a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
> > so the body of the loop should never execute. I also tried it on a 6 core
> > machine, and MAX_SMP was 6, not 3.
> 
> > Am I missing something?
> 
> To be clear, 12 cores was a simplified example I did not directly
> verify. What happened to me was 152 cores being cut down to 4. I was
> confused why one machine was running a test with 4 cores when my other
> machines were running with 8 and traced it to that loop. In effect the
> loop was doing MAX_SMP=floor(MAX_SMP / 2) until MAX_SMP <= 8. I printed
> the iterations and MAX_SMP followed the sequence 152->76->38->19->9->4.

Ah, I think I understand now. Were you running 32-bit arm tests? If so,
it'd be good to point that out explicitly in the commit message (the
'arm:' prefix in the summary is ambiguous).

Assuming the loop body was running because it needed to reduce MAX_SMP to
8 or lower for 32-bit arm tests, then we should be replacing the loop with
something that caps MAX_SMP at 8 for 32-bit arm tests instead.

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Tue, Dec 20, 2022 at 04:32:00PM +0000, Colton Lewis wrote:
> Alexandru Elisei <alexandru.elisei@arm.com> writes:
> 
> > Though I'm not sure how you managed to get MAX_SMP to go down to 6 cores
> > on
> > a 12 core machine. MAX_SMP is initialized to $(getconf _NPROCESSORS_ONLN),
> > so the body of the loop should never execute. I also tried it on a 6 core
> > machine, and MAX_SMP was 6, not 3.
> 
> > Am I missing something?
> 
> To be clear, 12 cores was a simplified example I did not directly
> verify. What happened to me was 152 cores being cut down to 4. I was
> confused why one machine was running a test with 4 cores when my other
> machines were running with 8 and traced it to that loop. In effect the
> loop was doing MAX_SMP=floor(MAX_SMP / 2) until MAX_SMP <= 8. I printed
> the iterations and MAX_SMP followed the sequence 152->76->38->19->9->4.

Ah, I think I understand now. Were you running 32-bit arm tests? If so,
it'd be good to point that out explicitly in the commit message (the
'arm:' prefix in the summary is ambiguous).

Assuming the loop body was running because it needed to reduce MAX_SMP to
8 or lower for 32-bit arm tests, then we should be replacing the loop with
something that caps MAX_SMP at 8 for 32-bit arm tests instead.

Thanks,
drew

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

[Colton Lewis]

Andrew Jones <andrew.jones@linux.dev> writes:
> On Tue, Dec 20, 2022 at 04:32:00PM +0000, Colton Lewis wrote:
>> Alexandru Elisei <alexandru.elisei@arm.com> writes:
> Ah, I think I understand now. Were you running 32-bit arm tests? If so,
> it'd be good to point that out explicitly in the commit message (the
> 'arm:' prefix in the summary is ambiguous).

No, this was happening on arm64. Since it had been a while since I noted
this issue, I reviewed it and realized the issue was only happening
using -accel tcg. That was automatically being used on my problem test
machine without me noticing. That's where the limit of 8 seems to be
coming from and why the loop is triggered.

qemu-system-aarch64: Number of SMP CPUs requested (152) exceeds max CPUs  
supported by machine 'mach-virt' (8)

Since this case doesn't directly involve KVM, I doubt anyone cares about
a fix.

> Assuming the loop body was running because it needed to reduce MAX_SMP to
> 8 or lower for 32-bit arm tests, then we should be replacing the loop with
> something that caps MAX_SMP at 8 for 32-bit arm tests instead.

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.

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

[Andrew Jones]

On Thu, Jan 05, 2023 at 11:09:58PM +0000, Colton Lewis wrote:
> Andrew Jones <andrew.jones@linux.dev> writes:
> > On Tue, Dec 20, 2022 at 04:32:00PM +0000, Colton Lewis wrote:
> > > Alexandru Elisei <alexandru.elisei@arm.com> writes:
> > Ah, I think I understand now. Were you running 32-bit arm tests? If so,
> > it'd be good to point that out explicitly in the commit message (the
> > 'arm:' prefix in the summary is ambiguous).
> 
> No, this was happening on arm64. Since it had been a while since I noted
> this issue, I reviewed it and realized the issue was only happening
> using -accel tcg. That was automatically being used on my problem test
> machine without me noticing. That's where the limit of 8 seems to be
> coming from and why the loop is triggered.
> 
> qemu-system-aarch64: Number of SMP CPUs requested (152) exceeds max CPUs
> supported by machine 'mach-virt' (8)
> 
> Since this case doesn't directly involve KVM, I doubt anyone cares about
> a fix.
> 
> > Assuming the loop body was running because it needed to reduce MAX_SMP to
> > 8 or lower for 32-bit arm tests, then we should be replacing the loop with
> > something that caps MAX_SMP at 8 for 32-bit arm tests instead.
> 
> 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. Using gic-version=3 or
gic-version=max should allow the 152 CPUs to work. Actually, I should
have asked about your gic version instead of whether or not the VM was
AArch32 in the first place. I was incorrectly associating the gicv2
limits with arm32 since my memories of these things have started to
blur together...

Thanks,
drew

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

[Colton Lewis]

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.

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.

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

[Andrew Jones]

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?

> 
> 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.

Thanks,
drew

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

[Colton Lewis]

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.