Re: [PATCH] powerpc/numa: Fix topology_physical_package_id() on pSeries

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

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

On Tue, Mar 16, 2021 at 01:24:37PM +0100, Cédric Le Goater wrote:
> The topology-id of a CPU in a pSeries machine can be queried from
> sysfs but under PowerVM the value is always -1 even if NUMA nodes are
> defined. This is because the topology_physical_package_id() routine is
> using the "ibm,chip-id" property which is not specified in PAPR.
> 
> Under QEMU/KVM, things are different because QEMU populates the CPU DT
> node with "ibm,chip-id" property. However, its value can be incorrect
> for uncommon SMT configuration and expose a bogus topology-id value in
> sysfs.

Incorrect in what sense?  It's still indicating the (admittedly
arbitrary) qemu socket number, isn't it?  And isn't that what it
should be?

> The use of cpu_to_node() guarantees to have a correct NUMA node id
> under both environments QEMU/KVM and PowerVM. This introduces a slight
> change for the QEMU/KVM guest, as the topology-id now matches the NUMA
> node and not the socket-id as before. Since QEMU also needs to remove
> "ibm,chip-id" property for the DT to follow the PAPR specs, both
> hypervisor environments will be in sync.
> 
> On the PowerNV side, the NUMA node id returned by cpu_to_node() is
> computed from the "ibm,associativity" property of the CPU. Its value
> is built from the OPAL chip id and is equivalent to "ibm,chip-id".

Like mpe, I'm not convinced this is the right approach.  "physical
packate" and NUMA node are not the same thing, except sometimes by
accident.

> 
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  arch/powerpc/include/asm/topology.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 3beeb030cd78..887c42a4e43d 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -123,7 +123,7 @@ static inline int cpu_to_coregroup_id(int cpu)
>  #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_node(cpu))
>  
>  #define topology_sibling_cpumask(cpu)	(per_cpu(cpu_sibling_map, cpu))
>  #define topology_core_cpumask(cpu)	(cpu_cpu_mask(cpu))

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