Re: [RFC v2 05/12] hw/core/machine: Introduce custom CPU topology with max limitations

[Zhao Liu <zhao1.liu@intel.com>]

> A few code style comments inline.
> 
> J
> > diff --git a/hw/cpu/cpu-slot.c b/hw/cpu/cpu-slot.c
> > index 1cc3b32ed675..2d16a2729501 100644
> > --- a/hw/cpu/cpu-slot.c
> > +++ b/hw/cpu/cpu-slot.c
> 
> > +
> > +bool machine_parse_custom_topo_config(MachineState *ms,
> > +                                      const SMPConfiguration *config,
> > +                                      Error **errp)
> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +    CPUSlot *slot = ms->topo;
> > +    bool is_valid;
> > +    int maxcpus;
> > +
> > +    if (!slot) {
> > +        return true;
> > +    }
> > +
> > +    is_valid = config->has_maxsockets && config->maxsockets;
> > +    if (mc->smp_props.custom_topo_supported) {
> > +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> > +            is_valid ? config->maxsockets : ms->smp.sockets;
> > +    } else if (is_valid) {
> > +        error_setg(errp, "maxsockets > 0 not supported "
> > +                   "by this machine's CPU topology");
> > +        return false;
> > +    } else {
> > +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
> > +            ms->smp.sockets;
> > +    }
> Having the error condition in the middle is rather confusing to
> read to my eyes. Playing with equivalents I wonder what works best..

Figuring out how to clearly express the logic here was indeed a bit of a
struggle for me at first. :-)

>     if (!is_valid) {
>         slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
>             ms->smp.sockets;
>     } else if (mc->smp_props.custom_topo_supported) {
>         slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
>             config->max_sockets;
>     } else {
>         error_setg...
>         return false;
>     }
> 
> or take the bad case out first.  Maybe this is a little obscure
> though (assuming I even got it right) as it relies on the fact
> that is_valid must be false for the legacy path.
> 
>     if (!mc->smp_props.custom_topo_supported && is_valid) {
>         error_setg();
>         return false;
>     }
> 
>     slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit =
>           is_valid ? config->maxsockets : ms->smp.sockets;
> 
> Similar for other cases.

I prefer the first style, as it's more natural and clear enough!

Many thanks!

[snip]

> > +    maxcpus = 1;
> > +    /* Initizlize max_limit to 1, as members of CpuTopology. */
> > +    for (int i = 0; i < CPU_TOPOLOGY_LEVEL__MAX; i++) {
> > +        maxcpus *= slot->stat.entries[i].max_limit;
> > +    }
> > +
> > +    if (!config->has_maxcpus) {
> > +        ms->smp.max_cpus = maxcpus;
> Maybe early return here to get rid of need for the else?

Yes, it's better to reduce else.

> > +    } else {
> > +        if (maxcpus != ms->smp.max_cpus) {
> 
> Unless this is going to get more complex later,  else if probably appropriate here
> (if you don't drop the else above.


I can organize it like:

if (!config->has_maxcpus) {
    ...
    return true;
}

if (maxcpus != ms->smp.max_cpus) {
    error_steg...
    return false;
}

return true;

As you suggested to get rid of a "else". :)

> > +            error_setg(errp, "maxcpus (%d) should be equal to "
> > +                       "the product of the remaining max parameters (%d)",
> > +                       ms->smp.max_cpus, maxcpus);
> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}

Regards,
Zhao