On Wed, Sep 20, 2017 at 12:48:55PM +0530, Nikunj A Dadhania wrote: > David Gibson <da...@gibson.dropbear.id.au> writes: > > > On Wed, Sep 20, 2017 at 12:10:48PM +0530, Nikunj A Dadhania wrote: > >> David Gibson <da...@gibson.dropbear.id.au> writes: > >> > >> > On Wed, Sep 20, 2017 at 10:43:19AM +0530, Nikunj A Dadhania wrote: > >> >> David Gibson <da...@gibson.dropbear.id.au> writes: > >> >> > >> >> > On Wed, Sep 20, 2017 at 09:50:24AM +0530, Nikunj A Dadhania wrote: > >> >> >> David Gibson <da...@gibson.dropbear.id.au> writes: > >> >> >> > >> >> >> > On Fri, Sep 15, 2017 at 02:39:16PM +0530, Nikunj A Dadhania wrote: > >> >> >> >> David Gibson <da...@gibson.dropbear.id.au> writes: > >> >> >> >> > >> >> >> >> > On Fri, Sep 15, 2017 at 01:53:15PM +0530, Nikunj A Dadhania > >> >> >> >> > wrote: > >> >> >> >> >> David Gibson <da...@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. > >> >> >> > >> >> >> 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. -- 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