Re: [PATCH v5 02/12] mm/page_alloc: keep the pagesets of an empty zone

[Zi Yan]

On Sat Oct 31, 2026 at 10:13 PM EDT, Gregory Price (Meta) wrote:
> From: Josef Bacik <josef@xxxxxxxxxxxxxx>
>
> When the last memory of a zone goes offline, zone_pcp_reset() frees
> zone->per_cpu_pageset and zone->per_cpu_zonestats. Nothing waits for those
> who found the zone populated and still use the two pointers. They take
> neither the memory hotplug lock nor any other lock which offline_pages()
> holds, and some of them can sleep:
>
>   CPU0 vmstat_update()                CPU1 offline_pages()
>   refresh_cpu_vm_stats()
>     for_each_populated_zone(zone)
>       pcp = zone->per_cpu_pageset
>       cond_resched()
>                                       adjust_present_page_count()
>                                       zone_pcp_reset(zone)
>                                         free_percpu(zone->per_cpu_pageset)
>       decay_pcp_high(zone, pcp)
>       __this_cpu_write(pcp->expire, 0)
>
> The same goes for drain_all_pages(NULL), which takes pcp->lock, for
> zone_pcp_update(), show_free_areas() and the readers and writers of the
> zonestats. If the per-cpu memory has got a new owner in the meantime, it
> is corrupted.
>
> The pagesets are only freed because setup_zone_pageset() would otherwise
> leak them when the zone gets memory again. Keep them instead and let
> setup_zone_pageset() and zone_pcp_init() use them again, like it is done
> with pgdat->per_cpu_nodestats. Their lists are empty, and their limits
> have been calculated for a zone without memory by the
> init_per_zone_wmark_min() of offline_pages(), which is what
> setup_zone_pageset() would come up with as well.
>
> Fixes: 340175b7d14d ("mm/hotplug: free zone->pageset when a zone becomes empty")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Reviewed-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
> ---
>  mm/page_alloc.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

LGTM. Just a nit below:

Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f1048168c90c..1850a41f24f7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6308,6 +6308,10 @@ void __meminit setup_zone_pageset(struct zone *zone)
>  {
>  	int cpu;
>  
> +	/* The zone has been populated before, see zone_pcp_reset(). */
> +	if (zone->per_cpu_pageset != &boot_pageset)
> +		return;
> +
>  	/* Size may be 0 on !SMP && !NUMA */
>  	if (sizeof(struct per_cpu_zonestat) > 0)
>  		zone->per_cpu_zonestats = alloc_percpu(struct per_cpu_zonestat);
> @@ -6400,6 +6404,10 @@ void __init setup_per_cpu_pageset(void)
>  
>  __meminit void zone_pcp_init(struct zone *zone)
>  {
> +	/* A node which is added again, see zone_pcp_reset(). */

it might be better to explain zone->per_cpu_pageset can only be NULL at
boot time and it needs an initialization.

> +	if (zone->per_cpu_pageset && zone->per_cpu_pageset != &boot_pageset)
> +		return;
> +
>  	/*
>  	 * per cpu subsystem is not up at this point. The following code
>  	 * relies on the ability of the linker to provide the


-- 
Best Regards,
Yan, Zi