linux-kernel - Re: [PATCH 2/7] mm/numa: Introduce nearest_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z69pi2KDLB5eZ29A@thinkpad>
Date: Fri, 14 Feb 2025 11:04:27 -0500
From: Yury Norov <yury.norov@...il.com>
To: Andrea Righi <arighi@...dia.com>
Cc: Tejun Heo <tj@...nel.org>, David Vernet <void@...ifault.com>,
	Changwoo Min <changwoo@...lia.com>, Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Joel Fernandes <joel@...lfernandes.org>, Ian May <ianm@...dia.com>,
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] mm/numa: Introduce nearest_node_nodemask()

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