From: Kefeng Wang <wangkefeng.wang@huawei.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Peng Liu <liupeng256@huawei.com>, <mike.kravetz@oracle.com>,
<david@redhat.com>, <yaozhenguo1@gmail.com>,
<baolin.wang@linux.alibaba.com>, <songmuchun@bytedance.com>,
<liuyuntao10@huawei.com>, <linux-mm@kvack.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes
Date: Tue, 19 Apr 2022 16:54:02 +0800 [thread overview]
Message-ID: <477dcc83-4298-b7e4-3a6a-c8fa23b27d01@huawei.com> (raw)
In-Reply-To: <20220418214040.419a2e17254eb33f68f94b59@linux-foundation.org>
On 2022/4/19 12:40, Andrew Morton wrote:
> On Sat, 16 Apr 2022 09:21:45 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>> On 2022/4/15 13:41, Kefeng Wang wrote:
>>> On 2022/4/15 10:09, Davidlohr Bueso wrote:
>>>> On Wed, 13 Apr 2022, Peng Liu wrote:
>>>>
>>>>> Certain systems are designed to have sparse/discontiguous nodes. In
>>>>> this case, nr_online_nodes can not be used to walk through numa node.
>>>>> Also, a valid node may be greater than nr_online_nodes.
>>>>>
>>>>> However, in hugetlb, it is assumed that nodes are contiguous. Recheck
>>>>> all the places that use nr_online_nodes, and repair them one by one.
>>>>>
>>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>>> Fixes: 4178158ef8ca ("hugetlbfs: fix issue of preallocation of
>>>>> gigantic pages can't work")
>>>>> Fixes: b5389086ad7b ("hugetlbfs: extend the definition of hugepages
>>>>> parameter to support node allocation")
>>>>> Fixes: e79ce9832316 ("hugetlbfs: fix a truncation issue in hugepages
>>>>> parameter")
>>>>> Fixes: f9317f77a6e0 ("hugetlb: clean up potential spectre issue
>>>>> warnings")
>>>>> Signed-off-by: Peng Liu <liupeng256@huawei.com>
>>>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>>>>
>>>> ... but
>>>>
>>>>> ---
>>>>> mm/hugetlb.c | 12 ++++++------
>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>> index b34f50156f7e..5b5a2a5a742f 100644
>>>>> --- a/mm/hugetlb.c
>>>>> +++ b/mm/hugetlb.c
>>>>> @@ -2979,7 +2979,7 @@ int __alloc_bootmem_huge_page(struct hstate
>>>>> *h, int nid)
>>>>> struct huge_bootmem_page *m = NULL; /* initialize for clang */
>>>>> int nr_nodes, node;
>>>>>
>>>>> - if (nid != NUMA_NO_NODE && nid >= nr_online_nodes)
>>>>> + if (nid != NUMA_NO_NODE && !node_online(nid))
>>>> afaict null_blk could also use this, actually the whole thing wants a
>>>> helper - node_valid()?
>>>>
>>> This one should be unnecessary, and this patch looks has a bug,
>>>
>>> if a very nid passed to node_online(), it may crash, could you
>>> re-check it,
>>>
>>> see my changes below,
>>>
>>> 1) add tmp check against MAX_NUMNODES before node_online() check,
>>>
>>> and move it after get tmp in hugepages_setup() , this could cover
>>> both per-node alloc and normal alloc
>> sorry,for normal alloc, tmp is the number of huge pages, we don't need
>> the movement, only add tmp >= MAX_NUMNODES is ok
>>
> Does the v4 patch address the issues which were raised in this thread?
Yes, v4 has fix this.
>
>
> --- a/mm/hugetlb.c~hugetlb-fix-wrong-use-of-nr_online_nodes-v4
> +++ a/mm/hugetlb.c
> @@ -2986,8 +2986,6 @@ int __alloc_bootmem_huge_page(struct hst
> struct huge_bootmem_page *m = NULL; /* initialize for clang */
> int nr_nodes, node;
>
> - if (nid != NUMA_NO_NODE && !node_online(nid))
> - return 0;
> /* do node specific alloc */
> if (nid != NUMA_NO_NODE) {
> m = memblock_alloc_try_nid_raw(huge_page_size(h), huge_page_size(h),
> @@ -4174,7 +4172,7 @@ static int __init hugepages_setup(char *
> pr_warn("HugeTLB: architecture can't support node specific alloc, ignoring!\n");
> return 0;
> }
> - if (!node_online(tmp))
> + if (tmp >= MAX_NUMNODES || !node_online(tmp))
> goto invalid;
> node = array_index_nospec(tmp, MAX_NUMNODES);
> p += count + 1;
> _
>
> .
next prev parent reply other threads:[~2022-04-19 8:54 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-13 3:29 [PATCH v3 0/4] hugetlb: Fix some incorrect behavior Peng Liu
2022-04-13 3:29 ` [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes Peng Liu
2022-04-13 4:42 ` Andrew Morton
[not found] ` <692ee24c-a705-0c54-7cad-a9ecf49a8f15@huawei.com>
2022-04-13 22:04 ` Andrew Morton
2022-04-13 6:29 ` Baolin Wang
2022-04-14 23:36 ` Mike Kravetz
2022-04-15 2:09 ` Davidlohr Bueso
2022-04-15 5:41 ` Kefeng Wang
2022-04-16 1:21 ` Kefeng Wang
2022-04-19 4:40 ` Andrew Morton
2022-04-19 8:54 ` Kefeng Wang [this message]
2022-04-16 10:35 ` [PATCH v4] " Peng Liu
2022-04-18 5:53 ` Kefeng Wang
2022-04-19 4:03 ` Andrew Morton
2022-04-19 14:07 ` Kefeng Wang
2022-04-20 6:17 ` liupeng (DM)
2022-04-29 9:32 ` David Hildenbrand
2022-04-13 3:29 ` [PATCH v3 2/4] hugetlb: Fix hugepages_setup when deal with pernode Peng Liu
2022-04-14 23:50 ` Mike Kravetz
2022-04-29 9:30 ` David Hildenbrand
2022-04-13 3:29 ` [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers Peng Liu
2022-04-13 6:39 ` Baolin Wang
2022-04-13 7:55 ` Muchun Song
[not found] ` <5bbf45e7-1d92-f543-5cfc-bc0141999d46@huawei.com>
2022-04-13 8:21 ` Muchun Song
2022-04-13 8:45 ` Kefeng Wang
2022-04-13 9:01 ` Muchun Song
2022-04-15 0:01 ` Mike Kravetz
2022-04-15 2:24 ` Davidlohr Bueso
2022-04-29 3:02 ` [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding Peng Liu
2022-04-29 9:29 ` David Hildenbrand
2022-04-29 11:44 ` Muchun Song
2022-04-13 3:29 ` [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve Peng Liu
2022-04-13 5:50 ` Muchun Song
2022-04-13 6:41 ` Baolin Wang
2022-04-15 0:03 ` Mike Kravetz
2022-04-15 2:15 ` Davidlohr Bueso
2022-04-29 9:28 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=477dcc83-4298-b7e4-3a6a-c8fa23b27d01@huawei.com \
--to=wangkefeng.wang@huawei.com \
--cc=akpm@linux-foundation.org \
--cc=baolin.wang@linux.alibaba.com \
--cc=david@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=liupeng256@huawei.com \
--cc=liuyuntao10@huawei.com \
--cc=mike.kravetz@oracle.com \
--cc=songmuchun@bytedance.com \
--cc=yaozhenguo1@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).