<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="appendonsend"></div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<br>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="divRplyFwdMsg" dir="ltr"><font style="font-size:11pt" face="Calibri, sans-serif" color="#000000"><b>From:</b> Srikar Dronamraju <srikar@linux.vnet.ibm.com><br>
<b>Sent:</b> Friday, April 8, 2022 5:55 PM<br>
<b>To:</b> Oscar Salvador <osalvador@suse.de><br>
<b>Cc:</b> Michael Ellerman <mpe@ellerman.id.au>; linuxppc-dev <linuxppc-dev@lists.ozlabs.org>; linux-mm@kvack.org <linux-mm@kvack.org>; Michal Hocko <mhocko@kernel.org>; Geetika Moolchandani1 <Geetika.Moolchandani1@ibm.com><br>
<b>Subject:</b> Re: [PATCH] powerpc/numa: Handle partially initialized numa nodes</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt">
<div class="PlainText">* Oscar Salvador <osalvador@suse.de> [2022-04-06 18:19:00]:<br>
<br>
> On Wed, Mar 30, 2022 at 07:21:23PM +0530, Srikar Dronamraju wrote:<br>
> > arch/powerpc/mm/numa.c | 2 +-<br>
> > 1 file changed, 1 insertion(+), 1 deletion(-)<br>
> > <br>
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c<br>
> > index b9b7fefbb64b..13022d734951 100644<br>
> > --- a/arch/powerpc/mm/numa.c<br>
> > +++ b/arch/powerpc/mm/numa.c<br>
> > @@ -1436,7 +1436,7 @@ int find_and_online_cpu_nid(int cpu)<br>
> > if (new_nid < 0 || !node_possible(new_nid))<br>
> > new_nid = first_online_node;<br>
> > <br>
> > - if (NODE_DATA(new_nid) == NULL) {<br>
> > + if (!node_online(new_nid)) {<br>
> > #ifdef CONFIG_MEMORY_HOTPLUG<br>
> > /*<br>
> > * Need to ensure that NODE_DATA is initialized for a node from<br>
> <br>
> Because of this fix, I wanted to check whether we might have any more fallouts due<br>
> to ("mm: handle uninitialized numa nodes gracefully"), and it made me look closer<br>
> as to why powerpc is the only platform that special cases try_online_node(),<br>
> while all others rely on cpu_up()->try_online_node() to do the right thing.<br>
> <br>
> So, I had a look.<br>
> Let us rewind a bit:<br>
> <br>
> The commit that sets find_and_online_cpu_nid() in dlpar_online_cpu was<br>
> e67e02a544e9 ("powerpc/pseries: Fix cpu hotplug crash with memoryless nodes").<br>
> <br>
> In there, it says that we have the following picture:<br>
> <br>
> partition_sched_domains<br>
> arch_update_cpu_topology<br>
> numa_update_cpu_topology<br>
> find_and_online_cpu_nid<br>
> <br>
> and by the time find_and_online_cpu_nid() gets called to online the node, it might be<br>
> too late as we might have referenced some NODE_DATA() fields.<br>
> Note that this happens at a much later stage in cpuhp.<br>
> <br>
> Also note that at a much earlier stage, we do already have a try_online_node() in cpu_up(),<br>
> which should allocate-and-online the node and prevent accessing garbage.<br>
> But the problem is that, on powerpc, all possible cpus have the same node set at boot stage<br>
> (see arch/powerpc/mm/numa.c:mem_topology_setup),<br>
> so cpu_to_node() returns the same thing until it the mapping gets update (which happens in<br>
> start_secondary()->set_numa_node()), and so, the try_online_node() from cpu_up() does not<br>
> apply on the right node, because it still holds the not-up-to-date mapping node <-> cpu.<br>
> <br>
> (e.g: in my test case, when onlining a CPU belongin to node1, cpu_up()->try_online_node()<br>
> tries to online node0, or whatever old mapping numa<->cpu is there).<br>
> <br>
> So, we have something like:<br>
> <br>
> dlpar_online_cpu<br>
> device_online<br>
> dev->bus->online<br>
> cpu_subsys_online<br>
> cpu_device_up<br>
> cpu_up<br>
> try_online_node (old mapping nid <-> cpu )<br>
> ...<br>
> ...<br>
> cphp_callbacks<br>
> sched_cpu_activate<br>
> cpuset_update_active_cpus<br>
> schedule_work(&cpuset_hotplug_work)<br>
> cpuset_hotplug_work<br>
> partition_sched_domains<br>
> arch_update_cpu_topology<br>
> numa_update_cpu_topology<br>
> find_and_online_cpu_nid (online new_nid)<br>
> <br>
> <br>
> So, yeah, the real onlining in numa_update_cpu_topology()->find_and_online_cpu_nid()<br>
> happens too late, that is why adding find_and_online_cpu_nid() back in dlpar_online_cpu()<br>
> fixed the issue, but we should not need this special casing at all.<br>
> <br>
> We do already know the numa<->cpu associativity in<br>
> dlpar_online_cpu()->find_and_online_cpu_nid(), so we should just be able to<br>
> update the numa<->cpu mapping, and let the try_online_node() do the right thing.<br>
> <br>
> With this in mind, I came up with the following patch, where I carried out a battery<br>
> of tests for CPU hotplug stuff and it worked as expected.<br>
> But I am not familiar with all powerpc pitfalls, e.g: dedicated vs shared cpus etc, so<br>
> I would like to hear from more familiar people.<br>
> <br>
> The patch is:<br>
> <br>
> From: Oscar Salvador <osalvador@suse.de><br>
> Date: Wed, 6 Apr 2022 14:39:15 +0200<br>
> Subject: [PATCH] powerpc/numa: Associate numa node to its cpu earlier<br>
> <br>
> powerpc is the only platform that do not rely on<br>
> cpu_up()->try_online_node() to bring up a numa node,<br>
> and special cases it, instead, deep in its own machinery:<br>
> <br>
> dlpar_online_cpu<br>
> find_and_online_cpu_nid<br>
> try_online_node<br>
> <br>
> This should not be needed, but the thing is that the try_online_node()<br>
> from cpu_up() will not apply on the right node, because cpu_to_node()<br>
> will return the old mapping numa<->cpu that gets set on boot stage<br>
> for all possible cpus.<br>
> <br>
> That can be seen easily if we try to print out the numa node passed<br>
> to try_online_node() in cpu_up().<br>
> <br>
> The thing is that the numa<->cpu mapping does not get updated till a much<br>
> later stage in start_secondary:<br>
> <br>
> start_secondary:<br>
> set_numa_node(numa_cpu_lookup_table[cpu])<br>
> <br>
> But we do not really care, as we already now the<br>
> CPU <-> NUMA associativity back in find_and_online_cpu_nid(),<br>
> so let us make use of that and set the proper numa<->cpu mapping,<br>
> so cpu_to_node() in cpu_up() returns the right node and<br>
> try_online_node() can do its work.<br>
> <br>
> Signed-off-by: Oscar Salvador <osalvador@suse.de><br>
<br>
Looks good to me.<br>
<br>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com><br>
<br>
<br>
> ---<br>
> arch/powerpc/include/asm/topology.h | 8 ++-----<br>
> arch/powerpc/mm/numa.c | 31 +++++++---------------------<br>
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +-<br>
> 3 files changed, 11 insertions(+), 30 deletions(-)<br>
> <br>
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h<br>
> index 36fcafb1fd6d..6ae1b2dce83e 100644<br>
> --- a/arch/powerpc/include/asm/topology.h<br>
> +++ b/arch/powerpc/include/asm/topology.h<br>
> @@ -111,14 +111,10 @@ static inline void unmap_cpu_from_node(unsigned long cpu) {}<br>
> #endif /* CONFIG_NUMA */<br>
> <br>
> #if defined(CONFIG_NUMA) && defined(CONFIG_PPC_SPLPAR)<br>
> -extern int find_and_online_cpu_nid(int cpu);<br>
> +extern void find_and_update_cpu_nid(int cpu);<br>
> extern int cpu_to_coregroup_id(int cpu);<br>
> #else<br>
> -static inline int find_and_online_cpu_nid(int cpu)<br>
> -{<br>
> - return 0;<br>
> -}<br>
> -<br>
> +static inline void find_and_update_cpu_nid(int cpu) {}<br>
> static inline int cpu_to_coregroup_id(int cpu)<br>
> {<br>
> #ifdef CONFIG_SMP<br>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c<br>
> index b9b7fefbb64b..b5bc8b1a833d 100644<br>
> --- a/arch/powerpc/mm/numa.c<br>
> +++ b/arch/powerpc/mm/numa.c<br>
> @@ -1423,43 +1423,28 @@ static long vphn_get_associativity(unsigned long cpu,<br>
> return rc;<br>
> }<br>
> <br>
> -int find_and_online_cpu_nid(int cpu)<br>
> +void find_and_update_cpu_nid(int cpu)<br>
> {<br>
> __be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};<br>
> int new_nid;<br>
> <br>
> /* Use associativity from first thread for all siblings */<br>
> if (vphn_get_associativity(cpu, associativity))<br>
> - return cpu_to_node(cpu);<br>
> + return;<br>
> <br>
> + /* Do not have previous associativity, so find it now. */<br>
> new_nid = associativity_to_nid(associativity);<br>
> +<br>
> if (new_nid < 0 || !node_possible(new_nid))<br>
> new_nid = first_online_node;<br>
> -<br>
> - if (NODE_DATA(new_nid) == NULL) {<br>
> -#ifdef CONFIG_MEMORY_HOTPLUG<br>
> - /*<br>
> - * Need to ensure that NODE_DATA is initialized for a node from<br>
> - * available memory (see memblock_alloc_try_nid). If unable to<br>
> - * init the node, then default to nearest node that has memory<br>
> - * installed. Skip onlining a node if the subsystems are not<br>
> - * yet initialized.<br>
> - */<br>
> - if (!topology_inited || try_online_node(new_nid))<br>
> - new_nid = first_online_node;<br>
> -#else<br>
> - /*<br>
> - * Default to using the nearest node that has memory installed.<br>
> - * Otherwise, it would be necessary to patch the kernel MM code<br>
> - * to deal with more memoryless-node error conditions.<br>
> + else<br>
> + /* Associate node <-> cpu, so cpu_up() calls<br>
> + * try_online_node() on the right node.<br>
> */<br>
> - new_nid = first_online_node;<br>
> -#endif<br>
> - }<br>
> + set_cpu_numa_node(cpu, new_nid);<br>
> <br>
> pr_debug("%s:%d cpu %d nid %d\n", __FUNCTION__, __LINE__,<br>
> cpu, new_nid);<br>
> - return new_nid;<br>
> }<br>
> <br>
> int cpu_to_coregroup_id(int cpu)<br>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c<br>
> index b81fc846d99c..0f8cd8b06432 100644<br>
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c<br>
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c<br>
> @@ -398,7 +398,7 @@ static int dlpar_online_cpu(struct device_node *dn)<br>
> if (get_hard_smp_processor_id(cpu) != thread)<br>
> continue;<br>
> cpu_maps_update_done();<br>
> - find_and_online_cpu_nid(cpu);<br>
> + find_and_update_cpu_nid(cpu);<br>
> rc = device_online(get_cpu_device(cpu));<br>
> if (rc) {<br>
> dlpar_offline_cpu(dn);<br>
> --<br>
> 2.16.4<br>
> <br>
> -- <br>
> Oscar Salvador<br>
> SUSE Labs</div>
<div class="PlainText"><br>
</div>
<div class="PlainText">Tested-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com><br>
</div>
</span></font></div>
</body>
</html>