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

["Zi Yan" <ziy@nvidia.com>]

On Tue Nov 3, 2026 at 9:57 AM EST, Vlastimil Babka (SUSE) wrote:
> On 11/1/26 21:07, Zi Yan wrote:
>> On Sat Oct 31, 2026 at 10:13 PM EDT, Gregory Price (Meta) wrote:
>>> From: Josef Bacik <josef@toxicpanda.com>
>>>
>>> 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@vger.kernel.org>
>>> Assisted-by: LLM
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
>>> Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>
>>> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>>> Signed-off-by: Gregory Price (Meta) <gourry@gourry.net>
>>> ---
>>>  mm/page_alloc.c | 20 ++++++++++++++------
>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>> 
>> LGTM. Just a nit below:
>> 
>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>> 
>>>
>>> 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.
>
> Hmm.
>
>>> +	if (zone->per_cpu_pageset && zone->per_cpu_pageset != &boot_pageset)
>>> +		return;
>
> Shouldn't then the check be just:
>
> if (zone->per_cpu_pageset)
> 	return;
>
> Because if it's == &boot_pageset, we are just setting the same values that
> area already there?
>

Right. And zone_pcp_init(), called by zone_init_internals(), becomes a nop for
free_area_init_core_hotplug(). Then I wonder if zone_init_internals() is
needed for hotplug, but that belongs to a later cleanup.


-- 
Best Regards,
Yan, Zi