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

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

[Cédric Le Goater]

Initial commit 15863ff3b8da ("powerpc: Make chip-id information
available to userspace") introduce a cpu_to_chip_id() routine for the
PowerNV platform using the "ibm,chip-id" property to query the chip id
of a CPU. But PAPR does not specify such a property and the node id
query is broken.

Use cpu_to_node() instead which guarantees to have a correct value on
all platforms, PowerNV an pSeries.

Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Vasant Hegde <hegdevasant@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))
-- 
2.26.2

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

[Greg Kurz]

On Fri, 12 Mar 2021 15:31:54 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
> 
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.
> 
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Makes sense.

FWIW

Reviewed-by: Greg Kurz <groug@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))

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

[Daniel Henrique Barboza]

On 3/12/21 11:31 AM, Cédric Le Goater wrote:
> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
> 
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.

It is worth mentioning that that this patch will change how
topology_physical_package_id() represents in a QEMU guest. Right now, ibm,chip-id
in QEMU is matching the socket-id. After this patch, topology_physical_package_id()
will now match the NUMA id of the CPU.



Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> 
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@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))
> 

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

[Cédric Le Goater]

On 3/15/21 4:12 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 3/12/21 11:31 AM, Cédric Le Goater wrote:
>> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
>> available to userspace") introduce a cpu_to_chip_id() routine for the
>> PowerNV platform using the "ibm,chip-id" property to query the chip id
>> of a CPU. But PAPR does not specify such a property and the node id
>> query is broken.
>>
>> Use cpu_to_node() instead which guarantees to have a correct value on
>> all platforms, PowerNV an pSeries.
> 
> It is worth mentioning that that this patch will change how
> topology_physical_package_id() represents in a QEMU guest. Right now, ibm,chip-id
> in QEMU is matching the socket-id. After this patch, topology_physical_package_id()
> will now match the NUMA id of the CPU.

yes. I should have added some more background. 

LPARs are impacted by the use of ibm,chip-id because the property 
does not exist under PowerVM and the topology-id in sysfs is always
-1 even if NUMA nodes are defined.

Under QEMU/KVM, ibm,chip-id is badly calculated when using uncommon 
SMT configuration. This leads to a bogus topology-id value being 
exported in sysfs.

The use of cpu_to_node() guarantees to have a correct NUMA node id 
under both environments QEMU/KVM and PowerVM.

On the PowerNV platform, 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. 

May be I should rephrase the commit log in a v2 ?

C.

 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
>>
>> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Cc: Vasant Hegde <hegdevasant@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))
>>

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

[Daniel Henrique Barboza]

On 3/15/21 1:16 PM, Cédric Le Goater wrote:
> On 3/15/21 4:12 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 3/12/21 11:31 AM, Cédric Le Goater wrote:
>>> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
>>> available to userspace") introduce a cpu_to_chip_id() routine for the
>>> PowerNV platform using the "ibm,chip-id" property to query the chip id
>>> of a CPU. But PAPR does not specify such a property and the node id
>>> query is broken.
>>>
>>> Use cpu_to_node() instead which guarantees to have a correct value on
>>> all platforms, PowerNV an pSeries.
>>
>> It is worth mentioning that that this patch will change how
>> topology_physical_package_id() represents in a QEMU guest. Right now, ibm,chip-id
>> in QEMU is matching the socket-id. After this patch, topology_physical_package_id()
>> will now match the NUMA id of the CPU.
> 
> yes. I should have added some more background.
> 
> LPARs are impacted by the use of ibm,chip-id because the property
> does not exist under PowerVM and the topology-id in sysfs is always
> -1 even if NUMA nodes are defined.
> 
> Under QEMU/KVM, ibm,chip-id is badly calculated when using uncommon
> SMT configuration. This leads to a bogus topology-id value being
> exported in sysfs.
> 
> The use of cpu_to_node() guarantees to have a correct NUMA node id
> under both environments QEMU/KVM and PowerVM.
> 
> On the PowerNV platform, 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.
> 
> May be I should rephrase the commit log in a v2 ?

It's a fine idea, given that apparently we don't have documentation explaining
these details (well, at least I didn't find any). We can reference the commit
message later on as explanation :)




Thanks,

DHB

> 
> C.
> 
>   
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>>
>>> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>>> Cc: Vasant Hegde <hegdevasant@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))
>>>
> 

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

[Srikar Dronamraju]

* C?dric Le Goater <clg@kaod.org> [2021-03-12 15:31:54]:

> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
> 
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.
> 

While this looks good to me, @mpe had reservations on using nid as chip-id.
https://lore.kernel.org/linuxppc-dev/87lfwhypv0.fsf@concordia.ellerman.id.au/t/#u
He may be okay with using nid as a "virtual" package id in a pseries
environment.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@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))
> -- 
> 2.26.2
> 

-- 
Thanks and Regards
Srikar Dronamraju

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

