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

[David Gibson]

On Thu, Sep 21, 2017 at 09:42:26AM +0200, Igor Mammedov wrote:
> On Thu, 21 Sep 2017 08:04:55 +0200
> Cédric Le Goater <address@hidden> wrote:
> 
> > On 09/21/2017 05:54 AM, Nikunj A Dadhania wrote:
> > > David Gibson <address@hidden> writes:
> > >   
> > >> On Wed, Sep 20, 2017 at 12:48:55PM +0530, 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.  
> > >>
> > >> Right, so, -smp 4 defaults to 4 sockets, each with 1 core of 1
> > >> thread.  If you can't supply 4 sockets you should error, but you
> > >> shouldn't go and change the number of cores per socket.  
> > > 
> > > OK, that makes sense now. And I do see that smp_cpus is 4 in the above
> > > case. Now looking more into it, i see that powernv has something called
> > > "num_chips", isnt this same as sockets ? Do we need num_chips separately? 
> > >  
> > 
> > yes that would do for cpus, but how do we retrieve the number of 
> > sockets ? I don't see a smp_sockets. 
> I'd suggest to rewrite QEMU again :)
> 
> more exactly, -smp parsing is global and sometimes doesn't suite
> target device model/machine.
> Idea was to make it's options machine properties to get rid of globals
> and then let leaf machine redefine parsing behaviour.
> here is Drew's take on it:
> 
> [Qemu-devel] [PATCH RFC 00/16] Rework SMP parameters
> https://www.mail-archive.com/address@hidden/msg376961.html
> 
> considering there weren't pressing need, the series has been pushed
> to the end of TODO list. Maybe you can revive it and make work for
> pnv and other machines.

Right, making the core smp parsing more flexible might be good.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature