[PATCH RESEND V2 ] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[PATCH RESEND V2 ] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[Nathan Fontenot]

When DLPAR removing a CPU, the unmapping of the cpu from a node in
unmap_cpu_from_node() should also invalidate the CPUs entry in the
numa_cpu_lookup_table. There is not a guarantee that on a subsequent
DLPAR add of the CPU the associativity will be the same and thus
could be in a different node. Invalidating the entry in the
numa_cpu_lookup_table causes the associativity to be read from the
device tree at the time of the add.

The current behavior of not invalidating the CPUs entry in the
numa_cpu_lookup_table can result in scenarios where the the topology
layout of CPUs in the partition does not match the device tree
or the topology reported by the HMC.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---

Originally sent Dec. 5 2017, no reply, resending.

Updates for V2: Move the invalidation from unmap_cpu_from_node to
pseries_remove_processor, the former routine is also called during cpu
offline and we do not want to invalidate during cpu offline.

 arch/powerpc/include/asm/topology.h          |    5 +++++
 arch/powerpc/mm/numa.c                       |    5 -----
 arch/powerpc/platforms/pseries/hotplug-cpu.c |    2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index 88187c285c70..1c02e6900f78 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -44,6 +44,11 @@ extern int sysfs_add_device_to_node(struct device *dev, int nid);
 extern void sysfs_remove_device_from_node(struct device *dev, int nid);
 extern int numa_update_cpu_topology(bool cpus_locked);
 
+static inline void update_numa_cpu_lookup_table(unsigned int cpu, int node)
+{
+	numa_cpu_lookup_table[cpu] = node;
+}
+
 static inline int early_cpu_to_node(int cpu)
 {
 	int nid;
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 268c7a2d9a5b..7ec3a0d787d3 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -143,11 +143,6 @@ static void reset_numa_cpu_lookup_table(void)
 		numa_cpu_lookup_table[cpu] = -1;
 }
 
-static void update_numa_cpu_lookup_table(unsigned int cpu, int node)
-{
-	numa_cpu_lookup_table[cpu] = node;
-}
-
 static void map_cpu_to_node(int cpu, int node)
 {
 	update_numa_cpu_lookup_table(cpu, node);
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index a7d14aa7bb7c..09083ad82f7a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -36,6 +36,7 @@
 #include <asm/xics.h>
 #include <asm/xive.h>
 #include <asm/plpar_wrappers.h>
+#include <asm/topology.h>
 
 #include "pseries.h"
 #include "offline_states.h"
@@ -331,6 +332,7 @@ static void pseries_remove_processor(struct device_node *np)
 			BUG_ON(cpu_online(cpu));
 			set_cpu_present(cpu, false);
 			set_hard_smp_processor_id(cpu, -1);
+			update_numa_cpu_lookup_table(cpu, -1);
 			break;
 		}
 		if (cpu >= nr_cpu_ids)

Re: [PATCH RESEND V2 ] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[Tyrel Datwyler]

On 01/26/2018 11:41 AM, Nathan Fontenot wrote:
> When DLPAR removing a CPU, the unmapping of the cpu from a node in
> unmap_cpu_from_node() should also invalidate the CPUs entry in the
> numa_cpu_lookup_table. There is not a guarantee that on a subsequent
> DLPAR add of the CPU the associativity will be the same and thus
> could be in a different node. Invalidating the entry in the
> numa_cpu_lookup_table causes the associativity to be read from the
> device tree at the time of the add.
> 
> The current behavior of not invalidating the CPUs entry in the
> numa_cpu_lookup_table can result in scenarios where the the topology
> layout of CPUs in the partition does not match the device tree
> or the topology reported by the HMC.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Reviewed-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

