[PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Andrew Jones]

We can only use online cpus, so make sure we check specifically for
those.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 scripts/runtime.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 200d5b67290c..fbad0bd05fc5 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -1,5 +1,5 @@
 : "${RUNTIME_arch_run?}"
-: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
+: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
 : ${TIMEOUT:=90s}
 
 PASS() { echo -ne "\e[32mPASS\e[0m"; }
-- 
2.21.0

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Alexandru Elisei]

Hi,

On 11/20/19 2:19 PM, Andrew Jones wrote:
> We can only use online cpus, so make sure we check specifically for
> those.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  scripts/runtime.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 200d5b67290c..fbad0bd05fc5 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -1,5 +1,5 @@
>  : "${RUNTIME_arch_run?}"
> -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
> +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}

I tested it on my machine by offlining a CPU and calling getconf _NPROCESSORS_CONF
(returned 32) and getconf _NPROCESSORS_ONLN (returned 31). man 3 sysconf also
agrees with your patch.

I am wondering though, if _NPROCESSORS_CONF is 8 and _NPROCESSORS_ONLN is 1
(meaning that 7 CPUs were offlined), that means that qemu will create 8 VCPUs
which will share the same physical CPU. Is that undesirable?

Thanks,
Alex
>  : ${TIMEOUT:=90s}
>  
>  PASS() { echo -ne "\e[32mPASS\e[0m"; }

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Andrew Jones]

On Fri, Nov 22, 2019 at 10:45:08AM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 11/20/19 2:19 PM, Andrew Jones wrote:
> > We can only use online cpus, so make sure we check specifically for
> > those.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  scripts/runtime.bash | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > index 200d5b67290c..fbad0bd05fc5 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -1,5 +1,5 @@
> >  : "${RUNTIME_arch_run?}"
> > -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
> > +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
> 
> I tested it on my machine by offlining a CPU and calling getconf _NPROCESSORS_CONF
> (returned 32) and getconf _NPROCESSORS_ONLN (returned 31). man 3 sysconf also
> agrees with your patch.
> 
> I am wondering though, if _NPROCESSORS_CONF is 8 and _NPROCESSORS_ONLN is 1
> (meaning that 7 CPUs were offlined), that means that qemu will create 8 VCPUs
> which will share the same physical CPU. Is that undesirable?

With KVM enabled that's not recommended. KVM_CAP_NR_VCPUS returns the
number of online VCPUs (at least for arm). Since the guest code may not
run as expected with overcommitted VCPUs we don't usually want to test
that way. OTOH, maybe we should write a test or two that does run with
overcommitted VCPUs in order to look for bugs in KVM.

Thanks,
drew

> 
> Thanks,
> Alex
> >  : ${TIMEOUT:=90s}
> >  
> >  PASS() { echo -ne "\e[32mPASS\e[0m"; }
> 

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Alexandru Elisei]

Hi,

On 11/22/19 11:19 AM, Andrew Jones wrote:
> On Fri, Nov 22, 2019 at 10:45:08AM +0000, Alexandru Elisei wrote:
>> Hi,
>>
>> On 11/20/19 2:19 PM, Andrew Jones wrote:
>>> We can only use online cpus, so make sure we check specifically for
>>> those.
>>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  scripts/runtime.bash | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>>> index 200d5b67290c..fbad0bd05fc5 100644
>>> --- a/scripts/runtime.bash
>>> +++ b/scripts/runtime.bash
>>> @@ -1,5 +1,5 @@
>>>  : "${RUNTIME_arch_run?}"
>>> -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
>>> +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
>> I tested it on my machine by offlining a CPU and calling getconf _NPROCESSORS_CONF
>> (returned 32) and getconf _NPROCESSORS_ONLN (returned 31). man 3 sysconf also
>> agrees with your patch.
>>
>> I am wondering though, if _NPROCESSORS_CONF is 8 and _NPROCESSORS_ONLN is 1
>> (meaning that 7 CPUs were offlined), that means that qemu will create 8 VCPUs
>> which will share the same physical CPU. Is that undesirable?
> With KVM enabled that's not recommended. KVM_CAP_NR_VCPUS returns the
> number of online VCPUs (at least for arm). Since the guest code may not
> run as expected with overcommitted VCPUs we don't usually want to test

