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

[Andrew Jones]

On Fri, Nov 07, 2014 at 10:22:39AM +0100, Andrew Jones wrote:
> 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.

Actually, scratch most of this. In the absence of an input max_cpus, we
should use smp_cpus, as we don't necessarily want to support hotplug for
this machine instance at all. However the sanity check against
machine_class->max_cpus needs to be updated to check against max_cpus,
not smp_cpus in order to be correct.

drew