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