Re: [PATCH v5 13/18] mm: Remove __alloc_pages_node()

[Zi Yan]

On 13 Jul 2026, at 12:37, Vlastimil Babka (SUSE) wrote:

> On 7/13/26 17:54, Zi Yan wrote:
>> On 13 Jul 2026, at 11:26, Brendan Jackman wrote:
>>
>>> On Fri Jul 3, 2026 at 2:57 PM UTC, Zi Yan wrote:
>>>> On Fri Jul 3, 2026 at 8:31 AM EDT, Brendan Jackman wrote:
>>>>> There were only a few users, which have been removed. The only advantage
>>>>> of this API over alloc_pages_node() is avoiding a single conditional
>>>>> branch. The disadvantages are:
>>>>>
>>>>> 1. More API surface, more sources of confusion, more maintenance.
>>>>>
>>>>> 2. Worse impact of CPU hotplug bugs: most users of __alloc_pages_node()
>>>>>    were using the result of cpu_to_node(); if the CPU gets hotplugged
>>>>>    out this will return NUMA_NO_NODE. If one of these paths fails to
>>>>>    protect against a concurrent hotplug then page_alloc.c will use
>>>>>    NUMA_NO_NODE as an index into NODE_DATA() and cause some horrible
>>>>>    memory corruption or other. With alloc_pages_node(), the code might
>>>>>    just work fine.
>>>>>
>>>>> Ulterior motive: this frees up the __* variants of the allocator APIs to
>>>>> serve specifically for use as mm-internal API.
>>>>>
>>>>> Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
>>>>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>>>>> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
>>>>> ---
>>>>>  include/linux/gfp.h | 20 ++++----------------
>>>>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
>>>>> index 01d6d2591f49e..3bf55a5f9143e 100644
>>>>> --- a/include/linux/gfp.h
>>>>> +++ b/include/linux/gfp.h
>>>>> @@ -256,21 +256,6 @@ static inline void warn_if_node_offline(int this_node, gfp_t gfp_mask)
>>>>>  	dump_stack();
>>>>>  }
>>>>>
>>>>> -/*
>>>>> - * Allocate pages, preferring the node given as nid. The node must be valid and
>>>>> - * online. For more general interface, see alloc_pages_node().
>>>>> - */
>>>>> -static inline struct page *
>>>>> -__alloc_pages_node_noprof(int nid, gfp_t gfp_mask, unsigned int order)
>>>>> -{
>>>>> -	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
>>>>> -	warn_if_node_offline(nid, gfp_mask);
>>>>> -
>>>>> -	return __alloc_pages_noprof(gfp_mask, order, nid, NULL);
>>>>> -}
>>>>> -
>>>>> -#define  __alloc_pages_node(...)		alloc_hooks(__alloc_pages_node_noprof(__VA_ARGS__))
>>>>> -
>>>>>  static inline
>>>>>  struct folio *__folio_alloc_node_noprof(gfp_t gfp, unsigned int order, int nid)
>>>>>  {
>>>>> @@ -293,7 +278,10 @@ static inline struct page *alloc_pages_node_noprof(int nid, gfp_t gfp_mask,
>>>>>  	if (nid == NUMA_NO_NODE)
>>>>>  		nid = numa_mem_id();
>>>>>
>>>>> -	return __alloc_pages_node_noprof(nid, gfp_mask, order);
>>>>> +	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
>>>>
>>>> Could this become a VM_WARN_ON?
>>>
>>> Er, it will almost certainly crash later if this is violated. Personally
>>
>> You mean in warn_if_node_offline() when accessing the nodemask?
>>
>>> if I'm enabling DEBUG_VM I'd rather it crashes with a helpful stacktrace
>>> instead of chasing random poihnters, likely corrupting memory, and then
>>> crashing later in a completely undebuggable way instead.
>>
>> In this case, VM_WARN_ON + return NULL could work, right?
>
> See below.
>
>>>
>>> TBH I don't really understand the "don't BUG" culture we have in Linux.
>>> I usually just go along with it coz it doesn't seem important enough to
>>> argue about. But now I'm wondering: if we really don't like VM_BUG_ON()
>>> why not get rid of it completely? I wonder if there's some case-by-case
>>> difference that I'm not picking up on.
>>
>> My understanding is that issues like the above one should be caught
>> at development phase, so VM_WARN_ON() is sufficient.
>>
>> The source of “don’t BUG”: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1cfd9d7e43d5a1cf739d1420b10b1e65feb02f88
>> You can read the commit message to know when BUG_ON() can be used.
>
> Quoted from there:
>
>     I'd prefer to keep all these warnings 'simple' - i.e. no attempted
>     recovery & control flow, unless we ever expect these to trigger.
>     [5]
>
> So I wouldn't attempt recovery here.

Ah, missed that part. Yes, for this case, we do not expect the conditions
in VM_BUG_ON().


Best Regards,
Yan, Zi