Hi Srikar, Still some comments sorry ...
Srikar Dronamraju <sri...@linux.vnet.ibm.com> writes: > Package_id is to find out all cores that are part of the same chip. On > PowerNV machines, package_id defaults to chip_id. However ibm,chip_id > property is not present in device-tree of PowerVM Lpars. Hence lscpu > output shows one core per socket and multiple cores. > > To overcome this, use nid as the package_id on PowerVM Lpars. > > Before the patch. > --------------- > Architecture: ppc64le > Byte Order: Little Endian > CPU(s): 128 > On-line CPU(s) list: 0-127 > Thread(s) per core: 8 > Core(s) per socket: 1 <---------------------- > Socket(s): 16 <---------------------- > NUMA node(s): 2 > Model: 2.2 (pvr 004e 0202) > Model name: POWER9 (architected), altivec supported > Hypervisor vendor: pHyp > Virtualization type: para > L1d cache: 32K > L1i cache: 32K > L2 cache: 512K > L3 cache: 10240K > NUMA node0 CPU(s): 0-63 > NUMA node1 CPU(s): 64-127 > # > # cat /sys/devices/system/cpu/cpu0/topology/physical_package_id > -1 > # > > After the patch > --------------- > Architecture: ppc64le > Byte Order: Little Endian > CPU(s): 128 > On-line CPU(s) list: 0-127 > Thread(s) per core: 8 <------------------------------ > Core(s) per socket: 8 <------------------------------ > Socket(s): 2 > NUMA node(s): 2 > Model: 2.2 (pvr 004e 0202) > Model name: POWER9 (architected), altivec supported > Hypervisor vendor: pHyp > Virtualization type: para > L1d cache: 32K > L1i cache: 32K > L2 cache: 512K > L3 cache: 10240K > NUMA node0 CPU(s): 0-63 > NUMA node1 CPU(s): 64-127 > # > # cat /sys/devices/system/cpu/cpu0/topology/physical_package_id > 0 > # > Now lscpu output is more in line with the system configuration. > > Signed-off-by: Srikar Dronamraju <sri...@linux.vnet.ibm.com> > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Michael Ellerman <m...@ellerman.id.au> > Cc: Vasant Hegde <hegdevas...@linux.vnet.ibm.com> > Cc: Vaidyanathan Srinivasan <sva...@linux.vnet.ibm.com> > --- > Changelog from v2: https://patchwork.ozlabs.org/patch/1151649 > - Rebased to v5.5-rc2 > - Renamed the new function to cpu_to_nid > - Removed checks to fadump property. (Looked too excessive) > - Moved pseries specific code to pseries/lpar.c > > Changelog from v1: https://patchwork.ozlabs.org/patch/1126145 > - In v1 cpu_to_chip_id was overloaded to fallback on nid. Michael > Ellerman wasn't comfortable with nid being shown up as chip_id. > > arch/powerpc/include/asm/topology.h | 7 ++++++- > arch/powerpc/kernel/smp.c | 6 +++--- > arch/powerpc/platforms/pseries/lpar.c | 22 ++++++++++++++++++++++ > 3 files changed, 31 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/include/asm/topology.h > b/arch/powerpc/include/asm/topology.h > index 2f7e1ea5089e..7422ef913c75 100644 > --- a/arch/powerpc/include/asm/topology.h > +++ b/arch/powerpc/include/asm/topology.h > @@ -130,11 +130,16 @@ static inline void shared_proc_topology_init(void) {} > > #ifdef CONFIG_SMP > #include <asm/cputable.h> > +#ifdef CONFIG_PPC_SPLPAR > +int cpu_to_nid(int); > +#else > +#define cpu_to_nid(cpu) cpu_to_chip_id(cpu) > +#endif We already have cpu_to_node(), which returns the numa node (nid) for a given CPU, so I think introducing a new accessor with an almost identical name is going to cause confusion. ie. when should code use cpu_to_node() and when should it use cpu_to_nid()? > #ifdef CONFIG_PPC64 > #include <asm/smp.h> > > -#define topology_physical_package_id(cpu) (cpu_to_chip_id(cpu)) > +#define topology_physical_package_id(cpu) (cpu_to_nid(cpu)) I think you should be overriding topology_physical_package_id() instead of introducing cpu_to_nid(). > #define topology_sibling_cpumask(cpu) (per_cpu(cpu_sibling_map, cpu)) > #define topology_core_cpumask(cpu) (per_cpu(cpu_core_map, cpu)) > #define topology_core_id(cpu) (cpu_to_core_id(cpu)) > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index ea6adbf6a221..b0c1438d8d9a 100644 > --- a/arch/powerpc/kernel/smp.c > +++ b/arch/powerpc/kernel/smp.c > @@ -1188,7 +1188,7 @@ static inline void add_cpu_to_smallcore_masks(int cpu) > static void add_cpu_to_masks(int cpu) > { > int first_thread = cpu_first_thread_sibling(cpu); > - int chipid = cpu_to_chip_id(cpu); > + int nid = cpu_to_nid(cpu); And here you should be using topology_physical_package_id(), rather than cpu_to_nid() directly. > int i; > > /* > @@ -1217,11 +1217,11 @@ static void add_cpu_to_masks(int cpu) > for_each_cpu(i, cpu_l2_cache_mask(cpu)) > set_cpus_related(cpu, i, cpu_core_mask); > > - if (chipid == -1) > + if (nid == -1) > return; > > for_each_cpu(i, cpu_online_mask) > - if (cpu_to_chip_id(i) == chipid) > + if (cpu_to_nid(i) == nid) > set_cpus_related(cpu, i, cpu_core_mask); > } > > diff --git a/arch/powerpc/platforms/pseries/lpar.c > b/arch/powerpc/platforms/pseries/lpar.c > index 60cb29ae4739..99583b9f00e4 100644 > --- a/arch/powerpc/platforms/pseries/lpar.c > +++ b/arch/powerpc/platforms/pseries/lpar.c > @@ -650,6 +650,28 @@ static int __init vcpudispatch_stats_procfs_init(void) I think this function should actually be in smp.c or numa.c That's because it will be called on non-pseries platforms when we build a multi platform kernel (pseries & powernv). So it's not really pseries specific code. > } > > machine_device_initcall(pseries, vcpudispatch_stats_procfs_init); > + > +int cpu_to_nid(cpu) > +{ > + int nid = cpu_to_chip_id(cpu); > + > + /* > + * If the platform is PowerNV or Guest on KVM, ibm,chip-id is > + * defined. Hence we would return the chip-id as the > + * cpu_to_nid. > + */ > + if (nid == -1 && firmware_has_feature(FW_FEATURE_LPAR)) { > + struct device_node *np = of_get_cpu_node(cpu, NULL); > + > + if (np) { > + nid = of_node_to_nid(np); > + of_node_put(np); > + } > + } > + return nid; > +} > +EXPORT_SYMBOL(cpu_to_nid); Should be GPL. cheers
