Re: [Qemu-devel] [PATCH] ppc/pnv: fix cores per chip for multiple cpus

[Cédric Le Goater]

On 09/20/2017 09:18 AM, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
>> On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote:
>>> David Gibson <address@hidden> writes:
>>>
>>>> On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote:
>>>>> David Gibson <address@hidden> writes:
>>>>>
>>>>>> On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote:
>>>>>>> David Gibson <address@hidden> writes:
>>>>>>>
>>>>>>>> On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>> David Gibson <address@hidden> writes:
>>>>>>>>>
>>>>>>>>>> On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
>>>>>>>>>>> David Gibson <address@hidden> writes:
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I thought, I am doing the same here for PowerNV, number of online 
>>>>>>>>>>>>> cores
>>>>>>>>>>>>> is equal to initial online vcpus / threads per core
>>>>>>>>>>>>>
>>>>>>>>>>>>>    int boot_cores_nr = smp_cpus / smp_threads;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Only difference that I see in PowerNV is that we have multiple 
>>>>>>>>>>>>> chips
>>>>>>>>>>>>> (max 2, at the moment)
>>>>>>>>>>>>>
>>>>>>>>>>>>>         cores_per_chip = smp_cpus / (smp_threads * 
>>>>>>>>>>>>> pnv->num_chips);
>>>>>>>>>>>>
>>>>>>>>>>>> This doesn't make sense to me.  Cores per chip should *always* 
>>>>>>>>>>>> equal
>>>>>>>>>>>> smp_cores, you shouldn't need another calculation for it.
>>>>>>>>>>>>
>>>>>>>>>>>>> And in case user has provided sane smp_cores, we use it.
>>>>>>>>>>>>
>>>>>>>>>>>> If smp_cores isn't sane, you should simply reject it, not try to 
>>>>>>>>>>>> fix
>>>>>>>>>>>> it.  That's just asking for confusion.
>>>>>>>>>>>
>>>>>>>>>>> This is the case where the user does not provide a topology(which 
>>>>>>>>>>> is a
>>>>>>>>>>> valid scenario), not sure we should reject it. So qemu defaults
>>>>>>>>>>> smp_cores/smt_threads to 1. I think it makes sense to over-ride.
>>>>>>>>>>
>>>>>>>>>> If you can find a way to override it by altering smp_cores when it's
>>>>>>>>>> not explicitly specified, then ok.
>>>>>>>>>
>>>>>>>>> Should I change the global smp_cores here as well ?
>>>>>>>>
>>>>>>>> I'm pretty uneasy with that option.
>>>>>>>
>>>>>>> Me too.
>>>>>>>
>>>>>>>> It would take a fair bit of checking to ensure that changing smp_cores
>>>>>>>> is safe here. An easier to verify option would be to make the generic
>>>>>>>> logic which splits up an unspecified -smp N into cores and sockets
>>>>>>>> more flexible, possibly based on machine options for max values.
>>>>>>>>
>>>>>>>> That might still be more trouble than its worth.
>>>>>>>
>>>>>>> I think the current approach is the simplest and less intrusive, as we
>>>>>>> are handling a case where user has not bothered to provide a detailed
>>>>>>> topology, the best we can do is create single threaded cores equal to
>>>>>>> number of cores.
>>>>>>
>>>>>> No, sorry.  Having smp_cores not correspond to the number of cores per
>>>>>> chip in all cases is just not ok.  Add an error message if the
>>>>>> topology isn't workable for powernv by all means.  But users having to
>>>>>> use a longer command line is better than breaking basic assumptions
>>>>>> about what numbers reflect what topology.
>>>>>
>>>>> Sorry to ask again, as I am still not convinced, we do similar
>>>>> adjustment in spapr where the user did not provide the number of cores,
>>>>> but qemu assumes them as single threaded cores and created
>>>>> cores(boot_cores_nr) that were not same as smp_cores ?
>>>>
>>>> What?  boot_cores_nr has absolutely nothing to do with adjusting the
>>>> topology, and it certainly doesn't assume they're single threaded.
>>>
>>> When we start a TCG guest and user provides following commandline, e.g.
>>> "-smp 4", smt_threads is set to 1 by default in vl.c. So the guest boots
>>> with 4 cores, each having 1 thread.
>>
>> Ok.. and what's the problem with that behaviour on powernv?
> 
> As smp_thread defaults to 1 in vl.c, similarly smp_cores also has the
> default value of 1 in vl.c. In powernv, we were setting nr-cores like
> this:
> 
>         object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal);
> 
> Even when there were multiple cpus (-smp 4), when the guest boots up, we
> just get one core (i.e. smp_cores was 1) with single thread(smp_threads
> was 1), which is wrong as per the command-line that was provided.

I have never noticed as always use the cores= option but if you use
the following on a powernv machine:

 -smp 4                         1 cpu
 -smp cpus=4,threads=1          1 cpu
 -smp cores=4,threads=1         4 cpus
 -smp cpus=4,cores=4,threads=1  4 cpus
 -smp cpus=1,cores=4,threads=1  fails
 -smp cpus=4,cores=1,threads=1  1 cpu

Should we be using 'smp_cpus' instead of 'smp_cores' then ? Honestly,
I feel a bit lost with all default behaviors.

C.