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

[wangyanan (Y)]

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