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

[Andrew Jones]

On Thu, Nov 06, 2014 at 05:17:44PM -0200, Eduardo Habkost wrote:
> 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".

Will hotplug still work correctly with this meaning? I expect that
we need holes to fill.

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

True. I should have preserved that behavior.

The current code actually ends up with

  sockets=1 cores=3 threads=2 cpus=30 maxcpus=30

in smp_parse(), which is nonsense, but as the sockets count isn't saved
(it's always derivable from smp_cpus, cores, threads), then in practice
it doesn't matter.

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

I should have added an underscore to maxcpus in that error message, as
max_cpus is either maxcpus or the result of the topology now.

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

I guess the main questions are:
  - do we need to make this change for hotplug?
  - do we need to make this change to make the command line more
    intuitive?
 
> 
> (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
>

yeah, these work fine. But maybe they shouldn't... It seems to me
that if any topology is specified then the whole topology should
be specified. And, specifying both topology and maxcpus is redundant.
So we only need/should allow

-smp <n>                                        # smp_cpus = max_cpus = <n>
-smp maxcpus=<m>                                # smp_cpus = max_cpus = <m>
-smp sockets=<s>,cores=<c>,threads=<t>          # smp_cpus = max_cpus = 
<s>*<c>*<t>

# hotplug support: smp_cpus = <n>, max_cpus = <m>
-smp <n>,maxcpus=<m>

# hotplug support: smp_cpus = <n>, max_cpus = <s>*<c>*<t>
-smp <n>,sockets=<s>,cores=<c>,threads=<t>

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

I guess not, see my opinion above.

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

As said above, it should be max_cpus.

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

The current code doesn't abort, as there's no final sanity check.


There's another max_cpus issue I thought of after posting. I think
max_cpus should be initialized to machine_class->max_cpus. Then, for the
case we only have '-smp <n>' input by the user (assuming <n> <=
max_cpus and we don't error out), max_cpus should remain equal to
machine_class->max_cpus, keeping holes for hotplug. Currently it would
get set to smp_cpus.

drew