Re: [PATCH] mm: rename and document alloc_pages_exact_node

[Vlastimil Babka <vbabka@suse.cz>]

On 07/21/2015 11:31 PM, David Rientjes wrote:
> On Tue, 21 Jul 2015, Vlastimil Babka wrote:
> 
>> The function alloc_pages_exact_node() was introduced in 6484eb3e2a81 ("page
>> allocator: do not check NUMA node ID when the caller knows the node is valid")
>> as an optimized variant of alloc_pages_node(), that doesn't allow the node id
>> to be -1. Unfortunately the name of the function can easily suggest that the
>> allocation is restricted to the given node. In truth, the node is only
>> preferred, unless __GFP_THISNODE is among the gfp flags.
>>
>> The misleading name has lead to mistakes in the past, see 5265047ac301 ("mm,
>> thp: really limit transparent hugepage allocation to local node") and
>> b360edb43f8e ("mm, mempolicy: migrate_to_node should only migrate to node").
>>
>> To prevent further mistakes, this patch renames the function to
>> alloc_pages_prefer_node() and documents it together with alloc_pages_node().
>>
> 
> alloc_pages_exact_node(), as you said, connotates that the allocation will
> take place on that node or will fail.  So why not go beyond this patch and
> actually make alloc_pages_exact_node() set __GFP_THISNODE and then call
> into a new alloc_pages_prefer_node(), which would be the current
> alloc_pages_exact_node() implementation, and then fix up the callers?

OK, but then we have alloc_pages_node(), alloc_pages_prefer_node() and
alloc_pages_exact_node(). Isn't that a bit too much? The first two
differ only in tiny bit:

static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
                                                unsigned int order)
{
        /* Unknown node is current node */
        if (nid < 0)
                nid = numa_node_id();

        return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
}

static inline struct page *alloc_pages_prefer_node(int nid, gfp_t gfp_mask,
                                                unsigned int order)
{
        VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES || !node_online(nid));

        return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
}

So _prefer_node is just a tiny optimization over the other one. It
should be maybe called __alloc_pages_node() then? This would perhaps
discourage users outside of mm/arch code (where it may matter). The
savings of a skipped branch is likely dubious anyway... It would be also
nice if alloc_pages_node() could use __alloc_pages_node() internally, but
I'm not sure if all callers are safe wrt the
VM_BUG_ON(!node_online(nid)) part.

So when the alloc_pages_prefer_node is diminished as __alloc_pages_node
or outright removed, then maybe alloc_pages_exact_node() which adds
__GFP_THISNODE on its own, might be a useful wrapper. But I agree with
Christoph it's a duplication of the gfp_flags functionality and I don't
think there would be many users left anyway.