Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology

[Eduardo Habkost <ehabkost@redhat.com>]

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
> 
> 

-- 
Eduardo