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

[Daniel P . Berrangé]

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