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 <[email protected]>
>>
>> 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: <[email protected]>
>> Assisted-by: LLM
>> Signed-off-by: Josef Bacik <[email protected]>
>> Signed-off-by: David Hildenbrand (Arm) <[email protected]>
>> Reviewed-by: Johannes Weiner <[email protected]>
>> Signed-off-by: Vlastimil Babka (SUSE) <[email protected]>
>> Signed-off-by: Gregory Price (Meta) <[email protected]>
>> ---
>> mm/page_alloc.c | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> LGTM. Just a nit below:
>
> Reviewed-by: Zi Yan <[email protected]>
>
>>
>> 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
>
>
