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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]

On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <david@gibson.dropbear.id.au> 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.  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.

> > But overriding smp_cores with a different variable that's the "real"
> > number of cores is not acceptable. If that means the user has to
> > specify cores explicitly, so be it.
> 
> Right, we would error out in case there is mismatch.
> 
> > Slight awkwardness in command line is preferable to breaking the
> > assumption that smp_cores == (# of cores per next level up cpu object)
> > which is used all over the place.
> 
> Regards
> Nikunj
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]