On Thu, 19 Sep 2024 14:11:21 +0800
Zhao Liu <zhao1.liu@xxxxxxxxx> 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@xxxxxxxxx>
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;
> +}
