Re: [PATCH v5 29/36] mm: add NODE_PRIVATE_CAP_DEMOTION for private-node tiering demotion

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent, unprotected access to the global `tierable_nodes` nodemask can result in a torn read.
--

--- Patch [29]: [PATCH v5 29/36] mm: add NODE_PRIVATE_CAP_DEMOTION for private-node tiering demotion ---
commit e4de60011eb318a533982e5d4b896e77cc0ad141
Author: Gregory Price <[email protected]>

mm: add NODE_PRIVATE_CAP_DEMOTION for private-node tiering demotion

This commit introduces NODE_PRIVATE_CAP_DEMOTION, allowing private memory
nodes to be included in the tiering and demotion hierarchy. By opting into
this capability, memory-tiers will include the private node in demotion
paths, allowing page reclaim to target it.

> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> index 25e121851b586..c673080d153e4 100644
> --- a/mm/memory-tiers.c
> +++ b/mm/memory-tiers.c
> @@ -317,6 +318,21 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
>  	rcu_read_unlock();
>  }
>  
> +/* Tiering set: N_MEMORY | (N_MEMORY_PRIVATE w/ CAP_DEMOTION) */
> +static nodemask_t tierable_nodes;
> +
> +static void update_tierable_nodes(void)
> +{
> +	int node;
> +
> +	lockdep_assert_held_once(&memory_tier_lock);
> +
> +	tierable_nodes = node_states[N_MEMORY];
> +	for_each_node_state(node, N_MEMORY_PRIVATE)
> +		if (node_allows_demotion(node))
> +			node_set(node, tierable_nodes);
> +}
> +

[ ... ]

> @@ -330,7 +346,7 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
>  int next_demotion_node(int node, const nodemask_t *allowed_mask)
>  {
>  	struct demotion_nodes *nd;
> -	nodemask_t mask;
> +	nodemask_t mask, tierable;
>  
>  	if (!node_demotion)
>  		return NUMA_NO_NODE;

[ ... ]

> @@ -370,7 +386,8 @@ int next_demotion_node(int node, const nodemask_t *allowed_mask)
>  	 * closest demotion target.
>  	 */
>  	nodes_complement(mask, *allowed_mask);
> -	return find_next_best_node_in(node, &mask, &node_states[N_MEMORY]);
> +	tierable = tierable_nodes;

[Severity: Medium]
Could this lockless copy result in a torn read of the tierable_nodes nodemask?

In update_tierable_nodes(), the global tierable_nodes mask is updated via a
non-atomic struct assignment and iterative node_set() modifications. Here in
next_demotion_node(), the mask is copied locklessly to the stack during the
reclaim hotpath.

Since nodemask_t can be up to 128 bytes depending on MAX_NUMNODES, concurrent
updates from memory hotplug could cause next_demotion_node() to see a
partially updated mask. This could cause page reclaim to transiently skip valid
private nodes or behave suboptimally during demotion.

> +	return find_next_best_node_in(node, &mask, &tierable);
>  }
>  
>  static void disable_all_demotion_targets(void)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=29