| Message ID | 1465580427-13596-3-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <drjones@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
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;
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 <drjones@redhat.com> --- vl.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) -- 2.4.11