[Srikar Dronamraju]

* C?dric Le Goater <clg@kaod.org> [2021-03-12 15:31:54]:

> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
>
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.
>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@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(-)
>

(Sorry I somehow managed to mangle to-address. Hence resending this mail
again)

While this looks good to me, @mpe had reservations on using nid as chip-id.
https://lore.kernel.org/linuxppc-dev/87lfwhypv0.fsf@concordia.ellerman.id.au/t/#u
He may be okay with using nid as a "virtual" package id in a pseries
environment.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

[---=| TOFU protection by t-prot: 24 lines snipped |=---]

--
Thanks and Regards
Srikar Dronamraju


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

--
Thanks and Regards
Srikar Dronamraju

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

[Cédric Le Goater]

On 3/16/21 12:28 PM, Srikar Dronamraju wrote:
> * C?dric Le Goater <clg@kaod.org> [2021-03-12 15:31:54]:
> 
>> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
>> available to userspace") introduce a cpu_to_chip_id() routine for the
>> PowerNV platform using the "ibm,chip-id" property to query the chip id
>> of a CPU. But PAPR does not specify such a property and the node id
>> query is broken.
>>
>> Use cpu_to_node() instead which guarantees to have a correct value on
>> all platforms, PowerNV an pSeries.
>>
>> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Cc: Vasant Hegde <hegdevasant@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(-)
>>
> 
> (Sorry I somehow managed to mangle to-address. Hence resending this mail
> again)
> 
> While this looks good to me, @mpe had reservations on using nid as chip-id.
> https://lore.kernel.org/linuxppc-dev/87lfwhypv0.fsf@concordia.ellerman.id.au/t/#u

that was a different approach.

> He may be okay with using nid as a "virtual" package id in a pseries
> environment.

ibm,chip-id is only used in a couple of places in low level PowerNV
drivers. The CPU doesn't use this property. We should be fine on
all platforms.

> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks,

C.
 
> [---=| TOFU protection by t-prot: 24 lines snipped |=---]
> 
> --
> Thanks and Regards
> Srikar Dronamraju
> 
> 
>> 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))
>> --
>> 2.26.2
>>
> 
> --
> Thanks and Regards
> Srikar Dronamraju
> 

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

[Michael Ellerman]

Cédric Le Goater <clg@kaod.org> writes:
> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
>
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.

I'm not convinced this is correct on any platforms :)

The node (nid) is just a number made up by Linux, so it's not a
"physical package id".

It might correspond to a "physical package" (whatever that means), on
existing skiboot, but it's not guaranteed.

The PAPR text around associativity and so on is incredibly dense, but I
think one thing that is clear is that firmware is allowed to present to
us whatever boundaries it thinks are most meaningful.

ie. if two things on one "physical package" have a meaningful distance
between them, then they might be presented to us as two nodes.

Having said that, if you look at the documentation in the kernel we
have:

	physical_package_id: physical package id of cpu#. Typically
	corresponds to a physical socket number, but the actual value
	is architecture and platform dependent.

Which is nicely vague.

But then:

	core_siblings: internal kernel map of cpu#'s hardware threads
	within the same physical_package_id.

Which is not true for us already on bare metal. And is just wrong on
modern CPUs where there's multiple non-siblings in a single
core/chip/package.

So a patch to update the documentation would be good :)

As far as what we should do in our topology code, I think we should use
the chip-id when we have it - ie. on bare metal.

It may not be exactly correct, but it's at least not filtered through
any indirection about NUMA-ness, ie. the associativity properties.

Also we've been using it for several years and I don't think we should
risk breaking anything by changing the value now.

As far as pseries, I'm still a bit dubious, but maybe using nid is
better than providing nothing, even if it is a lie.

cheers

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

[Cédric Le Goater]

> Also we've been using it for several years and I don't think we should
> risk breaking anything by changing the value now.

I guess we can leave it that way. Please read the commit log of 
the second patch (not tagged as a v2 ...).

But we should remove ibm,chip-id from QEMU since the property does 
not exist on PAPR and that the calculation is anyhow very broken. 

Thanks,

C. 

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

[Daniel Henrique Barboza]

On 3/18/21 4:28 AM, Cédric Le Goater wrote:
>> Also we've been using it for several years and I don't think we should
>> risk breaking anything by changing the value now.
> 
> I guess we can leave it that way. Please read the commit log of
> the second patch (not tagged as a v2 ...).
> 
> But we should remove ibm,chip-id from QEMU since the property does
> not exist on PAPR and that the calculation is anyhow very broken.


I am a strong advocate of getting rid of ibm,chip-id in QEMU. That said,
we need to make sure that the current problem with CPU topologies, that
I reported in that other thread, can be fixed without it.


Thanks,


DHB




> 
> Thanks,
> 
> C.
> 

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

[Cédric Le Goater]

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.

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

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

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

[David Gibson]

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