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
