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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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

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

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

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

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.

Paolo