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

[David Gibson]

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.

boot_cores_nr is simply the number of cores (each of smp_threads
threads) which are online initially.  In an sPAPR system there are
max_cpus total potential threads.  Those are divided into cores each
with smp_threads threads (so max_cpus / smp_threads total cores), and
those cores are divided into sockets each with smp_cores sockets (so
max_cpus / smp_threads / smp_cores total sockets).  Of all those
potential threads smp_cpus are initially online (the rest can be
hotplugged later), so there are smp_cpus / smp_threads cores initially
online.

We need that calculation because we can only hotplug cpus on spapr at
core granularity, not thread granularity (x86 can do that).  If
smp_cpus is not a multiple of smp_threads we give an error (except for
old machine types, where we have some hacks for backwards compat).

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