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

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

[Li Xinhai]

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>
---
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);
 			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
 
-- 
2.18.4

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

[Michal Hocko]

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>

> ---
> 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);
>  			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
>  
> -- 
> 2.18.4

-- 
Michal Hocko
SUSE Labs

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

[Mike Kravetz]

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
 

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

[Li Xinhai]

On 2020-09-02 at 02:43 Mike Kravetz wrote:
>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.
> 
It looks good to me, and it makes sure same behavior when allocate from CMA or
non-CMA for gigantic page, and non-gigantic page from buddy.

I will send a formal V3 patch with updated commit message.

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

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

[Roman Gushchin]

On Tue, Sep 01, 2020 at 10:49:24PM +0800, 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>
> ---
> v1->v2:
> With review by Mike and Michal, need to check __GFP_THISNODE to avoid
> allocate from other nodes.

Acked-by: Roman Gushchin <guro@fb.com>

Thank you!

> 
>  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);
>  			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
>  
> -- 
> 2.18.4
>