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

[Zi Yan]

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?

>
> 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.



Best Regards,
Yan, Zi