Re: [PATCH V7 3/3] hotplug/cpu: Fix crash with memoryless nodes

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 11/16/2017 11:28 AM, Michael Bringmann wrote:
> On powerpc systems with shared configurations of CPUs and memory and
> memoryless nodes at boot, an event ordering problem was observed on
> a SLES12 build platforms with the hot-add of CPUs to the memoryless
> nodes.
> 
> * The most common error occurred when the memory SLAB driver attempted
>   to reference the memoryless node to which a CPU was being added
>   before the kernel had finished initializing all of the data structures
>   for the CPU and exited 'device_online' under DLPAR/hot-add.
> 
>   Normally the memoryless node would be initialized through the call
>   path device_online ... arch_update_cpu_topology ... find_cpu_nid
>   ...  try_online_node.  This patch ensures that the powerpc node will
>   be initialized as early as possible, even if it was memoryless and
>   CPU-less at the point when we are trying to hot-add a new CPU to it.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> Changes in V7:
>   -- Make function find_cpu_nid() externally visible/usable so that
>      it may be used from hotplug-cpu.c
> ---
>  arch/powerpc/mm/numa.c                       |    3 ++-
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |    3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 163f4cc..d6d4f7c 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1310,7 +1310,7 @@ static long vphn_get_associativity(unsigned long cpu,
>  	return rc;
>  }
> 
> -static inline int find_cpu_nid(int cpu)
> +int find_cpu_nid(int cpu)
>  {
>  	__be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
>  	int new_nid;
> @@ -1343,6 +1343,7 @@ static inline int find_cpu_nid(int cpu)
>  #endif
>  	}
> 
> +	printk(KERN_INFO "%s:%d cpu %d nid %d\n", __FUNCTION__, __LINE__, cpu, new_nid);

This seems like a more likely pr_debug statement.

>  	return new_nid;
>  }
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index a7d14aa7..df8c732 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -340,6 +340,8 @@ static void pseries_remove_processor(struct device_node *np)
>  	cpu_maps_update_done();
>  }
> 
> +extern int find_cpu_nid(int cpu);
> +
>  static int dlpar_online_cpu(struct device_node *dn)
>  {
>  	int rc = 0;
> @@ -364,6 +366,7 @@ static int dlpar_online_cpu(struct device_node *dn)
>  					!= CPU_STATE_OFFLINE);
>  			cpu_maps_update_done();
>  			timed_topology_update(1);
> +			find_cpu_nid(cpu);

We don't use the returned node from this call, so I'm not sure why it gets
called. Perhaps its the possible call to try_online_node() that may get called
in find_cpu_nid(), if so perhpas naming the routine something slightly
different would be good, like find_and_online_cpu_nid?

-Nathan
>  			rc = device_online(get_cpu_device(cpu));
>  			if (rc)
>  				goto out;
>