Can you give more details about what may go wrong if several kvm-unit-tests VCPUs
run on the same physical CPU? I was thinking that maybe we want to run the VCPUs
in parallel (each on its own physical CPU) to detect races in KVM, not because of
a limitation in kvm-unit-tests.

Thanks,
Alex
> that way. OTOH, maybe we should write a test or two that does run with
> overcommitted VCPUs in order to look for bugs in KVM.
>
> Thanks,
> drew
>> Thanks,
>> Alex
>>>  : ${TIMEOUT:=90s}
>>>  
>>>  PASS() { echo -ne "\e[32mPASS\e[0m"; }

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Andrew Jones]

On Fri, Nov 22, 2019 at 11:35:33AM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 11/22/19 11:19 AM, Andrew Jones wrote:
> > On Fri, Nov 22, 2019 at 10:45:08AM +0000, Alexandru Elisei wrote:
> >> Hi,
> >>
> >> On 11/20/19 2:19 PM, Andrew Jones wrote:
> >>> We can only use online cpus, so make sure we check specifically for
> >>> those.
> >>>
> >>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >>> ---
> >>>  scripts/runtime.bash | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> >>> index 200d5b67290c..fbad0bd05fc5 100644
> >>> --- a/scripts/runtime.bash
> >>> +++ b/scripts/runtime.bash
> >>> @@ -1,5 +1,5 @@
> >>>  : "${RUNTIME_arch_run?}"
> >>> -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
> >>> +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
> >> I tested it on my machine by offlining a CPU and calling getconf _NPROCESSORS_CONF
> >> (returned 32) and getconf _NPROCESSORS_ONLN (returned 31). man 3 sysconf also
> >> agrees with your patch.
> >>
> >> I am wondering though, if _NPROCESSORS_CONF is 8 and _NPROCESSORS_ONLN is 1
> >> (meaning that 7 CPUs were offlined), that means that qemu will create 8 VCPUs
> >> which will share the same physical CPU. Is that undesirable?
> > With KVM enabled that's not recommended. KVM_CAP_NR_VCPUS returns the
> > number of online VCPUs (at least for arm). Since the guest code may not
> > run as expected with overcommitted VCPUs we don't usually want to test
> 
> Can you give more details about what may go wrong if several kvm-unit-tests VCPUs
> run on the same physical CPU? I was thinking that maybe we want to run the VCPUs
> in parallel (each on its own physical CPU) to detect races in KVM, not because of
> a limitation in kvm-unit-tests.

There's no specific limitation in kvm-unit-tests. All guest kernels are at
risk of behaving strangely with overcommitted VCPUs.

A made-up example could be that a guest kernel thread T1 needs to wait for
another thread T2. T1 may choose not to yield because T2 appears to be
running on another CPU. But it isn't, because both T1's VCPU and T2's VCPU
are running on the same PCPU. In general, telling a guest kernel it can
run threads in parallel may cause it to make decisions that won't work
well when executed serially.

In kvm-unit-tests, where we can know that we're overcommitted in certain
test cases, we can actually write tests that will still behave
predictably. Knowing the test runs fine with overcommitted VCPUs will
allow us to check if KVM does as well.

Thanks,
drew

> 
> Thanks,
> Alex
> > that way. OTOH, maybe we should write a test or two that does run with
> > overcommitted VCPUs in order to look for bugs in KVM.
> >
> > Thanks,
> > drew
> >> Thanks,
> >> Alex
> >>>  : ${TIMEOUT:=90s}
> >>>  
> >>>  PASS() { echo -ne "\e[32mPASS\e[0m"; }
> 

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Alexandru Elisei]

