| Message ID | 1415376280-14130-3-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 07, 2014 at 05:04:39PM +0100, Andrew Jones wrote: > smp_parse allows partial or complete cpu topology to be given. > In either case there may be inconsistencies in the input which > are currently not sounding any alarms. In some cases the input > is even being silently corrected. We shouldn't do this. Add > warnings when input isn't adding up right, and even abort when > the complete cpu topology has been input, but isn't correct. > > Signed-off-by: Andrew Jones <drjones@redhat.com> So, we are fixing bugs and changing behavior on two different cases here: 1) when all options are provided and they aren't enough for smp_cpus; 2) when one option was missing, but the existing calculation was incorrect because of division truncation. I don't think we need to keep compatibility on (1) because the user is obviously providing an invalid configuration. That's why I suggested we implemented it in 2.2. And it is safer because we won't be silently changing behavior: QEMU is going to abort and the mistake will be easily detected. But (2) is fixing a QEMU bug, not user error. The user may be unaware of the bug, and will get a silent ABI change once upgrading to a newer QEMU. I suggest fixing only (1) by now and keeping the behavior for (2) on QEMU 2.2. Something like: if (sockets == 0) { /* keep existing code for sockets == 0 */ } else if (cores == 0) { /* keep existing code for cores == 0 */ } else if (threads == 0) { /* keep existing code for threads == 0 */ } else { /* new code: */ if (sockets * cores * threads < cpus) { fprintf(stderr, "cpu topology: error: " "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n", sockets, cores, threads, cpus); exit(1); } } > --- > vl.c | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/vl.c b/vl.c > index 9d9855092ab4a..c62fe29aa8075 100644 > --- a/vl.c > +++ b/vl.c > @@ -1288,16 +1288,35 @@ static void smp_parse(QemuOpts *opts) > if (cores == 0) { > threads = threads > 0 ? threads : 1; > cores = cpus / (sockets * threads); > - } else { > - threads = cpus / (cores * sockets); > + if (cpus % (sockets * threads)) { > + fprintf(stderr, "cpu topology: warning: " > + "Calculation results in fractional cores number. " > + "Adjusting.\n"); > + cores += 1; > + } > + } else if (threads == 0) { > + threads = cpus / (sockets * cores); > + if (cpus % (sockets * cores)) { > + fprintf(stderr, "cpu topology: warning: " > + "Calculation results in fractional threads number. " > + "Adjusting.\n"); > + threads += 1; > + } > } > } > > + if (sockets * cores * threads < cpus) { > + fprintf(stderr, "cpu topology: error: " > + "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n", > + sockets, cores, threads, cpus); > + exit(1); > + } > + > max_cpus = qemu_opt_get_number(opts, "maxcpus", 0); > > smp_cpus = cpus; > - smp_cores = cores > 0 ? cores : 1; > - smp_threads = threads > 0 ? threads : 1; > + smp_cores = cores; > + smp_threads = threads; > > } > > -- > 1.9.3 > >
diff --git a/vl.c b/vl.c index 9d9855092ab4a..c62fe29aa8075 100644 --- a/vl.c +++ b/vl.c @@ -1288,16 +1288,35 @@ static void smp_parse(QemuOpts *opts) if (cores == 0) { threads = threads > 0 ? threads : 1; cores = cpus / (sockets * threads); - } else { - threads = cpus / (cores * sockets); + if (cpus % (sockets * threads)) { + fprintf(stderr, "cpu topology: warning: " + "Calculation results in fractional cores number. " + "Adjusting.\n"); + cores += 1; + } + } else if (threads == 0) { + threads = cpus / (sockets * cores); + if (cpus % (sockets * cores)) { + fprintf(stderr, "cpu topology: warning: " + "Calculation results in fractional threads number. " + "Adjusting.\n"); + threads += 1; + } } } + if (sockets * cores * threads < cpus) { + fprintf(stderr, "cpu topology: error: " + "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n", + sockets, cores, threads, cpus); + exit(1); + } + max_cpus = qemu_opt_get_number(opts, "maxcpus", 0); smp_cpus = cpus; - smp_cores = cores > 0 ? cores : 1; - smp_threads = threads > 0 ? threads : 1; + smp_cores = cores; + smp_threads = threads; }
smp_parse allows partial or complete cpu topology to be given. In either case there may be inconsistencies in the input which are currently not sounding any alarms. In some cases the input is even being silently corrected. We shouldn't do this. Add warnings when input isn't adding up right, and even abort when the complete cpu topology has been input, but isn't correct. Signed-off-by: Andrew Jones <drjones@redhat.com> --- vl.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)