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@xxxxxx>
>> Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>> Signed-off-by: Li Xinhai <lixinhai.lxh@xxxxxxxxx>
>
> LGTM
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
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
