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

[Andrew Jones]

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

I agree requiring all these properties means a lot of typing, but I'm
not sure we want to calculate defaults. Reasoning below

> 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

> 
> This is a simple, linear logic that assigns defaults to each argument in
> sequence.  At the same time -machine should fail for all invalid
> combinations, namely:
> 
> - 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.

> - 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,
but I guess that's not how things currently work. Maybe Igor's
work is changing that?

> - cpus > sockets * cores * threads
> - maxcpus != cores * threads * sockets

We check these two (the 2nd added with this series) already.

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

> 
> This will catch situations where assigning a default for sockets doesn't
> work well.  For example cores=2,cpus=4,maxcpus=5 will assign
> threads=1,sockets=2; then the last relation will fail and QEMU will give
> an error.
> 
> These invalid combination also make sure that other useful default cases
> work.  For example "maxcpus given, sockets and cpus not" works thanks to
> the following deduction:
> 
> - sockets = cpus/(cores*threads)
> - maxcpus = cores*threads*sockets
> - because cpus%(cores*threads) must be 0, this gives maxcpus=cpus
> 
> -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. I'm still leaning towards nothing, but
at least maxcpus and cpus could be optional, making it just 3 required
inputs. And, machines are free to override this, allowing -machine
MACHINE,cpus=N to generate their default topology supporting N cpus,
maxcpus also =N. Or, -machine MACHINE,maxcpus=N,cpus=M for their common
hotplug-enabling case.

> 
> Apart from this point, these are lovely patches. :)

Thanks!

drew