Re: [PATCH] mm/mempolicy: take a cpuset cookie for the interleave node count

[Huang, Ying]

Gregory Price <gourry@xxxxxxxxxx> writes:

> alloc_pages_bulk_interleave() counts pol->nodes without a cpuset cookie:
>
> 	nodes = nodes_weight(pol->nodes);
> 	nr_pages_per_node = nr_pages / nodes;
>
> nodemask_t spans several words once MAX_NUMNODES exceeds BITS_PER_LONG, so
> a concurrent cpuset rebind can tear that read and yield an empty mask even
> though neither version of it was empty.  The call then allocates nothing
> and returns 0.
>
> Some compilers will hoist the loop entry test above the division,
> because nr_pages_per_node is dead when the loop does not run.
>
> 	682e:	call	...		<- nodes_weight()
> 	6838:	test	%eax,%eax
> 	683a:	jle	692d		<- nodes <= 0 skips the loop
> 	684a:	div	%rcx
>
> So in most deployments, this div/0 is unreachable - but nothing in the
> source guarantees that, it's just not easily exercised.
>
> Take the cookie around the count and bail if the mask really is empty.
> Only the count needs it, interleave_nodes() takes the cookie itself so
> so a torn read there is already retried.
>
> A rebind landing mid-loop can still leave the count disagreeing with the
> mask, so the loop may revisit a node or skip one - but a rebind where
> nodes change causes migration, so a handful of misplaced pages isn't
> catastrophic in any sense.
>
> Measured on a 72 node VM (NODES_SHIFT=10) with a cgroup v2 cpuset flipping
> cpuset.mems between a word 0 and a word 1 node set, and the two word read
> artificially widened: 330 zero counts in 130414 calls without the cookie,
> and 401 retries with it.
>
> Reported-by: Chelsy Ratnawat <chelsyratnawat2001@xxxxxxxxx>
> Link: https://lore.kernel.org/all/20250907160829.91628-1-chelsyratnawat2001@xxxxxxxxx/
> Fixes: c00b6b961099 ("mm/vmalloc: introduce alloc_pages_bulk_array_mempolicy to accelerate memory allocation")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
> ---
>  mm/mempolicy.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 79053ece02cd..060a0eb26917 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2592,6 +2592,7 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
>  		struct mempolicy *pol, unsigned long nr_pages,
>  		struct page **page_array)
>  {
> +	unsigned int cpuset_mems_cookie;
>  	int nodes;
>  	unsigned long nr_pages_per_node;
>  	int delta;
> @@ -2599,7 +2600,16 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
>  	unsigned long nr_allocated;
>  	unsigned long total_allocated = 0;
>  
> -	nodes = nodes_weight(pol->nodes);
> +	/* count the nodes, retry if a rebind happened during the read */
> +	do {
> +		cpuset_mems_cookie = read_mems_allowed_begin();
> +		nodes = nodes_weight(pol->nodes);
> +	} while (read_mems_allowed_retry(cpuset_mems_cookie));
> +
> +	/* if the nodemask has become invalid, we cannot do anything */
> +	if (!nodes)
> +		return 0;
> +
>  	nr_pages_per_node = nr_pages / nodes;
>  	delta = nr_pages - nodes * nr_pages_per_node;

LGTM, feel free to add my

Reviewed-by: Huang Ying <ying.huang@xxxxxxxxxxxxxxxxx>

in the future version.

---
Best Regards,
Huang, Ying