Message-ID: <89da1ae8-3062-1fd9-a389-de4791c117e3@bytedance.com>
Date:   Tue, 14 Feb 2023 11:14:18 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.7.2
Subject: Re: [PATCH] mm: page_alloc: call panic() when memoryless node
 allocation fails
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20230212111027.95520-1-zhengqi.arch@bytedance.com>
 <20230213105318.e2a83d60c12e734d45dccf1a@linux-foundation.org>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <20230213105318.e2a83d60c12e734d45dccf1a@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/2/14 02:53, Andrew Morton wrote:
> On Sun, 12 Feb 2023 19:10:27 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote:
> 
>> In free_area_init(), we will continue to run after pgdat of memoryless
>> node allocation fails. However, in the subsequent process (such as when
>> initializing zonelist), the case that NODE_DATA(nid) is NULL is not
>> handled, which will cause panic. Instead of this, it's better to call
>> panic() directly when the memory allocation fails during system boot.
>>
>> ...
>>
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -8360,11 +8360,9 @@ void __init free_area_init(unsigned long *max_zone_pfn)
>>   
>>   			/* Allocator not initialized yet */
>>   			pgdat = arch_alloc_nodedata(nid);
>> -			if (!pgdat) {
>> -				pr_err("Cannot allocate %zuB for node %d.\n",
>> -						sizeof(*pgdat), nid);
>> -				continue;
>> -			}
>> +			if (!pgdat)
>> +				panic("Cannot allocate %zuB for node %d.\n",
>> +				       sizeof(*pgdat), nid);
>>   			arch_refresh_nodedata(nid, pgdat);
>>   			free_area_init_memoryless_node(nid);
> 
> Have you actually hit this at runtime?  If so, is there something we

No, I just quickly tested the following code and found that it will
immediately crash in the subsequent position (many paths do not handle
this case):

@@ -8368,6 +8368,7 @@ void __init free_area_init(unsigned long 
*max_zone_pfn)
                                                 sizeof(*pgdat), nid);
                                 continue;
                         }
+                       continue;
                         arch_refresh_nodedata(nid, pgdat);
                         free_area_init_memoryless_node(nid);

> can do to handle this failure more gracefully?

My consideration is that if the memory of the size of the pgdat
structure (so small) cannot be allocated at the boot time, there
is no need to continue running. So I choose to call the panic()
directly.

Thanks,
Qi

>