| Message ID | 20200901144924.678195-1-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] mm/hugetlb: try preferred node first when alloc gigantic page from cma | expand |
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
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.
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 >
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 >
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
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(-)