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

[wangyanan (Y)]

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 ? 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;
}

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