Re: [PATCH 4/4] mm/mempolicy: enhance weighted interleave with package-aware locality

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Package-aware weighted interleave silently degrades to flat mode on memoryless CPU nodes because they are not registered in the package topology.
- [Medium] Unstabilized TOCTOU access of `policy->nodes` in `policy_resolve_package_nodes()` can cause torn reads and inconsistent node intersection if the policy is concurrently modified.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the function `alloc_pages_bulk_weighted_interleave` unconditionally uses `GFP_KERNEL` for a `kzalloc` call, which can cause a sleep-in-atomic bug if invoked with a non-blocking `gfp` mask.
--

--- Patch [4]: [PATCH 4/4] mm/mempolicy: enhance weighted interleave with package-aware locality ---
commit 9bf854e133ba0fbe547595b9d47a304b6f619169
Author: Rakie Kim <[email protected]>

mm/mempolicy: enhance weighted interleave with package-aware locality

This commit introduces a package-aware mode for weighted interleave, aiming
to prioritize local nodes within the same package over remote nodes to
preserve bandwidth. The mode is opt-in via sysfs and restricts allocations
to the NUMA nodes of the task's current package.

> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 19417b0afc30b..66bccb9a0a191 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c

[ ... ]

> @@ -2138,17 +2146,97 @@ bool apply_policy_zone(struct mempolicy *policy, enum zone_type zone)
>  	return zone >= dynamic_policy_zone;
>  }
>  
> +/**
> + * policy_resolve_package_nodes - Restrict policy nodes to the current package
[ ... ]
> + */
> +static int policy_resolve_package_nodes(struct mempolicy *policy, nodemask_t *mask)
> +{
> +	nodemask_t package_mask;
> +	int node, ret;
> +
> +	if (!policy || !mask)
> +		return -EINVAL;
> +
> +	nodes_clear(*mask);
> +
> +	node = numa_node_id();
> +	ret = mp_get_package_nodes(node, &package_mask);

[Severity: High]
Will mp_get_package_nodes() fail and return an error for memoryless CPU
nodes?

Since memoryless CPU nodes are excluded from being registered into memory
packages by memory_package_init() because they lack N_MEMORY, it appears
this lookup will fail. Does this cause the package-aware interleave feature
to silently degrade and fall back to the unconstrained flat mode when a task
executes on a CPU node with no local memory (such as a compute die accessing
separate CXL memory)?

> +	if (ret)
> +		return ret;
> +
> +	nodes_and(*mask, package_mask, policy->nodes);

[Severity: Medium]
Is it safe to directly read policy->nodes multiple times here without first
stabilizing it into a local copy?

If policy->nodes is concurrently modified (for example, via sys_mbind() on a
shared VMA policy), a modification occurring between this first nodes_and()
check and the fallback first_node() or nodes_and() calls below might cause
the logic to observe mixed states.

Could this result in an inconsistent node intersection or an incorrect
fallback node being selected?

> +	if (!nodes_empty(*mask))
> +		return 0;
> +
> +	/*
> +	 * The user's nodemask excludes every node of the current package;
> +	 * fall back to the package spanned by the user's own first node.
> +	 */
> +	node = first_node(policy->nodes);
> +	ret = mp_get_package_nodes(node, &package_mask);
> +	if (ret)
> +		return ret;
> +
> +	nodes_and(*mask, package_mask, policy->nodes);
> +	if (nodes_empty(*mask))
> +		return -ENOENT;
> +
> +	return 0;
> +}

[ ... ]

> @@ -2695,7 +2807,7 @@ static unsigned long alloc_pages_bulk_weighted_interleave(gfp_t gfp,
>  	/* read the nodes onto the stack, retry if done during rebind */
>  	do {
>  		cpuset_mems_cookie = read_mems_allowed_begin();
> -		nnodes = read_once_policy_nodemask(pol, &nodes);
> +		nnodes = read_once_policy_package_nodemask(pol, &nodes);
>  	} while (read_mems_allowed_retry(cpuset_mems_cookie));
>  
>  	/* if the nodemask has become invalid, we cannot do anything */

[Severity: High]
This isn't a bug introduced by this patch, but does this function safely
handle atomic allocations?

Further down in alloc_pages_bulk_weighted_interleave(), the weights array
is allocated unconditionally with GFP_KERNEL:

    weights = kzalloc(nr_node_ids, GFP_KERNEL);
    if (!weights)
            return total_allocated;

If alloc_pages_bulk() is called from an atomic context with a non-blocking
mask like GFP_ATOMIC or GFP_NOWAIT, will ignoring the passed gfp argument and
using GFP_KERNEL here cause a 'scheduling while atomic' panic or deadlock?

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