Re: [RFC PATCH v3 03/13] hw/arm/virt: Replace smp_parse with one that prefers cores

[Andrew Jones]

On Mon, Nov 09, 2020 at 11:01:48AM +0000, Salil Mehta wrote:
> > From: fangying
> > Sent: Monday, November 9, 2020 3:05 AM
> > To: peter.maydell@linaro.org
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; drjones@redhat.com;
> > imammedo@redhat.com; shannon.zhaosl@gmail.com; alistair.francis@wdc.com;
> > Zhanghailiang <zhang.zhanghailiang@huawei.com>; Salil Mehta
> > <salil.mehta@huawei.com>
> > Subject: [RFC PATCH v3 03/13] hw/arm/virt: Replace smp_parse with one that
> > prefers cores
> > 
> > From: Andrew Jones <drjones@redhat.com>
> > 
> > The virt machine type has never used the CPU topology parameters, other
> > than number of online CPUs and max CPUs. When choosing how to allocate
> > those CPUs the default has been to assume cores. In preparation for
> > using the other CPU topology parameters let's use an smp_parse that
> > prefers cores over sockets. We can also enforce the topology matches
> > max_cpus check because we have no legacy to preserve.
> 
> 
> Hi Andrew,
> I am wondering if we need to take care of other levels of processor
> hierarchy as well in ARM64 like 'clusters'/'Dies'?

I don't know, but so far in Linux only x86 considers dies. Also, Linux
doesn't define the concept of a cluster in its arch-neutral cpu topology
API. With that in mind, I think we should just focus on sockets, cores,
and threads until we have a guest OS that can be used to test other
levels.

Thanks,
drew

> 
> Thanks
> 
> 
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/virt.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ea24b576c6..ba902b53ba 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -78,6 +78,8 @@
> >  #include "hw/virtio/virtio-iommu.h"
> >  #include "hw/char/pl011.h"
> >  #include "qemu/guest-random.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "sysemu/replay.h"
> > 
> >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > @@ -2444,6 +2446,79 @@ static int virt_kvm_type(MachineState *ms, const char
> > *type_str)
> >      return requested_pa_size > 40 ? requested_pa_size : 0;
> >  }
> > 
> > +/*
> > + * Unlike smp_parse() in hw/core/machine.c, we prefer cores over sockets,
> > + * e.g. '-smp 8' creates 1 socket with 8 cores.  Whereas '-smp 8' with
> > + * hw/core/machine.c's smp_parse() creates 8 sockets, each with 1 core.
> > + * Additionally, we can enforce the topology matches max_cpus check,
> > + * because we have no legacy to preserve.
> > + */
> > +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
> > +{
> > +    if (opts) {
> > +        unsigned cpus    = qemu_opt_get_number(opts, "cpus", 0);
> > +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> > +        unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
> > +        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> > +
> > +        /*
> > +         * Compute missing values; prefer cores over sockets and
> > +         * sockets over threads.
> > +         */
> > +        if (cpus == 0 || cores == 0) {
> > +            sockets = sockets > 0 ? sockets : 1;
> > +            threads = threads > 0 ? threads : 1;
> > +            if (cpus == 0) {
> > +                cores = cores > 0 ? cores : 1;
> > +                cpus = cores * threads * sockets;
> > +            } else {
> > +                ms->smp.max_cpus = qemu_opt_get_number(opts, "maxcpus", 
> > cpus);
> > +                cores = ms->smp.max_cpus / (sockets * threads);
> > +            }
> > +        } else if (sockets == 0) {
> > +            threads = threads > 0 ? threads : 1;
> > +            sockets = cpus / (cores * threads);
> > +            sockets = sockets > 0 ? sockets : 1;
> > +        } else if (threads == 0) {
> > +            threads = cpus / (cores * sockets);
> > +            threads = threads > 0 ? threads : 1;
> > +        } else if (sockets * cores * threads < cpus) {
> > +            error_report("cpu topology: "
> > +                         "sockets (%u) * cores (%u) * threads (%u) < "
> > +                         "smp_cpus (%u)",
> > +                         sockets, cores, threads, cpus);
> > +            exit(1);
> > +        }
> > +
> > +        ms->smp.max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > +
> > +        if (ms->smp.max_cpus < cpus) {
> > +            error_report("maxcpus must be equal to or greater than smp");
> > +            exit(1);
> > +        }
> > +
> > +        if (sockets * cores * threads != ms->smp.max_cpus) {
> > +            error_report("cpu topology: "
> > +                         "sockets (%u) * cores (%u) * threads (%u)"
> > +                         "!= maxcpus (%u)",
> > +                         sockets, cores, threads,
> > +                         ms->smp.max_cpus);
> > +            exit(1);
> > +        }
> > +
> > +        ms->smp.cpus = cpus;
> > +        ms->smp.cores = cores;
> > +        ms->smp.threads = threads;
> > +        ms->smp.sockets = sockets;
> > +    }
> > +
> > +    if (ms->smp.cpus > 1) {
> > +        Error *blocker = NULL;
> > +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> > +        replay_add_blocker(blocker);
> > +    }
> > +}
> > +
> >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -2469,6 +2544,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> > void
> > *data)
> >      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
> >      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
> >      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> > +    mc->smp_parse = virt_smp_parse;
> >      mc->kvm_type = virt_kvm_type;
> >      assert(!mc->get_hotplug_handler);
> >      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> > --
> > 2.23.0
> 
>