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

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

On Wed, Feb 12, 2025 at 05:48:09PM +0100, Andrea Righi wrote:
> Introduce the new helper nearest_node_nodemask() to find the closest
> node in a specified nodemask from a given starting node.
> 
> Returns MAX_NUMNODES if no node is found.
> 
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Andrea Righi <arighi@nvidia.com>

Suggested-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>

> ---
>  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);
  }

>  #ifndef memory_add_physaddr_to_nid
>  int memory_add_physaddr_to_nid(u64 start);
>  #endif
> @@ -47,6 +49,11 @@ static inline int numa_nearest_node(int node, unsigned int state)
>  	return NUMA_NO_NODE;
>  }
>  
> +static inline int nearest_node_nodemask(int node, nodemask_t *mask)
> +{
> +	return NUMA_NO_NODE;
> +}
> +
>  static inline int memory_add_physaddr_to_nid(u64 start)
>  {
>  	return 0;
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 162407fbf2bc7..1e2acf187ea3a 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -196,6 +196,38 @@ int numa_nearest_node(int node, unsigned int state)
>  }
>  EXPORT_SYMBOL_GPL(numa_nearest_node);
>  
> +/**
> + * nearest_node_nodemask - Find the node in @mask at the nearest distance
> + *			   from @node.
> + *
> + * @node: the node to start the search from.
> + * @mask: a pointer to a nodemask representing the allowed nodes.
> + *
> + * This function iterates over all nodes in the given state and calculates
> + * the distance to the starting node.
> + *
> + * Returns the node ID in @mask that is the closest in terms of distance
> + * from @node, or MAX_NUMNODES if no node is found.
> + */
> +int nearest_node_nodemask(int node, nodemask_t *mask)
> +{
> +	int dist, n, min_dist = INT_MAX, min_node = MAX_NUMNODES;
> +
> +	if (node == NUMA_NO_NODE)
> +		return MAX_NUMNODES;

This makes it unclear: you make it legal to pass NUMA_NO_NODE, but
your function returns something useless. I don't think it would help
users in any reasonable scenario.

So, if you don't want user to call this with node == NUMA_NO_NODE,
just describe it in comment on top of the function. Otherwise, please
do something useful like 

	if (node == NUMA_NO_NODE)
		node = current_node;

I would go with option 1. Notice, node_distance() doesn't bother to
check against NUMA_NO_NODE.

> +	for_each_node_mask(n, *mask) {
> +		dist = node_distance(node, n);
> +		if (dist < min_dist) {
> +			min_dist = dist;
> +			min_node = n;
> +		}
> +	}
> +
> +	return min_node;
> +}
> +EXPORT_SYMBOL_GPL(nearest_node_nodemask);
> +
>  struct mempolicy *get_task_policy(struct task_struct *p)
>  {
>  	struct mempolicy *pol = p->mempolicy;
> -- 
> 2.48.1