> ---
> 
> Originally sent Dec. 5 2017, no reply, resending.
> 
> Updates for V2: Move the invalidation from unmap_cpu_from_node to
> pseries_remove_processor, the former routine is also called during cpu
> offline and we do not want to invalidate during cpu offline.
> 
>  arch/powerpc/include/asm/topology.h          |    5 +++++
>  arch/powerpc/mm/numa.c                       |    5 -----
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |    2 ++
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 88187c285c70..1c02e6900f78 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -44,6 +44,11 @@ extern int sysfs_add_device_to_node(struct device *dev, int nid);
>  extern void sysfs_remove_device_from_node(struct device *dev, int nid);
>  extern int numa_update_cpu_topology(bool cpus_locked);
> 
> +static inline void update_numa_cpu_lookup_table(unsigned int cpu, int node)
> +{
> +	numa_cpu_lookup_table[cpu] = node;
> +}
> +
>  static inline int early_cpu_to_node(int cpu)
>  {
>  	int nid;
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 268c7a2d9a5b..7ec3a0d787d3 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -143,11 +143,6 @@ static void reset_numa_cpu_lookup_table(void)
>  		numa_cpu_lookup_table[cpu] = -1;
>  }
> 
> -static void update_numa_cpu_lookup_table(unsigned int cpu, int node)
> -{
> -	numa_cpu_lookup_table[cpu] = node;
> -}
> -
>  static void map_cpu_to_node(int cpu, int node)
>  {
>  	update_numa_cpu_lookup_table(cpu, node);
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index a7d14aa7bb7c..09083ad82f7a 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -36,6 +36,7 @@
>  #include <asm/xics.h>
>  #include <asm/xive.h>
>  #include <asm/plpar_wrappers.h>
> +#include <asm/topology.h>
> 
>  #include "pseries.h"
>  #include "offline_states.h"
> @@ -331,6 +332,7 @@ static void pseries_remove_processor(struct device_node *np)
>  			BUG_ON(cpu_online(cpu));
>  			set_cpu_present(cpu, false);
>  			set_hard_smp_processor_id(cpu, -1);
> +			update_numa_cpu_lookup_table(cpu, -1);
>  			break;
>  		}
>  		if (cpu >= nr_cpu_ids)
> 

Re: [PATCH RESEND V2 ] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[Michael Ellerman]

Nathan Fontenot <nfont@linux.vnet.ibm.com> writes:

> When DLPAR removing a CPU, the unmapping of the cpu from a node in
> unmap_cpu_from_node() should also invalidate the CPUs entry in the
> numa_cpu_lookup_table. There is not a guarantee that on a subsequent
> DLPAR add of the CPU the associativity will be the same and thus
> could be in a different node. Invalidating the entry in the
> numa_cpu_lookup_table causes the associativity to be read from the
> device tree at the time of the add.

This last part seems to contradict the change log of commit d4edc5b6c480
("powerpc: Fix the setup of CPU-to-Node mappings during CPU online"),
which seems to say that we shouldn't be looking at the device tree.

Can you explain to me what I'm missing?

Also when did this break, always? Which commit should I mark this as
fixing?

cheers

Re: [PATCH RESEND V2 ] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[Nathan Fontenot]

On 01/27/2018 02:58 AM, Michael Ellerman wrote:
> Nathan Fontenot <nfont@linux.vnet.ibm.com> writes:
> 
>> When DLPAR removing a CPU, the unmapping of the cpu from a node in
>> unmap_cpu_from_node() should also invalidate the CPUs entry in the
>> numa_cpu_lookup_table. There is not a guarantee that on a subsequent
>> DLPAR add of the CPU the associativity will be the same and thus
>> could be in a different node. Invalidating the entry in the
>> numa_cpu_lookup_table causes the associativity to be read from the
>> device tree at the time of the add.
> 
> This last part seems to contradict the change log of commit d4edc5b6c480
> ("powerpc: Fix the setup of CPU-to-Node mappings during CPU online"),
> which seems to say that we shouldn't be looking at the device tree.
> 
> Can you explain to me what I'm missing?

The commit you refer to addresses CPU online/offline behavior and is correct
that we shouldn't reference the device tree. The cpu-to-node mapping shouldn't
change across a offline/online operation since the CPU remains assigned to
the partition the entire time.

This patch addresses CPUs that have been DLPAR removed, and as such the CPU
is no longer assigned to the partition. Given this we don't have a guarantee
that the CPU will have the same node-to-cpu mapping when it is assigned
back to the partition on a subsequent DLPAR add operation.

Without this patch, the CPU is put back in the node it was in previously
which may not match the node firmware states it belongs to.

> 
> Also when did this break, always? Which commit should I mark this as
> fixing?
As far as I know this has always been broken. I've looked the the git logs
for the numa and pseries cpu hotplug code and don't see a specific
commit I can point at for breaking this.

-Nathan

Re: [RESEND, V2] powerpc/numa: Invalidate numa_cpu_lookup_table on cpu remove

[Michael Ellerman]

On Fri, 2018-01-26 at 19:41:59 UTC, Nathan Fontenot wrote:
> When DLPAR removing a CPU, the unmapping of the cpu from a node in
> unmap_cpu_from_node() should also invalidate the CPUs entry in the
> numa_cpu_lookup_table. There is not a guarantee that on a subsequent
> DLPAR add of the CPU the associativity will be the same and thus
> could be in a different node. Invalidating the entry in the
> numa_cpu_lookup_table causes the associativity to be read from the
> device tree at the time of the add.
> 
> The current behavior of not invalidating the CPUs entry in the
> numa_cpu_lookup_table can result in scenarios where the the topology
> layout of CPUs in the partition does not match the device tree
> or the topology reported by the HMC.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> Reviewed-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/1d9a090783bef19fe8cdec878620d2

cheers