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

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

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.

> +	for ((node) = 0;						\
> +	     (node) < 1 && !nodes_empty(mask);				\
> +	     (node)++, (void)(start), (void)(cnt))

This cnt is a leftover from v1, I guess.

> +#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()?

> +
>  #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