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 <[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? >
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
