Re: [PATCH 1/6] mm/numa: Introduce numa_nearest_nodemask()

[Andrea Righi <arighi@nvidia.com>]

On Mon, Feb 10, 2025 at 11:41:48AM -0500, Yury Norov wrote:
...
> > > Numa should include this via linux/nodemask_types.h, or maybe
> > > nodemask.h.
> > 
> > Hm... nodemask_types.h needs to include numa.h to resolve MAX_NUMNODES,
> > Maybe we can move numa_nearest_nodemask() to linux/nodemask.h?
> 
> Maybe yes, but it's generally wrong. nodemask.h is a library, and
> numa.h (generally, NUMA code) is one user. The right way to go is when
> client code includes all necessary libs, not vise-versa.

Ok, makes sense.

> 
> Regarding MAX_NUMNODES, it's a natural property of nodemasks, and
> should be declared in nodemask.h. The attached patch reverts the
> inclusion paths dependency. I build-tested it against today's master
> and x86_64 defconfig. Can you consider taking it and prepending your
> series?

Sure, I'll test it and include it in the next version.

>  
> > > >  #ifdef CONFIG_NUMA
> > > >  #include <asm/sparsemem.h>
> > > >  
> > > > @@ -38,6 +40,7 @@ void __init alloc_offline_node_data(int nid);
> > > >  
> > > >  /* Generic implementation available */
> > > >  int numa_nearest_node(int node, unsigned int state);
> > > > +int numa_nearest_nodemask(int node, unsigned int state, struct nodemask *mask);
> > > >  
> > > >  #ifndef memory_add_physaddr_to_nid
> > > >  int memory_add_physaddr_to_nid(u64 start);
> > > > @@ -55,6 +58,11 @@ static inline int numa_nearest_node(int node, unsigned int state)
> > > >  	return NUMA_NO_NODE;
> > > >  }
> > > >  
> > > > +static inline int numa_nearest_nodemask(int node, unsigned int state, struct nodemask *mask)
> > > > +{
> > > > +	return NUMA_NO_NODE;
> > > > +}
> > > > +
> > > >  static inline int memory_add_physaddr_to_nid(u64 start)
> > > >  {
> > > >  	return 0;
> > > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > > > index 162407fbf2bc7..1cfee509c7229 100644
> > > > --- a/mm/mempolicy.c
> > > > +++ b/mm/mempolicy.c
> > > > @@ -196,6 +196,44 @@ int numa_nearest_node(int node, unsigned int state)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(numa_nearest_node);
> > > >  
> > > > +/**
> > > > + * numa_nearest_nodemask - Find the node in @mask at the nearest distance
> > > > + *			   from @node.
> > > > + *
> > > 
> > > So, I have a feeling about this whole naming scheme. At first, this
> > > function (and the existing numa_nearest_node()) searches for something,
> > > but doesn't begin with find_, search_ or similar. Second, the naming
> > > of existing numa_nearest_node() doesn't reflect that it searches
> > > against the state. Should we always include some state for search? If
> > > so, we can skip mentioning the state, otherwise it should be in the
> > > name, I guess...
> > > 
> > > The problem is that I have no idea for better naming, and I have no
> > > understanding about the future of this functions family. If it's just
> > > numa_nearest_node() and numa_nearest_nodemask(), I'm OK to go this
> > > way. If we'll add more flavors similarly to find_bit() family, we
> > > could probably discuss a naming scheme.
> > > 
> > > Also, mm/mempolicy.c is a historical place for them, but maybe we need
> > > to move it somewhere else?
> > > 
> > > Any thoughts appreciated.
> > 
> > Personally I think adding "find_" to the name would be a bit redundant, as
> > it seems quite obvious that it's finding the nearest node. It sounds a bit
> > like the get_something() discussion and we can just use something().
> > 
> > About adding "_state" to the name, it'd make sense since we have
> > for_each_node_state/for_each_node(), but we would need to change
> > numa_nearest_node() -> numa_nearest_node_state((), that is beyond the scope
> > of this patch set.
> > 
> > If I had to design this completely from scratch I'd probably propose
> > something like this:
> >  - nearest_node_state(node, state)
> >  - nearest_node(node) -> nearest_node_state(node, N_POSSIBLE)
> >  - nearest_node_nodemask(node, nodemask) -> here the state can be managed
> >    with the nodemask (as you suggested below)
> > 
> > But again, this is probably a more generic discussion that can be addressed
> > in a separate thread.
> 
> Yes, it's not related to your series. Please just introduce
> nearest_node_nodemask(node, nodemask) as your minimal required change.
> I will do a necessary cleanup later, if needed.

Ok, thanks!

-Andrea