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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Thu, 19 Sep 2024 14:11:21 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> Custom topology allows user to create CPU topology totally via -device
> from CLI.
> 
> Once custom topology is enabled, machine will stop the default CPU
> creation and expect user's CPU topology tree built from CLI.
> 
> With custom topology, any CPU topology, whether symmetric or hybrid
> (aka, heterogeneous), can be created naturally.
> 
> However, custom topology also needs to be restricted because
> possible_cpus[] requires some preliminary topology information for
> initialization, which is the max limitation (the new max parameters in
> -smp). Custom topology will be subject to this max limitation.
> 
> Max limitations are necessary because creating custom topology before
> initializing possible_cpus[] would compromise future hotplug scalability.
> 
> Max limitations are placed in -smp, even though custom topology can be
> defined as hybrid. From an implementation perspective, any hybrid
> topology can be considered a subset of a complete SMP structure.
> Therefore, semantically, using max limitations to constrain hybrid
> topology is consistent.
> 
> Introduce custom CPU topology related properties in MachineClass. At the
> same time, add and parse max parameters from -smp, and store the max
> limitations in CPUSlot.
> 
> Signed-off-by: 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..


    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.

> +
> +    is_valid = config->has_maxdies && config->maxdies;
> +    if (mc->smp_props.custom_topo_supported &&
> +        mc->smp_props.dies_supported) {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_DIE].max_limit =
> +            is_valid ? config->maxdies : ms->smp.dies;
> +    } else if (is_valid) {
> +        error_setg(errp, "maxdies > 0 not supported "
> +                   "by this machine's CPU topology");
> +        return false;
> +    } else {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_DIE].max_limit =
> +            ms->smp.dies;
> +    }
> +
> +    is_valid = config->has_maxmodules && config->maxmodules;
> +    if (mc->smp_props.custom_topo_supported &&
> +        mc->smp_props.modules_supported) {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_MODULE].max_limit =
> +            is_valid ? config->maxmodules : ms->smp.modules;
> +    } else if (is_valid) {
> +        error_setg(errp, "maxmodules > 0 not supported "
> +                   "by this machine's CPU topology");
> +        return false;
> +    } else {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_MODULE].max_limit =
> +            ms->smp.modules;
> +    }
> +
> +    is_valid = config->has_maxcores && config->maxcores;
> +    if (mc->smp_props.custom_topo_supported) {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_CORE].max_limit =
> +            is_valid ? config->maxcores : ms->smp.cores;
> +    } else if (is_valid) {
> +        error_setg(errp, "maxcores > 0 not supported "
> +                   "by this machine's CPU topology");
> +        return false;
> +    } else {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_CORE].max_limit =
> +            ms->smp.cores;
> +    }
> +
> +    is_valid = config->has_maxthreads && config->maxthreads;
> +    if (mc->smp_props.custom_topo_supported) {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_THREAD].max_limit =
> +            is_valid ? config->maxthreads : ms->smp.threads;
> +    } else if (is_valid) {
> +        error_setg(errp, "maxthreads > 0 not supported "
> +                   "by this machine's CPU topology");
> +        return false;
> +    } else {
> +        slot->stat.entries[CPU_TOPOLOGY_LEVEL_THREAD].max_limit =
> +            ms->smp.threads;
> +    }
> +
> +    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?

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

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