Re: [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing

[Andrew Jones <drjones@redhat.com>]

On Thu, Jul 22, 2021 at 04:10:32PM +0800, wangyanan (Y) wrote:
> On 2021/7/20 0:53, Andrew Jones wrote:
> > On Mon, Jul 19, 2021 at 11:20:37AM +0800, Yanan Wang wrote:
> > > We totally have two requirements for a valid SMP configuration:
> > s/totally//
> > 
> > > the sum of "sockets * dies * cores * threads" must represent all
> > the product
> > 
> > > the possible cpus, i.e., max_cpus, and must include the initial
> > > present cpus, i.e., smp_cpus.
> > > 
> > > We only need to ensure "sockets * dies * cores * threads == maxcpus"
> > > at first and then ensure "sockets * dies * cores * threads >= cpus".
> > Or, "maxcpus >= cpus"
> > 
> > > With a reasonable order of the sanity-check, we can simplify the
> > > error reporting code.
> > > 
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >   hw/core/machine.c | 25 ++++++++++---------------
> > >   1 file changed, 10 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 668f0a1553..8b4d07d3fc 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -788,21 +788,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> > >       cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
> > >       maxcpus = maxcpus > 0 ? maxcpus : cpus;
> > > -    if (sockets * dies * cores * threads < cpus) {
> > > -        g_autofree char *dies_msg = g_strdup_printf(
> > > -            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> > > -        error_setg(errp, "cpu topology: "
> > > -                   "sockets (%u)%s * cores (%u) * threads (%u) < "
> > > -                   "smp_cpus (%u)",
> > > -                   sockets, dies_msg, cores, threads, cpus);
> > > -        return;
> > > -    }
> > > -
> > > -    if (maxcpus < cpus) {
> > > -        error_setg(errp, "maxcpus must be equal to or greater than smp");
> > > -        return;
> > > -    }
> > This may be redundant when determining a valid config, but by checking it
> > separately we can provide a more useful error message.
> Yes, this message is more useful. Can we also report the exact values of the
> parameters within this error message ?

sure

> How about the following:
> 
> if (sockets * cores * threads != maxcpus) {
>     error_setg("product of the topology must be equal to maxcpus"
>                "sockets (%u) * cores (%u) * threads (%u)"
>                "!= maxcpus (%u)",
>                sockets, cores, threads, maxcpus);
> return;
> }
> 
> if (maxcpus < cpus) {
>     error_setg("maxcpus must be equal to or greater than smp:"
>                "sockets (%u) * cores (%u) * threads (%u)"
>                "== maxcpus (%u) < smp_cpus (%u)",
>                sockets, cores, threads, maxcpus, cpus);
> return;
> }

OK by me

Thanks,
drew

> 
> Thanks,
> Yanan
> .
> > > -
> > >       if (sockets * dies * cores * threads != maxcpus) {
> > >           g_autofree char *dies_msg = g_strdup_printf(
> > >               mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> > > @@ -814,6 +799,16 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> > >           return;
> > >       }
> > > +    if (sockets * dies * cores * threads < cpus) {
> > > +        g_autofree char *dies_msg = g_strdup_printf(
> > > +            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> > > +        error_setg(errp, "Invalid CPU topology: "
> > > +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
> > > +                   "smp_cpus (%u)",
> > > +                   sockets, dies_msg, cores, threads, cpus);
> > > +        return;
> > > +    }
> > > +
> > >       ms->smp.cpus = cpus;
> > >       ms->smp.sockets = sockets;
> > >       ms->smp.dies = dies;
> > > -- 
> > > 2.19.1
> > > 
> > I'm not sure we need this patch.
> > 
> > Thanks,
> > drew
> > 
> > .
>