Hi,

On 11/22/19 2:57 PM, Andrew Jones wrote:
> On Fri, Nov 22, 2019 at 11:35:33AM +0000, Alexandru Elisei wrote:
>> Hi,
>>
>> On 11/22/19 11:19 AM, Andrew Jones wrote:
>>> On Fri, Nov 22, 2019 at 10:45:08AM +0000, Alexandru Elisei wrote:
>>>> Hi,
>>>>
>>>> On 11/20/19 2:19 PM, Andrew Jones wrote:
>>>>> We can only use online cpus, so make sure we check specifically for
>>>>> those.
>>>>>
>>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>>> ---
>>>>>  scripts/runtime.bash | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>>>>> index 200d5b67290c..fbad0bd05fc5 100644
>>>>> --- a/scripts/runtime.bash
>>>>> +++ b/scripts/runtime.bash
>>>>> @@ -1,5 +1,5 @@
>>>>>  : "${RUNTIME_arch_run?}"
>>>>> -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
>>>>> +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
>>>> I tested it on my machine by offlining a CPU and calling getconf _NPROCESSORS_CONF
>>>> (returned 32) and getconf _NPROCESSORS_ONLN (returned 31). man 3 sysconf also
>>>> agrees with your patch.
>>>>
>>>> I am wondering though, if _NPROCESSORS_CONF is 8 and _NPROCESSORS_ONLN is 1
>>>> (meaning that 7 CPUs were offlined), that means that qemu will create 8 VCPUs
>>>> which will share the same physical CPU. Is that undesirable?
>>> With KVM enabled that's not recommended. KVM_CAP_NR_VCPUS returns the
>>> number of online VCPUs (at least for arm). Since the guest code may not
>>> run as expected with overcommitted VCPUs we don't usually want to test
>> Can you give more details about what may go wrong if several kvm-unit-tests VCPUs
>> run on the same physical CPU? I was thinking that maybe we want to run the VCPUs
>> in parallel (each on its own physical CPU) to detect races in KVM, not because of
>> a limitation in kvm-unit-tests.
> There's no specific limitation in kvm-unit-tests. All guest kernels are at
> risk of behaving strangely with overcommitted VCPUs.
>
> A made-up example could be that a guest kernel thread T1 needs to wait for
> another thread T2. T1 may choose not to yield because T2 appears to be
> running on another CPU. But it isn't, because both T1's VCPU and T2's VCPU
> are running on the same PCPU. In general, telling a guest kernel it can
> run threads in parallel may cause it to make decisions that won't work
> well when executed serially.
>
> In kvm-unit-tests, where we can know that we're overcommitted in certain
> test cases, we can actually write tests that will still behave
> predictably. Knowing the test runs fine with overcommitted VCPUs will
> allow us to check if KVM does as well.

Makes sense, thank you for the explanation. For the patch:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex
> Thanks,
> drew
>
>> Thanks,
>> Alex
>>> that way. OTOH, maybe we should write a test or two that does run with
>>> overcommitted VCPUs in order to look for bugs in KVM.
>>>
>>> Thanks,
>>> drew
>>>> Thanks,
>>>> Alex
>>>>>  : ${TIMEOUT:=90s}
>>>>>  
>>>>>  PASS() { echo -ne "\e[32mPASS\e[0m"; }

Re: [PATCH kvm-unit-tests] runtime: set MAX_SMP to number of online cpus

[Paolo Bonzini]

On 20/11/19 15:19, Andrew Jones wrote:
> We can only use online cpus, so make sure we check specifically for
> those.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  scripts/runtime.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 200d5b67290c..fbad0bd05fc5 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -1,5 +1,5 @@
>  : "${RUNTIME_arch_run?}"
> -: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
> +: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
>  : ${TIMEOUT:=90s}
>  
>  PASS() { echo -ne "\e[32mPASS\e[0m"; }
> 

Applied, thanks.

Paolo