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

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

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?

>>  	/*
>>  	 * per cpu subsystem is not up at this point. The following code
>>  	 * relies on the ability of the linker to provide the
> 
>