Re: [PATCH] mm/mm_init: handle alloc_percpu failure in free_area_init_core_hotplug

[David Hildenbrand (Arm)]

On 7/1/26 17:05, Gregory Price wrote:
> On Wed, Jul 01, 2026 at 10:35:41AM +0200, David Hildenbrand (Arm) wrote:
>>
>>>  	 * The node we allocated has no zone fallback lists. For avoiding
>>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>>> index 306ea5c13f54..37fd64ce144d 100644
>>> --- a/mm/mm_init.c
>>> +++ b/mm/mm_init.c
>>> @@ -1536,7 +1536,7 @@ void __init set_pageblock_order(void)
>>>   * NOTE: this function is only called during memory hotplug
>>>   */
>>>  #ifdef CONFIG_MEMORY_HOTPLUG
>>> -void __ref free_area_init_core_hotplug(struct pglist_data *pgdat)
>>> +int __ref free_area_init_core_hotplug(struct pglist_data *pgdat)
>>>  {
>>>  	int nid = pgdat->node_id;
>>>  	enum zone_type z;
>>> @@ -1544,8 +1544,14 @@ void __ref free_area_init_core_hotplug(struct pglist_data *pgdat)
>>>  
>>>  	pgdat_init_internals(pgdat);
>>>  
>>> -	if (pgdat->per_cpu_nodestats == &boot_nodestats)
>>> -		pgdat->per_cpu_nodestats = alloc_percpu(struct per_cpu_nodestat);
>>> +	if (pgdat->per_cpu_nodestats == &boot_nodestats) {
>>> +		struct per_cpu_nodestat __percpu *p;
>>> +
>>> +		p = alloc_percpu(struct per_cpu_nodestat);
>>> +		if (!p)
>>> +			return -ENOMEM;
>>> +		pgdat->per_cpu_nodestats = p;
>>
>> Is there a need for the temporary variable?
>>
> 
> at start:
>     pgdat->per_cpu_nodestats = &boot_nodestats 
> 
> So need a tmp to do the swap/revert/etc

Reviewing too many patches today ...  makes sense.

> 
>> Also how to handle cleanup on error? Or why can we skip cleanup? (what happens
>> if we get another call to __try_online_node()  later?)
>>
> 
> -ENOMEM ->
>     hotadd_init_pgdat -> NULL
>         __try_online_node -> -ENOMEM
> 	    pr_err("Cannot online node %d due to NULL pgdat\n", nid);
> 	    try_online_node -> -ENOMEM
> 	        __add_memory_resource  ->  error_memblock_remove:
> 
> Basically we're left exactly where we were before we made the attempt.
> 
> Another call should work just fine after a bunch of dmesg spew.

Okay, so the additional pgdat_init_internals) should be ok.

Makes sense to me, thanks.

-- 
Cheers,

David