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

[David Gibson]

On Thu, Sep 21, 2017 at 09:24:46AM +0530, 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?

Ah, yes, I see.  It's probably still reasonable to keep num_chips as
an internal variable, rather than using (smp_cpus / smp_cores /
smp_threads) everywhere.  But we shouldn't have it as a direct
user-settable property, instead setting it from the -smp command line
option.

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