Re: [PATCH] mm/mempolicy: recompute automatic interleave weights on memory hotplug

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Sat, 24 Oct 2026 13:04:15 -0400 "Gregory Price (Meta)" <gourry@gourry.net> wrote:

> In auto mode, weighted interleave derives each node's weight from its
> bandwidth, reduced over the nodes in N_MEMORY. HMAT and CXL report a
> node's bandwidth once, when the node first gains memory, and nothing
> else on the hotplug path recomputes the weights.
> 
> When a node loses its memory, the other nodes keep weights computed
> with its bandwidth until another node reports. If one does while the
> node is away, the reduction skips the node and resets its weight to 1,
> which it keeps once its memory returns.
> 
> With HMAT bandwidths of 100G for nodes 0-3, and 50G and 25G for kmem
> nodes 4 and 5, weighted_interleave/nodeN reads:
> 
>   event             node 0-3   node 4   node 5
>   node 4 online         7         3        -
>   node 4 removed        7         -        -
>   node 5 online         7         -        1
>   node 4 back           7         1        1
> 
> Toggling weighted_interleave/auto to 0 and back to 1 gives the correct
> 6, 3 and 1.
> 
> Recompute the weights from the recorded bandwidth whenever a node gains
> its first memory or loses its last.
> 
> mempolicy_set_node_perf() now only records the bandwidth and calls the
> same helper, so the bandwidth is kept even if allocating the new weights
> fails. wi_state_lock is dropped between the two steps, which should be
> safe because the helper reads node_bw_table and the mode under the lock,
> so a racing writer only makes it work from newer state.
> 
> With this patch, weighted_interleave/nodeN reads 6, 3 and 1 as soon as
> node 4 is back.

Ouch, thanks for finding this fix Gregory. In addition to making the
code correct, I think it's a nice simplification to
mempolicy_set_node_perf() since we no longer have the nested rollbacks.

> Fixes: e341f9c3c841 ("mm/mempolicy: Weighted Interleave Auto-tuning")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gregory Price (Meta) <gourry@gourry.net>
> ---
>  mm/mempolicy.c | 66 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 0884c629670d..ffb293a4cc56 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -238,27 +238,45 @@ static void reduce_interleave_weights(unsigned int *bw, u8 *new_iw)
>  		new_iw[nid] /= iw_gcd;
>  }
>  
> +/* Recompute the weights from node_bw_table, unless they were set manually */
> +static int wi_state_refresh(void)
> +{
> +	struct weighted_interleave_state *new_wi_state, *old_wi_state;
> +	int i;
> +
> +	new_wi_state = kmalloc_flex(*new_wi_state, iw_table, nr_node_ids);
> +	if (!new_wi_state)
> +		return -ENOMEM;
> +	new_wi_state->mode_auto = true;
> +	for (i = 0; i < nr_node_ids; i++)
> +		new_wi_state->iw_table[i] = 1;
> +
> +	mutex_lock(&wi_state_lock);
> +	old_wi_state = rcu_dereference_protected(wi_state,
> +					lockdep_is_held(&wi_state_lock));
> +	/* NULL wi_state assumes auto=true */
> +	if (!node_bw_table || (old_wi_state && !old_wi_state->mode_auto)) {

I was also a bit confused by this because at first I thought it was a
simple code move, but this seems to be a new condition.

I traced the path in mempolicy_set_node_perf because I think 
node_bw_table must actually always be non-NULL here......

Aha! But it can indeed be NULL at the new callsite.

Thanks! This looks good to me. With or without David's suggested
pr_warn_once(), I think the change looks good to me : -) so please feel
free to add:

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

Thanks a lot Gregory, have a great day!
Joshua

> +		mutex_unlock(&wi_state_lock);
> +		kfree(new_wi_state);
> +		return 0;
> +	}
> +	reduce_interleave_weights(node_bw_table, new_wi_state->iw_table);
> +	rcu_assign_pointer(wi_state, new_wi_state);
> +	mutex_unlock(&wi_state_lock);
> +	wi_state_retire(old_wi_state);
> +	return 0;
> +}
> +
>  int mempolicy_set_node_perf(unsigned int node, struct access_coordinate *coords)
>  {
> -	struct weighted_interleave_state *new_wi_state, *old_wi_state = NULL;
>  	unsigned int *old_bw, *new_bw;
>  	unsigned int bw_val;
> -	int i;
>  
>  	bw_val = min(coords->read_bandwidth, coords->write_bandwidth);
>  	new_bw = kcalloc(nr_node_ids, sizeof(unsigned int), GFP_KERNEL);
>  	if (!new_bw)
>  		return -ENOMEM;
>  
> -	new_wi_state = kmalloc_flex(*new_wi_state, iw_table, nr_node_ids);
> -	if (!new_wi_state) {
> -		kfree(new_bw);
> -		return -ENOMEM;
> -	}
> -	new_wi_state->mode_auto = true;
> -	for (i = 0; i < nr_node_ids; i++)
> -		new_wi_state->iw_table[i] = 1;
> -
>  	/*
>  	 * Update bandwidth info, even in manual mode. That way, when switching
>  	 * to auto mode in the future, iw_table can be overwritten using
> @@ -271,25 +289,10 @@ int mempolicy_set_node_perf(unsigned int node, struct access_coordinate *coords)
>  		memcpy(new_bw, old_bw, nr_node_ids * sizeof(*old_bw));
>  	new_bw[node] = bw_val;
>  	node_bw_table = new_bw;
> -
> -	old_wi_state = rcu_dereference_protected(wi_state,
> -					lockdep_is_held(&wi_state_lock));
> -	if (old_wi_state && !old_wi_state->mode_auto) {
> -		/* Manual mode; skip reducing weights and updating wi_state */
> -		mutex_unlock(&wi_state_lock);
> -		kfree(new_wi_state);
> -		goto out;
> -	}
> -
> -	/* NULL wi_state assumes auto=true; reduce weights and update wi_state*/
> -	reduce_interleave_weights(new_bw, new_wi_state->iw_table);
> -	rcu_assign_pointer(wi_state, new_wi_state);
> -
>  	mutex_unlock(&wi_state_lock);
> -	wi_state_retire(old_wi_state);
> -out:
> +
>  	kfree(old_bw);
> -	return 0;
> +	return wi_state_refresh();
>  }
>  
>  /**
> @@ -3900,8 +3903,15 @@ static int wi_node_notifier(struct notifier_block *nb,
>  	case NODE_REMOVED_LAST_MEMORY:
>  		sysfs_wi_node_delete(nid);
>  		break;
> +	default:
> +		return NOTIFY_OK;
>  	}
>  
> +	/*
> +	 * HMAT and CXL report a node's bandwidth only when it first gains
> +	 * memory, so recompute the weights whenever N_MEMORY changes.
> +	 */
> +	wi_state_refresh();
>  	return NOTIFY_OK;
>  }
>  
> -- 
> 2.56.0