Re: [Qemu-arm] [Qemu-devel] [PATCH RFC 02/16] vl: smp: add checks for maxcpus based topologies

[Andrew Jones]

On Tue, Jun 14, 2016 at 11:28:52AM +1000, David Gibson wrote:
> On Fri, Jun 10, 2016 at 07:40:13PM +0200, Andrew Jones wrote:
> > smp_parse computes missing smp options. Unfortunately cores and
> > threads are computed by dividing smp_cpus, instead of max_cpus.
> > This is incorrect because the topology doesn't leave room for
> > hotplug. More unfortunately, we can't change it easily, as doing
> > so would impact existing command lines. This patch adds a warning
> > when the topology doesn't add up, and then checks that the topology
> > at least computes when sockets are recalculated. If not, then it
> > does fail.
> > 
> > Adding the new failure is justified by the fact that we don't
> > store the number of input sockets, and thus all consumers of
> > cpu topology information recalculate it. If they choose to
> > (correctly) calculate it based on maxcpus, then we need to
> > guard them against building topologies which provide more cpu
> > slots than are the maximum allowed cpus.
> > 
> > Signed-off-by: Andrew Jones <address@hidden>
> 
> Hmm.. this makes sense to me.  Except that I've never been clear if
> sockets= was supposed to match initial cpus or maxcpus.

The topology we describe in DT and ACPI (at least ACPI) must describe
all possible cpus (maxcpus).

Thanks,
drew

> 
> > ---
> >  vl.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7b96e787922f9..8d482cb1bf020 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1227,6 +1227,7 @@ static void smp_parse(QemuOpts *opts)
> >          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);
> > +        bool sockets_input = sockets > 0;
> >  
> >          /* compute missing values, prefer sockets over cores over threads 
> > */
> >          if (cpus == 0 || sockets == 0) {
> > @@ -1269,6 +1270,24 @@ static void smp_parse(QemuOpts *opts)
> >              exit(1);
> >          }
> >  
> > +        if (sockets_input && sockets * cores * threads != max_cpus) {
> > +            unsigned sockets_rounded = DIV_ROUND_UP(max_cpus, cores * 
> > threads);
> > +
> > +            error_report("warning: cpu topology: "
> > +                         "sockets (%u) * cores (%u) * threads (%u) != "
> > +                         "maxcpus (%u). Trying sockets=%u.",
> > +                         sockets, cores, threads, max_cpus, 
> > sockets_rounded);
> > +            sockets = sockets_rounded;
> > +
> > +            if (sockets * cores * threads > max_cpus) {
> > +                error_report("cpu topology: "
> > +                             "sockets (%u) * cores (%u) * threads (%u) > "
> > +                             "maxcpus (%u)",
> > +                             sockets, cores, threads, max_cpus);
> > +                exit(1);
> > +            }
> > +        }
> > +
> >          smp_cpus = cpus;
> >          smp_cores = cores;
> >          smp_threads = threads;
> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson