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

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 <[email protected]>
>>>> Reviewed-by: Vlastimil Babka (SUSE) <[email protected]>
>>>> Signed-off-by: Brendan Jackman <[email protected]>
>>>> ---
>>>>  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.

> 
> Best Regards,
> Yan, Zi