Re: [Qemu-devel] [PATCH RFC 06/16] vl: move smp parsing to machine pre_init

[Andrew Jones <drjones@redhat.com>]

On Tue, Jun 14, 2016 at 10:17:49AM +0200, Paolo Bonzini wrote:
> 
> 
> On 13/06/2016 22:35, Andrew Jones wrote:
> > On Mon, Jun 13, 2016 at 07:04:01PM +0200, Paolo Bonzini wrote:
> >> On 10/06/2016 19:40, Andrew Jones wrote:
> >>> +    if (sockets == -1 || cores == -1 || threads == -1 ||
> >>> +        maxcpus == -1 || cpus == -1) {
> >>> +        error_report("cpu topology: "
> >>> +                     "all machine properties must be specified");
> >>> +        exit(1);
> >>> +    }
> >>> +
> >>
> >> I think it's sane to accept some defaults.  It must not be the DWIM
> >> thing that -smp does (which is targeted to Windows's dislike of
> >> multi-socket machine on consumer hardware).  It must be something that
> >> makes sense, and my proposal is:
> >>
> >> - threads: 1
> >> - cores: 1
> >> - sockets:
> >>   - maxcpus / (cores * threads) if maxcpus given
> >>   - cpus / (cores * threads) if cpus given
> >>   - else 1
> >> - maxcpus: cores * threads * sockets
> >> - cpus: maxcpus
> > 
> > I think some machines may prefer
> > 
> > - threads: 1
> > - sockets: 1
> > - cores:
> >   - maxcpus / (sockets * threads) if maxcpus given
> >   - cpus / (sockets * threads) if cpus given
> >   - else 1
> 
> smp_cores is only used by pseries and x86 machines.  I expect machines
> that must be single-socket to disregard smp_sockets altogether.

mach-virt currently assumes 1 "socket" with N cores when -smp N is used.
This is because, until recently, ARM machines didn't define sockets. I
presume the mindset will remain that cpus=N means cores=N even after
introducing support for multi-socket topology.

> 
> >> - any argument < 1
> > 
> > What if a user says threads=0, because they don't have HT, and assume
> > that a value of zero will get them a non-HT system? Or what about
> > machines that don't describe sockets, so a user inputs zero? In both
> > those cases I agree we should just use 1, but from a user's perspective,
> > it might seem weird.
> 
> They should just not specify it and get a default of 1. ;)

Yeah, threads, the only one we should never calculate, could be
optional. If not specified, defaulting to 1 makes perfect sense.
But, threads=0, which is weird, but in a way specifying that it's
not specified, also makes some sense.

> 
> >> - any argument > some compile-time value (1024?) to avoid overflows
> > 
> > Agreed. We should do this regardless of this series.
> > 
> >> - cpus % (cores * threads) != 0
> > 
> > Hmm. This makes sense where cpus is the number of cpu packages,
> 
> I'm not sure I understand what you mean here.  The point is that the
> machine starts with an integral number of sockets.

OK, s/cpus/maxcpus/ then. By using the currently online number, I
thought you were starting to prepare for cpu packages, which are
indivisible sets of cores and threads. But now that I think about
it, cpus % (cores * threads) isn't right for that either. It should
be total-online-threads % (cores * threads) != 0

> 
> >> - cpus > sockets * cores * threads
> >> - maxcpus != cores * threads * sockets
> > 
> > We check these two (the 2nd added with this series) already.
> 
> Yup, I was just making a complete list.
> 
> >> Alone the last relation shows that requiring all four of maxcpus, cores,
> >> threads and sockets is unnecessary. :)
> > 
> > Not really. It depends on if you assume sockets, cores or threads to
> > be the N in maxcpus=N. We could just require sockets, cores, and threads
> > to be input, which allows us to easily calculate maxcpus, and then set
> > cpus from that. In that case maxcpus would just be an available input
> > for sanity checking.
> 
> Or you could just specify -machine cpus=16,maxcpus=32 and expect 1
> core/socket, 1 thread/core, and 16 to 32 sockets.

Or 32 cores/socket (only 16 populated), 1 thread/core

> 
> >> -smp should do its legacy magic and assign all five values based on it.
> >> If the results do not match the obvious s/smp/-machine/ command line it
> >> should warn, and perhaps suggest the equivalent -machine command line.
> >> It doesn't have to be a minimal command line, just equivalent.
> > 
> > This is a good idea. I'm just still not sure what the -machine command
> > line should/should not assume.
> 
> It's okay.  Adding defaults can be done later, as long as strict checks
> are in place from the beginning and there is a clean separation between
> -smp defaults and -machine.
> 
> Relaxing checks can be done later, so I am much more interested in
> having strict checks than in having defaults.

Yes, my current thinking (which is always adjustable :-) is to start
with strict checks, and maybe always have them in this common code.
Machines that override this can implement defaults based on their
machine-specific knowledge, allowing the user to get sane topologies
without really needing to know what they want.

> 
> Though I think that we can at least agree on defaults for threads,
> maxcpus and cpus.  The only sticky point is sockets vs. cores.  We can
> make them both mandatory for now.  That is: cores and sockets are
> mandatory until we decide which one to default to 1; threads is
> optional; cpus is mandatory if you want hotplug, otherwise it's
> redundant; maxcpus is optional and always redundant.

Agreed. I'll do the following for the next round of this series

 threads: 1
 cores:   required
 sockets: required
 maxcpus: maxcpus ? maxcpus : sockets * cores * threads
 cpus:    cpus ? cpus : maxcpus

If maxcpus is input, it's redundant, but should be sanity checked.
Maybe the user wants to use the QEMU cmdline to check their math...

Thanks,
drew