Re: [PATCH 2/2] mm/mempolicy: stop copying the nodemask in the interleave paths

[David Hildenbrand (Arm)]

On 8/29/26 03:59, Gregory Price wrote:
> The interleave node selectors copy pol->nodes onto the stack so the mask
> cannot change while they walk it.  nodemask_t is 128 bytes at
> MAX_NUMNODES=1024, and two of the three run per folio fault.
> 
> The copy only buys consistency between the node count and the walk.
> Drop the consistency and just bounds check the walk instead.

I think you should document here that accessing the node bitnmap is safe (is RCU
responsible for that? I think yes), but it can get updated concurrently.

> 
> If an empty nodelist or weight is perceived, fall back to numa_node_id(),
> which is what the functions already did when the copy came back empty.

Makes sense.

> 
> weighted_interleave_nid() counts the nodes as we sum the weights. We use
> that node count to limit the maximum skew a single node can host.
> 
> interleave_nid() walks with next_node_in() rather than next_node(), so a
> mask that shrank mid-walk wraps to a node still in the policy.
> 
> alloc_pages_bulk_weighted_interleave() derives per-node counts from a
> weight total summed over the mask, so a changing mask can make them exceed
> the request.  Clamp each chunk to the space left in page_array.
> 
> A cpuset cookie will not work here: two of these take VMA policies, which
> mpol_rebind_mm() rebinds under mmap_write_lock(), not mems_allowed_seq.
> 
> Cost is distribution accuracy during a rebind - but the copy never
> corrected this anyway, it was just a safety mechanism to prevent div/0
> and overrunning the alloc request buffer.
> 
> Remove read_once_policy_nodemask(), now unused.
> 
> -fstack-usage at MAX_NUMNODES=1024:
> 
>   weighted_interleave_nid            184 -> 56
>   interleave_nid                     168 -> 32
>   alloc_pages_bulk_mempolicy_noprof  360 -> 136

Nice!

>  static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
>  {
>  	struct weighted_interleave_state *state;
> -	nodemask_t nodemask;
> -	unsigned int target, nr_nodes;
> +	unsigned int target, nnodes = 0;
>  	u8 *table = NULL;
>  	unsigned int weight_total = 0;
>  	u8 weight;
>  	int nid = 0;
>  
> -	nr_nodes = read_once_policy_nodemask(pol, &nodemask);
> -	if (!nr_nodes)
> -		return numa_node_id();
> -
>  	rcu_read_lock();
>  
>  	state = rcu_dereference(wi_state);
> @@ -2232,22 +2213,40 @@ static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
>  	if (state)
>  		table = state->iw_table;
>  
> -	/* calculate the total weight */
> -	for_each_node_mask(nid, nodemask)
> +	/* calculate the total weight and the node count */
> +	for_each_node_mask(nid, pol->nodes) {
>  		weight_total += table ? table[nid] : 1;
> +		nnodes++;
> +	}
> +
> +	/* the mask is empty */
> +	if (!weight_total) {
> +		rcu_read_unlock();
> +		return numa_node_id();
> +	}
>  
>  	/* Calculate the node offset based on totals */
>  	target = ilx % weight_total;
> -	nid = first_node(nodemask);
> -	while (target) {
> +	nid = first_node(pol->nodes);
> +
> +	/*
> +	 * The target was calculated in a separate loop, and a concurrent
> +	 * rebind can change the total number of nodes.  Clamp this loop to
> +	 * a single pass (nnodes) to keep the walk bounded by node count.

Might want to explicitly comment here that we are looking at a moving target and
might race with node bitmap modifications.

> +	 */
> +	while (target && nnodes-- && nid < MAX_NUMNODES) {
>  		/* detect system default usage */
>  		weight = table ? table[nid] : 1;
>  		if (target < weight)
>  			break;
>  		target -= weight;
> -		nid = next_node_in(nid, nodemask);
> +		nid = next_node_in(nid, pol->nodes);
>  	}
>  	rcu_read_unlock();
> +
> +	/* the mask emptied under the walk */
> +	if (nid >= MAX_NUMNODES)
> +		return numa_node_id();

Makes sense.

>  	return nid;
>  }
>  
> @@ -2258,18 +2257,21 @@ static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
>   */
>  static unsigned int interleave_nid(struct mempolicy *pol, pgoff_t ilx)
>  {
> -	nodemask_t nodemask;
>  	unsigned int target, nnodes;
>  	int i;
>  	int nid;
>  
> -	nnodes = read_once_policy_nodemask(pol, &nodemask);
> +	nnodes = nodes_weight(pol->nodes);
>  	if (!nnodes)
>  		return numa_node_id();
>  	target = ilx % nnodes;
> -	nid = first_node(nodemask);
> -	for (i = 0; i < target; i++)
> -		nid = next_node(nid, nodemask);
> +	nid = first_node(pol->nodes);

Similarly, I wonder whether we should spell out the raciness.

[...]


Nothing jumped at me

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David