Re: [PATCH 1/2] nodemask: Introduce for_each_node_mask_wrap/for_each_node_state_wrap()

[Andrea Righi <arighi@nvidia.com>]

On Fri, Nov 29, 2024 at 11:27:37AM -0800, Yury Norov wrote:
> On Fri, Nov 29, 2024 at 06:54:31PM +0100, Andrea Righi wrote:
> > Introduce NUMA node iterators to support circular iteration, starting
> > from a specified node.
> >
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Andrea Righi <arighi@nvidia.com>
> > ---
> >  include/linux/nodemask.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
> > index b61438313a73..c99cea40dfac 100644
> > --- a/include/linux/nodemask.h
> > +++ b/include/linux/nodemask.h
> > @@ -392,6 +392,16 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
> >       for ((node) = 0; (node) < 1 && !nodes_empty(mask); (node)++)
> >  #endif /* MAX_NUMNODES */
> >
> > +#if MAX_NUMNODES > 1
> > +#define for_each_node_mask_wrap(node, nodemask, start)                       \
> > +     for_each_set_bit_wrap((node), (nodemask)->bits, MAX_NUMNODES, (start))
> > +#else /* MAX_NUMNODES == 1 */
> > +#define for_each_node_mask_wrap(node, mask, start)                   \
> 
> There's a very well made historical mess of how nodemasks are
> implemented. Contrary to bitmaps and cpumasks, we pass nodemasks by
> value, not by pointer. For example, try_to_free_low() in mm/hugetlb.c
> takes a pointer, but has to 'dereference' it before passing to
> for_each_node_mask():
> 
>   static void try_to_free_low(struct hstate *h, unsigned long count,
>                                                   nodemask_t *nodes_allowed)
>   {
>         for_each_node_mask(i, *nodes_allowed) {
>                 ...
>         }
>   }
> 
> That's because all nodemask functions takes an address from a variable
> provided. For example the below nodes_empty() is implemented like:
> 
>   #define nodes_empty(src) __nodes_empty(&(src), MAX_NUMNODES)
>   static __always_inline bool __nodes_empty(const nodemask_t *srcp, unsigned int nbits)
>   {
>           return bitmap_empty(srcp->bits, nbits);
>   }
> 
> It means that your 'MAX_NUMNODES > 1' version doesn't match the
> existing for_each_node_mask(), i.e. doesn't pass a nodemask by value.
> The opencoded 'MAX_NUMNODES == 1' version does, although.

Thanks for the detailed clarification! I'll change
for_each_node_mask_wrap() to pass the nodemask by value.

> 
> > +     for ((node) = 0;                                                \
> > +          (node) < 1 && !nodes_empty(mask);                          \
> > +          (node)++, (void)(start), (void)(cnt))
> 
> This cnt is a leftover from v1, I guess.

Indeed! Thanks for noticing it (my bad for not testing the build with
CONFIG_NUMA off), will fix this.

> 
> > +#endif /* MAX_NUMNODES */
> > +
> >  /*
> >   * Bitmasks that are kept for all the nodes.
> >   */
> > @@ -441,6 +451,9 @@ static inline int num_node_state(enum node_states state)
> >  #define for_each_node_state(__node, __state) \
> >       for_each_node_mask((__node), node_states[__state])
> >
> > +#define for_each_node_state_wrap(__node, __state, __start) \
> > +     for_each_node_mask_wrap((__node), &node_states[__state], __start)
> 
> Can you also add for_each_online_node_wrap() to align with the
> existing for_each_online_node()?

Ok.

> 
> > +
> >  #define first_online_node    first_node(node_states[N_ONLINE])
> >  #define first_memory_node    first_node(node_states[N_MEMORY])
> >  static inline unsigned int next_online_node(int nid)
> > --
> > 2.47.1

Thanks,
-Andrea