Re: [PATCH V2] mm/hugetlb: try preferred node first when alloc gigantic page from cma

[Mike Kravetz <mike.kravetz@oracle.com>]

On 9/1/20 8:04 AM, Michal Hocko wrote:
> On Tue 01-09-20 22:49:24, Li Xinhai wrote:
>> Since commit cf11e85fc08cc6a4 ("mm: hugetlb: optionally allocate gigantic
>> hugepages using cma"), the gigantic page would be allocated from node
>> which is not the preferred node, although there are pages available from
>> that node. The reason is that the nid parameter has been ignored in
>> alloc_gigantic_page().
>>
>> Besides, the __GFP_THISNODE also need be checked if user required to
>> alloc only from the preferred node.
>>
>> After this patch, the preferred node is tried first before other allowed
>> nodes, and don't try to allocate from other nodes if __GFP_THISNODE is
>> specified.
>>
>> Fixes: cf11e85fc08cc6a4 ("mm: hugetlb: optionally allocate gigantic hugepages using cma")
>> Cc: Roman Gushchin <guro@fb.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> 
> LGTM
> Acked-by: Michal Hocko <mhocko@suse.com>

Thank you both for the updates!

>> ---
>> v1->v2:
>> With review by Mike and Michal, need to check __GFP_THISNODE to avoid
>> allocate from other nodes.
>>
>>  mm/hugetlb.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index a301c2d672bf..d24986145087 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1256,15 +1256,24 @@ static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
>>  		struct page *page;
>>  		int node;
>>  
>> -		for_each_node_mask(node, *nodemask) {
>> -			if (!hugetlb_cma[node])
>> -				continue;
>> -
>> -			page = cma_alloc(hugetlb_cma[node], nr_pages,
>> -					 huge_page_order(h), true);
>> +		if (nid != NUMA_NO_NODE && hugetlb_cma[nid]) {
>> +			page = cma_alloc(hugetlb_cma[nid], nr_pages,
>> +					huge_page_order(h), true);

I missed the NUMA_NO_NODE issue yesterday, but thought about it a bit today.
As Michal says, we do not call into alloc_gigantic_page with
'nid == NUMA_NO_NODE' today, but we should handle it correctly.

Other places in the hugetlb code such as alloc_buddy_huge_page and even the
low level interface alloc_pages_node have code as follows:

	if (nid == NUMA_NO_NODE)
		nid = numa_mem_id();

this attempts the allocation on the current node first if NUMA_NO_NODE is
specified.  Of course, it falls back to other nodes allowed in the mask.
If we are adding the code to interpret NUMA_NO_NODE, I suggest we make this
type of change as well.  This would simply be added at the beginning of
alloc_gigantic_page to handle the non-CMA case as well.  Suggestion for an
updated patch below.

-- 
Mike Kravetz


diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a301c2d672bf..98dc44a602b4 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1251,20 +1251,32 @@ static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
 {
 	unsigned long nr_pages = 1UL << huge_page_order(h);
 
+	if (nid == NUMA_NO_NODE)
+		nid =  numa_mem_id();
+
 #ifdef CONFIG_CMA
 	{
 		struct page *page;
 		int node;
 
-		for_each_node_mask(node, *nodemask) {
-			if (!hugetlb_cma[node])
-				continue;
-
-			page = cma_alloc(hugetlb_cma[node], nr_pages,
-					 huge_page_order(h), true);
+		if (hugetlb_cma[nid]) {
+			page = cma_alloc(hugetlb_cma[nid], nr_pages,
+					huge_page_order(h), true);
 			if (page)
 				return page;
 		}
+
+		if (!(gfp_mask & __GFP_THISNODE)) {
+			for_each_node_mask(node, *nodemask) {
+				if (node == nid || !hugetlb_cma[node])
+					continue;
+
+				page = cma_alloc(hugetlb_cma[node], nr_pages,
+						huge_page_order(h), true);
+				if (page)
+					return page;
+			}
+		}
 	}
 #endif