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 <drjo...@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
>
>
--
Eduardo
