All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Kefeng Wang <wangkefeng.wang@huawei.com>
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: Mon, 18 Apr 2022 21:40:40 -0700	[thread overview]
Message-ID: <20220418214040.419a2e17254eb33f68f94b59@linux-foundation.org> (raw)
In-Reply-To: <1407c3bb-89c4-ae11-7b09-d42115ab693e@huawei.com>

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?


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


  reply	other threads:[~2022-04-19  4:40 UTC|newest]

Thread overview: 45+ 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
2022-04-13  6:27     ` liupeng (DM)
2022-04-13 22:04       ` Andrew Morton
2022-04-14  1:28         ` liupeng (DM)
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-15  7:01       ` liupeng (DM)
2022-04-16  1:21       ` Kefeng Wang
2022-04-19  4:40         ` Andrew Morton [this message]
2022-04-19  8:54           ` Kefeng Wang
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
2022-04-13  8:16     ` liupeng (DM)
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  2:43   ` [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding Peng Liu
2022-04-29  3:02     ` Peng Liu
2022-04-29  9:29     ` David Hildenbrand
2022-04-29  9:29       ` David Hildenbrand
2022-04-29 11:44     ` Muchun Song
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-15  7:03     ` liupeng (DM)
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=20220418214040.419a2e17254eb33f68f94b59@linux-foundation.org \
    --to=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=wangkefeng.wang@huawei.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.