Re: [PATCH 2/7] mm/numa: Introduce nearest_node_nodemask()

[Yury Norov <yury.norov@gmail.com>]

On Fri, Feb 14, 2025 at 09:55:25AM +0100, Andrea Righi wrote:
> Hi Yury,
> 
> On Thu, Feb 13, 2025 at 12:12:46PM -0500, Yury Norov wrote:
> ...
> > > > >  include/linux/numa.h |  7 +++++++
> > > > >  mm/mempolicy.c       | 32 ++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 39 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/numa.h b/include/linux/numa.h
> > > > > index 31d8bf8a951a7..e6baaf6051bcf 100644
> > > > > --- a/include/linux/numa.h
> > > > > +++ b/include/linux/numa.h
> > > > > @@ -31,6 +31,8 @@ void __init alloc_offline_node_data(int nid);
> > > > >  /* Generic implementation available */
> > > > >  int numa_nearest_node(int node, unsigned int state);
> > > > >  
> > > > > +int nearest_node_nodemask(int node, nodemask_t *mask);
> > > > > +
> > > > 
> > > > See how you use it. It looks a bit inconsistent to the other functions:
> > > > 
> > > >   #define for_each_node_numadist(node, unvisited)                                \
> > > >          for (int start = (node),                                                \
> > > >               node = nearest_node_nodemask((start), &(unvisited));               \
> > > >               node < MAX_NUMNODES;                                               \
> > > >               node_clear(node, (unvisited)),                                     \
> > > >               node = nearest_node_nodemask((start), &(unvisited)))
> > > >   
> > > > 
> > > > I would suggest to make it aligned with the rest of the API:
> > > > 
> > > >   #define node_clear(node, dst) __node_clear((node), &(dst))
> > > >   static __always_inline void __node_clear(int node, volatile nodemask_t *dstp)
> > > >   {
> > > >           clear_bit(node, dstp->bits);
> > > >   }
> > > 
> > > Sorry Yury, can you elaborate more on this? What do you mean with
> > > inconsistent, is it the volatile nodemask_t *?
> > 
> > What I mean is:
> >   #define nearest_node_nodemask(start, srcp)
> >                 __nearest_node_nodemask((start), &(srcp))
> >   int __nearest_node_nodemask(int node, nodemask_t *mask);
> 
> This all makes sense assuming that nearest_node_nodemask() is placed in
> include/linux/nodemask.h and is considered as a nodemask API, but I thought
> we determined to place it in include/linux/numa.h, since it seems more of a
> NUMA API, similar to numa_nearest_node(), so under this assumption I was
> planning to follow the same style of numa_nearest_node().
> 
> Or do you think it should go in linux/nodemask.h and follow the style of
> the other nodemask APIs?

Ok, I see. I have no strong opinion. I like to have the API looking
consistent, but I also like to have all functions of the same family
together. If we move nearest_node_nodemask to linux/nodemask.h, it
will help with consistency, but will separate it from the sibling
numa_nearest_node().

So, at your discretion. If you don't want to change anything - I'm OK
with that.

This is anyways the very final nits, and I feel like the series now is
in a good shape, almost ready to be merged.

Thanks,
Yury