* [kvm-unit-tests PATCH v3 0/1] arm: Replace MAX_SMP probe loop
@ 2023-01-30 19:56 Colton Lewis
2023-01-30 19:57 ` [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly Colton Lewis
0 siblings, 1 reply; 6+ messages in thread
From: Colton Lewis @ 2023-01-30 19:56 UTC (permalink / raw)
To: thuth, pbonzini, nrb, andrew.jones, imbrenda, marcorr,
alexandru.elisei, oliver.upton
Cc: kvm, kvmarm, Colton Lewis
v3:
Rewrite v2 using one invocation of QEMU and bash parameter
substitution instead of awk. Clarify conditions under which this
change is needed.
Colton Lewis (1):
arm: Replace MAX_SMP probe loop in favor of reading directly
scripts/runtime.bash | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
--
2.39.1.456.gfc5497dd1b-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly
2023-01-30 19:56 [kvm-unit-tests PATCH v3 0/1] arm: Replace MAX_SMP probe loop Colton Lewis
@ 2023-01-30 19:57 ` Colton Lewis
2023-01-31 6:32 ` Andrew Jones
0 siblings, 1 reply; 6+ messages in thread
From: Colton Lewis @ 2023-01-30 19:57 UTC (permalink / raw)
To: thuth, pbonzini, nrb, andrew.jones, imbrenda, marcorr,
alexandru.elisei, oliver.upton
Cc: kvm, kvmarm, Colton Lewis
Replace the MAX_SMP probe loop in favor of reading a number directly
from the QEMU error message. This is equally safe as the existing code
because the error message has had the same format as long as it has
existed, since QEMU v2.10. The final number before the end of the
error message line indicates the max QEMU supports. A short awk
program is used to extract the number, which becomes the new MAX_SMP
value.
This loop logic is broken for machines with a number of CPUs that
isn't a power of two. This problem was noticed for gicv2 tests on
machines with a non-power-of-two number of CPUs greater than 8 because
tests were running with MAX_SMP less than 8. As a hypthetical example,
a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 1 ==
6. This can, in rare circumstances, lead to different test results
depending only on the number of CPUs the machine has.
A previous comment explains the loop should only apply to kernels
<=v4.3 on arm and suggests deletion when it becomes tiresome to
maintian. However, it is always theoretically possible to test on a
machine that has more CPUs than QEMU supports, so it makes sense to
leave some check in place.
Signed-off-by: Colton Lewis <coltonlewis@google.com>
---
scripts/runtime.bash | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index f8794e9a..587ffe30 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -188,12 +188,10 @@ function run()
# 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
+# error first. The awk program takes the last number from the QEMU
+# error message, which gives the allowable MAX_SMP.
+if smp=$($RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
+ |& grep 'exceeds max CPUs'); then
+ smp=${smp##*(}
+ MAX_SMP=${smp:0:-1}
+fi
--
2.39.1.456.gfc5497dd1b-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly
2023-01-30 19:57 ` [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly Colton Lewis
@ 2023-01-31 6:32 ` Andrew Jones
2023-01-31 7:41 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jones @ 2023-01-31 6:32 UTC (permalink / raw)
To: Colton Lewis
Cc: thuth, pbonzini, nrb, imbrenda, marcorr, alexandru.elisei,
oliver.upton, kvm, kvmarm
On Mon, Jan 30, 2023 at 07:57:00PM +0000, Colton Lewis wrote:
> Replace the MAX_SMP probe loop in favor of reading a number directly
> from the QEMU error message. This is equally safe as the existing code
> because the error message has had the same format as long as it has
> existed, since QEMU v2.10. The final number before the end of the
> error message line indicates the max QEMU supports. A short awk
awk is not used, despite the comment also being updated to say it's
being used.
> program is used to extract the number, which becomes the new MAX_SMP
> value.
>
> This loop logic is broken for machines with a number of CPUs that
> isn't a power of two. This problem was noticed for gicv2 tests on
> machines with a non-power-of-two number of CPUs greater than 8 because
> tests were running with MAX_SMP less than 8. As a hypthetical example,
> a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 1 ==
> 6. This can, in rare circumstances, lead to different test results
> depending only on the number of CPUs the machine has.
>
> A previous comment explains the loop should only apply to kernels
> <=v4.3 on arm and suggests deletion when it becomes tiresome to
> maintian. However, it is always theoretically possible to test on a
> machine that has more CPUs than QEMU supports, so it makes sense to
> leave some check in place.
>
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> scripts/runtime.bash | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index f8794e9a..587ffe30 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -188,12 +188,10 @@ function run()
> # 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
It just occurred to me that this code runs on all architectures, even
though it only works for Arm. We should wrap this code in $ARCH
checks or put it in a function which only Arm calls. That change
should be a separate patch though.
> -# 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
> +# error first. The awk program takes the last number from the QEMU
> +# error message, which gives the allowable MAX_SMP.
> +if smp=$($RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> + |& grep 'exceeds max CPUs'); then
> + smp=${smp##*(}
> + MAX_SMP=${smp:0:-1}
> +fi
> --
> 2.39.1.456.gfc5497dd1b-goog
>
Thanks,
drew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly
2023-01-31 6:32 ` Andrew Jones
@ 2023-01-31 7:41 ` Thomas Huth
2023-01-31 10:57 ` Andrew Jones
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2023-01-31 7:41 UTC (permalink / raw)
To: Andrew Jones, Colton Lewis
Cc: pbonzini, nrb, imbrenda, marcorr, alexandru.elisei, oliver.upton,
kvm, kvmarm
On 31/01/2023 07.32, Andrew Jones wrote:
> On Mon, Jan 30, 2023 at 07:57:00PM +0000, Colton Lewis wrote:
>> Replace the MAX_SMP probe loop in favor of reading a number directly
>> from the QEMU error message. This is equally safe as the existing code
>> because the error message has had the same format as long as it has
>> existed, since QEMU v2.10. The final number before the end of the
>> error message line indicates the max QEMU supports. A short awk
>
> awk is not used, despite the comment also being updated to say it's
> being used.
>
>> program is used to extract the number, which becomes the new MAX_SMP
>> value.
>>
>> This loop logic is broken for machines with a number of CPUs that
>> isn't a power of two. This problem was noticed for gicv2 tests on
>> machines with a non-power-of-two number of CPUs greater than 8 because
>> tests were running with MAX_SMP less than 8. As a hypthetical example,
s/hypthetical/hypothetical/
>> a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 1 ==
>> 6. This can, in rare circumstances, lead to different test results
>> depending only on the number of CPUs the machine has.
>>
>> A previous comment explains the loop should only apply to kernels
>> <=v4.3 on arm and suggests deletion when it becomes tiresome to
>> maintian. However, it is always theoretically possible to test on a
s/maintian/maintain/
>> machine that has more CPUs than QEMU supports, so it makes sense to
>> leave some check in place.
>>
>> Signed-off-by: Colton Lewis <coltonlewis@google.com>
>> ---
>> scripts/runtime.bash | 16 +++++++---------
>> 1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> index f8794e9a..587ffe30 100644
>> --- a/scripts/runtime.bash
>> +++ b/scripts/runtime.bash
>> @@ -188,12 +188,10 @@ function run()
>> # 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
>
> It just occurred to me that this code runs on all architectures, even
> though it only works for Arm. We should wrap this code in $ARCH
> checks or put it in a function which only Arm calls. That change
> should be a separate patch though.
Or we just grep for "max CPUs", since this seems to be used on other
architectures, too:
$ qemu-system-x86_64 -smp 12345
qemu-system-x86_64: Invalid SMP CPUs 12345. The max CPUs supported by
machine 'pc-i440fx-8.0' is 255
?
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly
2023-01-31 7:41 ` Thomas Huth
@ 2023-01-31 10:57 ` Andrew Jones
2023-01-31 11:00 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jones @ 2023-01-31 10:57 UTC (permalink / raw)
To: Thomas Huth
Cc: Colton Lewis, pbonzini, nrb, imbrenda, marcorr, alexandru.elisei,
oliver.upton, kvm, kvmarm
On Tue, Jan 31, 2023 at 08:41:39AM +0100, Thomas Huth wrote:
> On 31/01/2023 07.32, Andrew Jones wrote:
> > On Mon, Jan 30, 2023 at 07:57:00PM +0000, Colton Lewis wrote:
> > > Replace the MAX_SMP probe loop in favor of reading a number directly
> > > from the QEMU error message. This is equally safe as the existing code
> > > because the error message has had the same format as long as it has
> > > existed, since QEMU v2.10. The final number before the end of the
> > > error message line indicates the max QEMU supports. A short awk
> >
> > awk is not used, despite the comment also being updated to say it's
> > being used.
> >
> > > program is used to extract the number, which becomes the new MAX_SMP
> > > value.
> > >
> > > This loop logic is broken for machines with a number of CPUs that
> > > isn't a power of two. This problem was noticed for gicv2 tests on
> > > machines with a non-power-of-two number of CPUs greater than 8 because
> > > tests were running with MAX_SMP less than 8. As a hypthetical example,
>
> s/hypthetical/hypothetical/
>
> > > a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 1 ==
> > > 6. This can, in rare circumstances, lead to different test results
> > > depending only on the number of CPUs the machine has.
> > >
> > > A previous comment explains the loop should only apply to kernels
> > > <=v4.3 on arm and suggests deletion when it becomes tiresome to
> > > maintian. However, it is always theoretically possible to test on a
>
> s/maintian/maintain/
>
> > > machine that has more CPUs than QEMU supports, so it makes sense to
> > > leave some check in place.
> > >
> > > Signed-off-by: Colton Lewis <coltonlewis@google.com>
> > > ---
> > > scripts/runtime.bash | 16 +++++++---------
> > > 1 file changed, 7 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > index f8794e9a..587ffe30 100644
> > > --- a/scripts/runtime.bash
> > > +++ b/scripts/runtime.bash
> > > @@ -188,12 +188,10 @@ function run()
> > > # 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
> >
> > It just occurred to me that this code runs on all architectures, even
> > though it only works for Arm. We should wrap this code in $ARCH
> > checks or put it in a function which only Arm calls. That change
> > should be a separate patch though.
>
> Or we just grep for "max CPUs", since this seems to be used on other
> architectures, too:
>
> $ qemu-system-x86_64 -smp 12345
> qemu-system-x86_64: Invalid SMP CPUs 12345. The max CPUs supported by
> machine 'pc-i440fx-8.0' is 255
>
> ?
>
Yes, if we can find an arch-common way to set MAX_SMP, then the variable
could be used in their test configs and gitlab-ci scripts. For example,
afaict, x86 doesn't have any tests that run with more than 4 cpus at the
moment. Being able to bump that up for some tests might increase test
coverage. That said, it might be stretching the scope of this patch a bit
much. How about we keep the grep the same for now and guard with $ARCH.
Other architectures can either share Arm's grep, tweak the grep and share
it, or add their own grep, when/if they want to start using MAX_SMP.
Thanks,
drew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly
2023-01-31 10:57 ` Andrew Jones
@ 2023-01-31 11:00 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-01-31 11:00 UTC (permalink / raw)
To: Andrew Jones
Cc: Colton Lewis, pbonzini, nrb, imbrenda, marcorr, alexandru.elisei,
oliver.upton, kvm, kvmarm
On 31/01/2023 11.57, Andrew Jones wrote:
> On Tue, Jan 31, 2023 at 08:41:39AM +0100, Thomas Huth wrote:
>> On 31/01/2023 07.32, Andrew Jones wrote:
>>> On Mon, Jan 30, 2023 at 07:57:00PM +0000, Colton Lewis wrote:
>>>> Replace the MAX_SMP probe loop in favor of reading a number directly
>>>> from the QEMU error message. This is equally safe as the existing code
>>>> because the error message has had the same format as long as it has
>>>> existed, since QEMU v2.10. The final number before the end of the
>>>> error message line indicates the max QEMU supports. A short awk
>>>
>>> awk is not used, despite the comment also being updated to say it's
>>> being used.
>>>
>>>> program is used to extract the number, which becomes the new MAX_SMP
>>>> value.
>>>>
>>>> This loop logic is broken for machines with a number of CPUs that
>>>> isn't a power of two. This problem was noticed for gicv2 tests on
>>>> machines with a non-power-of-two number of CPUs greater than 8 because
>>>> tests were running with MAX_SMP less than 8. As a hypthetical example,
>>
>> s/hypthetical/hypothetical/
>>
>>>> a machine with 12 CPUs will test with MAX_SMP=6 because 12 >> 1 ==
>>>> 6. This can, in rare circumstances, lead to different test results
>>>> depending only on the number of CPUs the machine has.
>>>>
>>>> A previous comment explains the loop should only apply to kernels
>>>> <=v4.3 on arm and suggests deletion when it becomes tiresome to
>>>> maintian. However, it is always theoretically possible to test on a
>>
>> s/maintian/maintain/
>>
>>>> machine that has more CPUs than QEMU supports, so it makes sense to
>>>> leave some check in place.
>>>>
>>>> Signed-off-by: Colton Lewis <coltonlewis@google.com>
>>>> ---
>>>> scripts/runtime.bash | 16 +++++++---------
>>>> 1 file changed, 7 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>>>> index f8794e9a..587ffe30 100644
>>>> --- a/scripts/runtime.bash
>>>> +++ b/scripts/runtime.bash
>>>> @@ -188,12 +188,10 @@ function run()
>>>> # 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
>>>
>>> It just occurred to me that this code runs on all architectures, even
>>> though it only works for Arm. We should wrap this code in $ARCH
>>> checks or put it in a function which only Arm calls. That change
>>> should be a separate patch though.
>>
>> Or we just grep for "max CPUs", since this seems to be used on other
>> architectures, too:
>>
>> $ qemu-system-x86_64 -smp 12345
>> qemu-system-x86_64: Invalid SMP CPUs 12345. The max CPUs supported by
>> machine 'pc-i440fx-8.0' is 255
>>
>> ?
>>
>
> Yes, if we can find an arch-common way to set MAX_SMP, then the variable
> could be used in their test configs and gitlab-ci scripts. For example,
> afaict, x86 doesn't have any tests that run with more than 4 cpus at the
> moment. Being able to bump that up for some tests might increase test
> coverage. That said, it might be stretching the scope of this patch a bit
> much. How about we keep the grep the same for now and guard with $ARCH.
> Other architectures can either share Arm's grep, tweak the grep and share
> it, or add their own grep, when/if they want to start using MAX_SMP.
Ok, fair point, we still can adjust later.
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-31 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-30 19:56 [kvm-unit-tests PATCH v3 0/1] arm: Replace MAX_SMP probe loop Colton Lewis
2023-01-30 19:57 ` [kvm-unit-tests PATCH v3 1/1] arm: Replace MAX_SMP probe loop in favor of reading directly Colton Lewis
2023-01-31 6:32 ` Andrew Jones
2023-01-31 7:41 ` Thomas Huth
2023-01-31 10:57 ` Andrew Jones
2023-01-31 11:00 ` Thomas Huth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.