1. Patchew
  2. QEMU
  3. [RFC PATCH 0/6] machine: smp parsing fixes and improvement
  4. View patch

[RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

Yanan Wang posted 6 patches 4 years, 7 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eric Blake <eblake@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets
Posted by Yanan Wang 4 years, 7 months ago 
We currently perform zero-check (default the value to 1 if zeroed)
for the computed values of cores/threads, to make sure they are at
least 1. For consistency, we probably should also default sockets
to 1 if the computed value is zero. Note that this won't affect
any existing working cmdlines but will improve the error reporting
of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 1 +
 hw/i386/pc.c      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f17bbe3275..1e194677cd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         } else {
             maxcpus = maxcpus > 0 ? maxcpus : cpus;
             sockets = maxcpus / (cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9b22fdc01..a44511c937 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
         } else {
             maxcpus = maxcpus > 0 ? maxcpus : cpus;
             sockets = maxcpus / (dies * cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-- 
2.19.1
Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets
Posted by Andrew Jones 4 years, 7 months ago 
On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
> We currently perform zero-check (default the value to 1 if zeroed)
> for the computed values of cores/threads, to make sure they are at
> least 1. For consistency, we probably should also default sockets
> to 1 if the computed value is zero. Note that this won't affect
> any existing working cmdlines but will improve the error reporting
> of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".

How does this help error checking? If the user input values that compute a
fractional (rounded down to zero with integer division) value, then we'll
catch that with the sockets*cores*threads == maxcpus test now, but we may
not catch that after this patch.

Thanks,
drew

> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 1 +
>  hw/i386/pc.c      | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f17bbe3275..1e194677cd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          } else {
>              maxcpus = maxcpus > 0 ? maxcpus : cpus;
>              sockets = maxcpus / (cores * threads);
> +            sockets = sockets > 0 ? sockets : 1;
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a9b22fdc01..a44511c937 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>          } else {
>              maxcpus = maxcpus > 0 ? maxcpus : cpus;
>              sockets = maxcpus / (dies * cores * threads);
> +            sockets = sockets > 0 ? sockets : 1;
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> -- 
> 2.19.1
>
Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets
Posted by Daniel P. Berrangé 4 years, 7 months ago 
On Mon, Jul 12, 2021 at 05:00:48PM +0200, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
> > We currently perform zero-check (default the value to 1 if zeroed)
> > for the computed values of cores/threads, to make sure they are at
> > least 1. For consistency, we probably should also default sockets
> > to 1 if the computed value is zero. Note that this won't affect
> > any existing working cmdlines but will improve the error reporting
> > of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".
> 
> How does this help error checking? If the user input values that compute a
> fractional (rounded down to zero with integer division) value, then we'll
> catch that with the sockets*cores*threads == maxcpus test now, but we may
> not catch that after this patch.

Since we're having alot of debates about what should be valid scenarios
vs invalid scenarios, I think this points towards introducing a tests
for the smp_parse function, that enumerates both the correct and
incorrect scenarios based on the current implementation.

This series should then update the test cases for scenarios that we
think are currently wrongly handled.

> 
> Thanks,
> drew
> 
> > 
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >  hw/core/machine.c | 1 +
> >  hw/i386/pc.c      | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index f17bbe3275..1e194677cd 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> >          } else {
> >              maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >              sockets = maxcpus / (cores * threads);
> > +            sockets = sockets > 0 ? sockets : 1;
> >          }
> >      } else if (cores == 0) {
> >          threads = threads > 0 ? threads : 1;
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index a9b22fdc01..a44511c937 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> >          } else {
> >              maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >              sockets = maxcpus / (dies * cores * threads);
> > +            sockets = sockets > 0 ? sockets : 1;
> >          }
> >      } else if (cores == 0) {
> >          threads = threads > 0 ? threads : 1;
> > -- 
> > 2.19.1
> > 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets
Posted by wangyanan (Y) 4 years, 7 months ago 
On 2021/7/12 23:30, Daniel P. Berrangé wrote:
> On Mon, Jul 12, 2021 at 05:00:48PM +0200, Andrew Jones wrote:
>> On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
>>> We currently perform zero-check (default the value to 1 if zeroed)
>>> for the computed values of cores/threads, to make sure they are at
>>> least 1. For consistency, we probably should also default sockets
>>> to 1 if the computed value is zero. Note that this won't affect
>>> any existing working cmdlines but will improve the error reporting
>>> of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".
>> How does this help error checking? If the user input values that compute a
>> fractional (rounded down to zero with integer division) value, then we'll
>> catch that with the sockets*cores*threads == maxcpus test now, but we may
>> not catch that after this patch.
> Since we're having alot of debates about what should be valid scenarios
> vs invalid scenarios, I think this points towards introducing a tests
> for the smp_parse function, that enumerates both the correct and
> incorrect scenarios based on the current implementation.
I think so! Actually I'm already working on a simple QEMU unit test
(test-smp-parse.c) for smp parsing and plan to post it in v2. But it
will directly test the finally improved generic parser.

Thanks,
Yanan
.
> This series should then update the test cases for scenarios that we
> think are currently wrongly handled.
>
>> Thanks,
>> drew
>>
>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>> ---
>>>   hw/core/machine.c | 1 +
>>>   hw/i386/pc.c      | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index f17bbe3275..1e194677cd 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>>           } else {
>>>               maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>>               sockets = maxcpus / (cores * threads);
>>> +            sockets = sockets > 0 ? sockets : 1;
>>>           }
>>>       } else if (cores == 0) {
>>>           threads = threads > 0 ? threads : 1;
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index a9b22fdc01..a44511c937 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>>           } else {
>>>               maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>>               sockets = maxcpus / (dies * cores * threads);
>>> +            sockets = sockets > 0 ? sockets : 1;
>>>           }
>>>       } else if (cores == 0) {
>>>           threads = threads > 0 ? threads : 1;
>>> -- 
>>> 2.19.1
>>>
> Regards,
> Daniel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.