NUMA topology question wrt. d4edc5b6

NUMA topology question wrt. d4edc5b6

[Nishanth Aravamudan]

Hi Srivatsa,

After d4edc5b6 ("powerpc: Fix the setup of CPU-to-Node mappings during
CPU online"), cpu_to_node() looks like:

static inline int cpu_to_node(int cpu)
{
        int nid;

        nid = numa_cpu_lookup_table[cpu];

        /*
         * During early boot, the numa-cpu lookup table might not have been
         * setup for all CPUs yet. In such cases, default to node 0.
         */
        return (nid < 0) ? 0 : nid;
}

However, I'm curious if this is correct in all cases. I have seen
several LPARs that do not have any CPUs on node 0. In fact, because node
0 is statically set online in the initialization of the N_ONLINE
nodemask, 0 is always present to Linux, whether it is present on the
system. I'm not sure what the best thing to do here is, but I'm curious
if you have any ideas? I would like to remove the static initialization
of node 0, as it's confusing to users to see an empty node (particularly
when it's completely separate in the numbering from other nodes), but
we trip a panic (refer to:
http://www.spinics.net/lists/linux-mm/msg73321.html).

Thanks,
Nish

Re: NUMA topology question wrt. d4edc5b6

[Srivatsa S. Bhat]

[ Adding a few more CC's ]

On 05/22/2014 01:34 AM, Nishanth Aravamudan wrote:
> Hi Srivatsa,
> 
> After d4edc5b6 ("powerpc: Fix the setup of CPU-to-Node mappings during
> CPU online"), cpu_to_node() looks like:
> 
> static inline int cpu_to_node(int cpu)
> {
>         int nid;
> 
>         nid = numa_cpu_lookup_table[cpu];
> 
>         /*
>          * During early boot, the numa-cpu lookup table might not have been
>          * setup for all CPUs yet. In such cases, default to node 0.
>          */
>         return (nid < 0) ? 0 : nid;
> }
> 
> However, I'm curious if this is correct in all cases. I have seen
> several LPARs that do not have any CPUs on node 0. In fact, because node
> 0 is statically set online in the initialization of the N_ONLINE
> nodemask, 0 is always present to Linux, whether it is present on the
> system. I'm not sure what the best thing to do here is, but I'm curious
> if you have any ideas? I would like to remove the static initialization
> of node 0, as it's confusing to users to see an empty node (particularly
> when it's completely separate in the numbering from other nodes), but
> we trip a panic (refer to:
> http://www.spinics.net/lists/linux-mm/msg73321.html).
> 

Ah, I see. I didn't have any particular reason to default it to zero.
I just did that because the existing code before this patch did the same
thing. (numa_cpu_lookup_table[] is a global array, so it will be initialized
with zeros. So if we access it before populating it via numa_setup_cpu(),
it would return 0. So I retained that behaviour with the above conditional).

Will something like the below [totally untested] patch solve the boot-panic?
I understand that as of today first_online_node will still pick 0 since
N_ONLINE is initialized statically, but with your proposed change to that
init code, I guess the following patch should avoid the boot panic.

[ But note that first_online_node is hard-coded to 0, if MAX_NUMNODES is = 1.
So we'll have to fix that if powerpc can have a single node system whose node
is numbered something other than 0. Can that happen as well? ]


And regarding your question about what is the best way to fix this whole Linux
MM's assumption about node0, I'm not really sure.. since I am not really aware
of the extent to which the MM subsystem is intertwined with this assumption
and what it would take to cure that :-(

Regards,
Srivatsa S. Bhat


diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index c920215..58e6469 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -18,6 +18,7 @@ struct device_node;
  */
 #define RECLAIM_DISTANCE 10
 
+#include <linux/nodemask.h>
 #include <asm/mmzone.h>
 
 static inline int cpu_to_node(int cpu)
@@ -30,7 +31,7 @@ static inline int cpu_to_node(int cpu)
 	 * During early boot, the numa-cpu lookup table might not have been
 	 * setup for all CPUs yet. In such cases, default to node 0.
 	 */
-	return (nid < 0) ? 0 : nid;
+	return (nid < 0) ? first_online_node : nid;
 }
 
 #define parent_node(node)	(node)

Re: NUMA topology question wrt. d4edc5b6

[Nishanth Aravamudan]

On 23.05.2014 [02:18:05 +0530], Srivatsa S. Bhat wrote:
> 
> [ Adding a few more CC's ]
> 
> On 05/22/2014 01:34 AM, Nishanth Aravamudan wrote:
> > Hi Srivatsa,
> > 
> > After d4edc5b6 ("powerpc: Fix the setup of CPU-to-Node mappings during
> > CPU online"), cpu_to_node() looks like:
> > 
> > static inline int cpu_to_node(int cpu)
> > {
> >         int nid;
> > 
> >         nid = numa_cpu_lookup_table[cpu];
> > 
> >         /*
> >          * During early boot, the numa-cpu lookup table might not have been
> >          * setup for all CPUs yet. In such cases, default to node 0.
> >          */
> >         return (nid < 0) ? 0 : nid;
> > }
> > 
> > However, I'm curious if this is correct in all cases. I have seen
> > several LPARs that do not have any CPUs on node 0. In fact, because node
> > 0 is statically set online in the initialization of the N_ONLINE
> > nodemask, 0 is always present to Linux, whether it is present on the
> > system. I'm not sure what the best thing to do here is, but I'm curious
> > if you have any ideas? I would like to remove the static initialization
> > of node 0, as it's confusing to users to see an empty node (particularly
> > when it's completely separate in the numbering from other nodes), but
> > we trip a panic (refer to:
> > http://www.spinics.net/lists/linux-mm/msg73321.html).
> > 
> 
> Ah, I see. I didn't have any particular reason to default it to zero.
> I just did that because the existing code before this patch did the same
> thing. (numa_cpu_lookup_table[] is a global array, so it will be initialized
> with zeros. So if we access it before populating it via numa_setup_cpu(),
> it would return 0. So I retained that behaviour with the above conditional).

Ok, that seems reasonable to me (keeping the behavior the same as it was
before).

> Will something like the below [totally untested] patch solve the boot-panic?
> I understand that as of today first_online_node will still pick 0 since
> N_ONLINE is initialized statically, but with your proposed change to that
> init code, I guess the following patch should avoid the boot panic.
> 
> [ But note that first_online_node is hard-coded to 0, if MAX_NUMNODES is = 1.
> So we'll have to fix that if powerpc can have a single node system whose node
> is numbered something other than 0. Can that happen as well? ]

I think all single-node systems are only Node 0, but I'm not 100% on
that.

> And regarding your question about what is the best way to fix this
> whole Linux MM's assumption about node0, I'm not really sure.. since I
> am not really aware of the extent to which the MM subsystem is
> intertwined with this assumption and what it would take to cure that
> :-(

Well, at this point, it might be fine to just leave it alone, as it
seems to be more trouble than it's worth -- and really the only
confusion is on those LPARs where there really isn't a Node 0. I'll take
another look later this week.

Thanks,
Nish

Re: NUMA topology question wrt. d4edc5b6

[David Rientjes]

On Fri, 23 May 2014, Srivatsa S. Bhat wrote:

> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index c920215..58e6469 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -18,6 +18,7 @@ struct device_node;
>   */
>  #define RECLAIM_DISTANCE 10
>  
> +#include <linux/nodemask.h>
>  #include <asm/mmzone.h>
>  
>  static inline int cpu_to_node(int cpu)
> @@ -30,7 +31,7 @@ static inline int cpu_to_node(int cpu)
>  	 * During early boot, the numa-cpu lookup table might not have been
>  	 * setup for all CPUs yet. In such cases, default to node 0.
>  	 */
> -	return (nid < 0) ? 0 : nid;
> +	return (nid < 0) ? first_online_node : nid;
>  }
>  
>  #define parent_node(node)	(node)

I wonder what would happen on ppc if we just returned NUMA_NO_NODE here 
for cpus that have not been mapped (they shouldn't even be possible).  
This would at least allow callers that do
kmalloc_node(..., cpu_to_node(cpu)) to be allocated on the local cpu 
rather than on a perhaps offline or remote node 0.

It would seem better to catch callers that do 
cpu_to_node(<not-possible-cpu>) rather than blindly return an online node.

Re: NUMA topology question wrt. d4edc5b6

[Nishanth Aravamudan]

On 09.06.2014 [14:38:26 -0700], David Rientjes wrote:
> On Fri, 23 May 2014, Srivatsa S. Bhat wrote:
> 
> > diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> > index c920215..58e6469 100644
> > --- a/arch/powerpc/include/asm/topology.h
> > +++ b/arch/powerpc/include/asm/topology.h
> > @@ -18,6 +18,7 @@ struct device_node;
> >   */
> >  #define RECLAIM_DISTANCE 10
> >  
> > +#include <linux/nodemask.h>
> >  #include <asm/mmzone.h>
> >  
> >  static inline int cpu_to_node(int cpu)
> > @@ -30,7 +31,7 @@ static inline int cpu_to_node(int cpu)
> >  	 * During early boot, the numa-cpu lookup table might not have been
> >  	 * setup for all CPUs yet. In such cases, default to node 0.
> >  	 */
> > -	return (nid < 0) ? 0 : nid;
> > +	return (nid < 0) ? first_online_node : nid;
> >  }
> >  
> >  #define parent_node(node)	(node)
> 
> I wonder what would happen on ppc if we just returned NUMA_NO_NODE here 
> for cpus that have not been mapped (they shouldn't even be possible).  

Well, with my patch (Ben sent it to Linus in the last pull request, I
think), powerpc uses the generic per-cpu stuff, so this function is
gone. Dunno if it makes sense to initialize the per-cpu data to
NUMA_NO_NODE (rather than 0?).

For powerpc, it's a timing thing. We can call cpu_to_node() quite early,
and we may not have set up the mapping information yet.

> This would at least allow callers that do
> kmalloc_node(..., cpu_to_node(cpu)) to be allocated on the local cpu 
> rather than on a perhaps offline or remote node 0.
> 
> It would seem better to catch callers that do 
> cpu_to_node(<not-possible-cpu>) rather than blindly return an online node.

Agreed, but I've not seen such a case.

Thanks,
Nish