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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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
>