Re: [Qemu-devel] [PATCH] vl: rework smp_parse

[Eduardo Habkost]

On Thu, Nov 06, 2014 at 05:09:35PM +0100, Andrew Jones wrote:
> smp_parse has a couple problems. First, it should use max_cpus,
> not smp_cpus when calculating missing topology information.
> Conversely, if maxcpus is not input, then the topology should
> dictate max_cpus, as the topology may support more than the
> input smp_cpus number. Second, smp_parse shouldn't silently
> adjust the number of threads a user provides, which it currently
> does in order to ensure the topology supports the number
> of cpus provided. smp_parse should rather complain and exit
> when input isn't consistent. This patch fixes those issues and
> attempts to clarify the code a bit too.
> 
> I don't believe we need to consider compatibility with the old
> behaviors. Specifying something like
> 
>   -smp 4,sockets=1,cores=1,threads=1
> 
> is wrong, even though it currently works (the number of threads
> is silently adjusted to 4).

Agreed, in this case.

> And, specifying something like
> 
>   -smp 4,sockets=1,cores=4,threads=1,maxcpus=8
> 
> is also wrong, as there's no way to hotplug the additional 4 cpus
> with this topology. So, any users doing these things should be
> corrected. The new error message this patch provides should help
> them do that.

In this case, you are proposing a new meaning for "sockets", and it
makes sense (I as suppose people understand "sockets" to mean all
sockets, even the empty ones). But it looks like it will break
compatility on cases we don't want to.

For example, the case below is currently valid, and probably can be
generated by libvirt today. But you are now rejecting it:

 -smp 30,sockets=5,cores=3,threads=2,maxcpus=60   "cpu topology: sockets (5) * 
cores (3) * threads (2) != max_cpus (60)"

This is currently valid because "sockets" today means "online sockets",
not "all sockets, even the empty ones".


It looks like the patch also breaks automatic socket count calculation,
which (AFAIK) works today:

I get:
 -smp 30,cores=3,threads=2        "maxcpus must be equal to or greater than smp"
but I expect:
 -smp 30,cores=3,threads=2        sockets=5 cores=3 threads=2 smp_cpus=30 
max_cpus=30

(And the error message is confusing: I am not even setting max_cpus, why
is it saying maxcpus is wrong?)


> 
> Below are some examples with before/after results
> 
> // topology should support up to max_cpus
> < -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=2 threads=1 
> smp_cpus=4 max_cpus=8
> > -smp 4,sockets=2,maxcpus=8                    sockets=2 cores=4 threads=1 
> > smp_cpus=4 max_cpus=8

So, like in my first example above, we are breaking compatibility here
because the meaning of "sockets" changed. Do we want to?


(Unless otherwise noted in my other comments below, I am using the new
meaning for "sockets", not the old one.)

> 
> // topology supports more than smp_cpus, max_cpus should be higher
> < -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 
> smp_cpus=4 max_cpus=4
> > -smp 4,sockets=4,cores=2                      sockets=4 cores=2 threads=1 
> > smp_cpus=4 max_cpus=8

There are also existing cases where the user could simply expect
smp_threads to be calculated automatically. You seem to cover that, but
explicit testing would be nice. e.g., to make sure we won't break stuff
like:

 -smp  8,sockets=2,cores=2               sockets=2 cores=2 threads=2 smp_cpus=8 
max_cpus=8
 -smp 12,sockets=2,cores=2               sockets=2 cores=2 threads=3 
smp_cpus=12 max_cpus=12


Anyway, I see a problem here because we are trying to automatically
calculate two variables (max_cpus and smp_threads) when multiple
solutions may exist. We are just trying to choose the solution that
makes more sense, I guess, but do we really want to go down that path?

e.g. what about this one:
  -smp 6,sockets=2,cores=2

It has a solution:
  sockets=2 cores=2 threads=3 smp_cpus=6 max_cpus=12
but should the code find it?

> 
> // shouldn't silently adjust threads
> < -smp 4,sockets=1,cores=2,threads=1            sockets=1 cores=2 threads=2 
> smp_cpus=4 max_cpus=4
> > -smp 4,sockets=1,cores=2,threads=1            "maxcpus must be equal to or 
> > greater than smp"

I find it very confusing that the error message mentions "maxcpus" when
it was never used in the command-line. But I agree we should reject the
above configuration, as we must never silently adjust any option that
was explicitly set.

> < -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  sockets=2 cores=2 threads=1 
> smp_cpus=4 max_cpus=8
> > -smp 4,sockets=2,cores=2,threads=4,maxcpus=8  "cpu topology: sockets (2) * 
> > cores (2) * threads (4) != max_cpus (8)"

In this specific case I would expect it to abort too, because both
max_cpus (8) and smp_cpus (4) don't match sockets*cores*threads (16).

-- 
Eduardo