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 <drjo...@redhat.com>
>
> 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
