Re: [PATCH v3] topology: add support for node_to_mem_node() to determine the fallback node

[Nishanth Aravamudan <nacc@linux.vnet.ibm.com>]

On 10.09.2014 [12:06:16 -0700], Andrew Morton wrote:
> On Tue, 9 Sep 2014 17:47:23 -0700 Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote:
> 
> > On 09.09.2014 [17:11:15 -0700], Andrew Morton wrote:
> > > On Tue, 9 Sep 2014 12:03:27 -0700 Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote:
> > > 
> > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > > 
> > > > We need to determine the fallback node in slub allocator if the
> > > > allocation target node is memoryless node. Without it, the SLUB wrongly
> > > > select the node which has no memory and can't use a partial slab,
> > > > because of node mismatch. Introduced function, node_to_mem_node(X), will
> > > > return a node Y with memory that has the nearest distance. If X is
> > > > memoryless node, it will return nearest distance node, but, if X is
> > > > normal node, it will return itself.
> > > > 
> > > > We will use this function in following patch to determine the fallback
> > > > node.
> > > > 
> > > > ...
> > > >
> > > > --- a/include/linux/topology.h
> > > > +++ b/include/linux/topology.h
> > > > @@ -119,11 +119,20 @@ static inline int numa_node_id(void)
> > > >   * Use the accessor functions set_numa_mem(), numa_mem_id() and cpu_to_mem().
> > > 
> > > This comment could be updated.
> > 
> > Will do, do you prefer a follow-on patch or one that replaces this one?
> 
> Either is OK for me.  I always turn replacement patches into
> incrementals so I (and others) can see what changed.

Ok, I'll probably send you just an incremental then myself.

> > > >   */
> > > >  DECLARE_PER_CPU(int, _numa_mem_);
> > > > +extern int _node_numa_mem_[MAX_NUMNODES];
> > > >  
> > > >  #ifndef set_numa_mem
> > > >  static inline void set_numa_mem(int node)
> > > >  {
> > > >  	this_cpu_write(_numa_mem_, node);
> > > > +	_node_numa_mem_[numa_node_id()] = node;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#ifndef node_to_mem_node
> > > > +static inline int node_to_mem_node(int node)
> > > > +{
> > > > +	return _node_numa_mem_[node];
> > > >  }
> > > 
> > > A wee bit of documentation wouldn't hurt.
> 
> This?

Yep, I'll make sure it gets added.

> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -85,6 +85,7 @@ EXPORT_PER_CPU_SYMBOL(numa_node);
> > > >   */
> > > >  DEFINE_PER_CPU(int, _numa_mem_);		/* Kernel "local memory" node */
> > > >  EXPORT_PER_CPU_SYMBOL(_numa_mem_);
> > > > +int _node_numa_mem_[MAX_NUMNODES];
> > > 
> > > How does this get updated as CPUs, memory and nodes are hot-added and
> > > removed?
> > 
> > As CPUs are added, the architecture code in the CPU bringup will update
> > the NUMA topology. Memory and node hotplug are still open issues, I
> > mentioned the former in the cover letter. I should have mentioned it in
> > this commit message as well.
> 
> Please define "open issue".  The computer will crash and catch fire?
> If not that, then what?

Umm, let's call it "undefined" (untested?). Which is no different than
where we are today, afaict, with memoryless nodes. I think going from
memoryless->memoryful probably works, but the other direction may not
(and may not be possible in the common case).

-Nish