Re: [PATCH for-6.2 v2 02/11] machine: Make smp_parse generic enough for all arches

[wangyanan (Y)]

On 2021/7/20 0:53, Daniel P. Berrangé wrote:
> On Mon, Jul 19, 2021 at 11:20:34AM +0800, Yanan Wang wrote:
> > Currently the only difference between smp_parse and pc_smp_parse
> > is the support of multi-dies and the related error reporting code.
> > With an arch compat variable "bool smp_dies_supported", we can
> > easily make smp_parse generic enough for all arches and the PC
> > specific one can be removed.
> >
> > Making smp_parse() generic enough can reduce code duplication and
> > ease the code maintenance, and also allows extending the topology
> > with more arch specific members (e.g., clusters) in the future.
> >
> > No functional change intended.
> >
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >   hw/core/machine.c   | 28 ++++++++++-------
> >   hw/i386/pc.c        | 76 +--------------------------------------------
> >   include/hw/boards.h |  1 +
> >   3 files changed, 19 insertions(+), 86 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index d73daa10f4..ed6712e964 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -743,6 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
> >   
> >   static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> >   {
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> >       unsigned cpus    = config->has_cpus ? config->cpus : 0;
> >       unsigned sockets = config->has_sockets ? config->sockets : 0;
> >       unsigned dies    = config->has_dies ? config->dies : 1;
> > @@ -761,7 +762,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> > *config, Error **errp)
> >           return;
> >       }
> >   
> > -    if (dies > 1) {
> > +    if (!mc->smp_dies_supported && dies > 1) {
> >           error_setg(errp, "dies not supported by this machine's CPU topology");
> >           return;
> >       }
> > @@ -772,23 +773,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> > *config, Error **errp)
> >           threads = threads > 0 ? threads : 1;
> >           if (cpus == 0) {
> >               sockets = sockets > 0 ? sockets : 1;
> > -            cpus = cores * threads * sockets;
> > +            cpus = sockets * dies * cores * threads;
> >           } else {
> >               maxcpus = maxcpus > 0 ? maxcpus : cpus;
> > -            sockets = maxcpus / (cores * threads);
> > +            sockets = maxcpus / (dies * cores * threads);
> >           }
> >       } else if (cores == 0) {
> >           threads = threads > 0 ? threads : 1;
> > -        cores = cpus / (sockets * threads);
> > +        cores = cpus / (sockets * dies * threads);
> >           cores = cores > 0 ? cores : 1;
> >       } else if (threads == 0) {
> > -        threads = cpus / (cores * sockets);
> > +        threads = cpus / (sockets * dies * cores);
> >           threads = threads > 0 ? threads : 1;
> > -    } else if (sockets * cores * threads < cpus) {
> > +    } else 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) * cores (%u) * threads (%u) < "
> > +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
> >                      "smp_cpus (%u)",
> > -                   sockets, cores, threads, cpus);
> > +                   sockets, dies_msg, cores, threads, cpus);
> Since we're allowing dies=1 (but not greater), I'm not convinced we
> need the conditionally built error message, and could just include
> "* dies" all the time.
>
> If we do want it to be conditionally different though, I'd just
> sugest calling error_setg twice. Although this duplicates stuff,
> it'll be clearer to read which I think is a net win.
The duplicates may increase quickly if more arch specific members
are introduced, I think the conditional error reporting may still be
necessary, but should be more clearer than current approach. :)

Thanks,
Yanan
.