[PATCH v3 0/4] hugetlb: Fix some incorrect behavior

[PATCH v3 0/4] hugetlb: Fix some incorrect behavior

[Peng Liu]

This series fix three bugs of hugetlb:
1) Invalid use of nr_online_nodes;
2) Inconsistency between 1G hugepage and 2M hugepage;
3) Useless information in dmesg.

v1->v2:
  Split "return 1" into a single patch as suggested by Mike.
v2->v3:
  Fix invalid use of nr_online_nodes as suggested by David.

Peng Liu (4):
  hugetlb: Fix wrong use of nr_online_nodes
  hugetlb: Fix hugepages_setup when deal with pernode
  hugetlb: Fix return value of __setup handlers
  hugetlb: Clean up hugetlb_cma_reserve

 mm/hugetlb.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

-- 
2.18.0.huawei.25

[PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Peng Liu]

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>
---
 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))
 		return 0;
 	/* do node specific alloc */
 	if (nid != NUMA_NO_NODE) {
@@ -3088,7 +3088,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
 	}
 
 	/* do node specific alloc */
-	for (i = 0; i < nr_online_nodes; i++) {
+	for_each_online_node(i) {
 		if (h->max_huge_pages_node[i] > 0) {
 			hugetlb_hstate_alloc_pages_onenode(h, i);
 			node_specific_alloc = true;
@@ -4049,7 +4049,7 @@ static int __init hugetlb_init(void)
 			default_hstate.max_huge_pages =
 				default_hstate_max_huge_pages;
 
-			for (i = 0; i < nr_online_nodes; i++)
+			for_each_online_node(i)
 				default_hstate.max_huge_pages_node[i] =
 					default_hugepages_in_node[i];
 		}
@@ -4164,9 +4164,9 @@ static int __init hugepages_setup(char *s)
 				pr_warn("HugeTLB: architecture can't support node specific alloc, ignoring!\n");
 				return 0;
 			}
-			if (tmp >= nr_online_nodes)
+			if (!node_online(tmp))
 				goto invalid;
-			node = array_index_nospec(tmp, nr_online_nodes);
+			node = array_index_nospec(tmp, MAX_NUMNODES);
 			p += count + 1;
 			/* Parse hugepages */
 			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
@@ -4294,7 +4294,7 @@ static int __init default_hugepagesz_setup(char *s)
 	 */
 	if (default_hstate_max_huge_pages) {
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
-		for (i = 0; i < nr_online_nodes; i++)
+		for_each_online_node(i)
 			default_hstate.max_huge_pages_node[i] =
 				default_hugepages_in_node[i];
 		if (hstate_is_gigantic(&default_hstate))
-- 
2.18.0.huawei.25

[PATCH v3 2/4] hugetlb: Fix hugepages_setup when deal with pernode

[Peng Liu]

Hugepages can be specified to pernode since "hugetlbfs: extend
the definition of hugepages parameter to support node allocation",
but the following problem is observed.

Confusing behavior is observed when both 1G and 2M hugepage is set
after "numa=off".
 cmdline hugepage settings:
  hugepagesz=1G hugepages=0:3,1:3
  hugepagesz=2M hugepages=0:1024,1:1024
 results:
  HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
  HugeTLB registered 2.00 MiB page size, pre-allocated 1024 pages

Furthermore, confusing behavior can be also observed when an
invalid node behind a valid node. To fix this, never allocate any
typical hugepage when an invalid parameter is received.

Fixes: b5389086ad7b ("hugetlbfs: extend the definition of hugepages parameter to support node allocation")
Signed-off-by: Peng Liu <liupeng256@huawei.com>
---
 mm/hugetlb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5b5a2a5a742f..1930b6341f7e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4116,6 +4116,20 @@ bool __init __weak hugetlb_node_alloc_supported(void)
 {
 	return true;
 }
+
+static void __init hugepages_clear_pages_in_node(void)
+{
+	if (!hugetlb_max_hstate) {
+		default_hstate_max_huge_pages = 0;
+		memset(default_hugepages_in_node, 0,
+			MAX_NUMNODES * sizeof(unsigned int));
+	} else {
+		parsed_hstate->max_huge_pages = 0;
+		memset(parsed_hstate->max_huge_pages_node, 0,
+			MAX_NUMNODES * sizeof(unsigned int));
+	}
+}
+
 /*
  * hugepages command line processing
  * hugepages normally follows a valid hugepagsz or default_hugepagsz
@@ -4203,6 +4217,7 @@ static int __init hugepages_setup(char *s)
 
 invalid:
 	pr_warn("HugeTLB: Invalid hugepages parameter %s\n", p);
+	hugepages_clear_pages_in_node();
 	return 0;
 }
 __setup("hugepages=", hugepages_setup);
-- 
2.18.0.huawei.25

[PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Peng Liu]

When __setup() return '0', using invalid option values causes the
entire kernel boot option string to be reported as Unknown. Hugetlb
calls __setup() and will return '0' when set invalid parameter
string.

The following phenomenon is observed:
 cmdline:
  hugepagesz=1Y hugepages=1
 dmesg:
  HugeTLB: unsupported hugepagesz=1Y
  HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
  Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"

Since hugetlb will print warning/error information before return for
invalid parameter string, just use return '1' to avoid print again.

Signed-off-by: Peng Liu <liupeng256@huawei.com>
---
 mm/hugetlb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1930b6341f7e..2e4d8d9fb7c6 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4149,7 +4149,7 @@ static int __init hugepages_setup(char *s)
 	if (!parsed_valid_hugepagesz) {
 		pr_warn("HugeTLB: hugepages=%s does not follow a valid hugepagesz, ignoring\n", s);
 		parsed_valid_hugepagesz = true;
-		return 0;
+		return 1;
 	}
 
 	/*
@@ -4165,7 +4165,7 @@ static int __init hugepages_setup(char *s)
 
 	if (mhp == last_mhp) {
 		pr_warn("HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=%s\n", s);
-		return 0;
+		return 1;
 	}
 
 	while (*p) {
@@ -4176,7 +4176,7 @@ static int __init hugepages_setup(char *s)
 		if (p[count] == ':') {
 			if (!hugetlb_node_alloc_supported()) {
 				pr_warn("HugeTLB: architecture can't support node specific alloc, ignoring!\n");
-				return 0;
+				return 1;
 			}
 			if (!node_online(tmp))
 				goto invalid;
@@ -4218,7 +4218,7 @@ static int __init hugepages_setup(char *s)
 invalid:
 	pr_warn("HugeTLB: Invalid hugepages parameter %s\n", p);
 	hugepages_clear_pages_in_node();
-	return 0;
+	return 1;
 }
 __setup("hugepages=", hugepages_setup);
 
@@ -4239,7 +4239,7 @@ static int __init hugepagesz_setup(char *s)
 
 	if (!arch_hugetlb_valid_size(size)) {
 		pr_err("HugeTLB: unsupported hugepagesz=%s\n", s);
-		return 0;
+		return 1;
 	}
 
 	h = size_to_hstate(size);
@@ -4254,7 +4254,7 @@ static int __init hugepagesz_setup(char *s)
 		if (!parsed_default_hugepagesz ||  h != &default_hstate ||
 		    default_hstate.max_huge_pages) {
 			pr_warn("HugeTLB: hugepagesz=%s specified twice, ignoring\n", s);
-			return 0;
+			return 1;
 		}
 
 		/*
@@ -4285,14 +4285,14 @@ static int __init default_hugepagesz_setup(char *s)
 	parsed_valid_hugepagesz = false;
 	if (parsed_default_hugepagesz) {
 		pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", s);
-		return 0;
+		return 1;
 	}
 
 	size = (unsigned long)memparse(s, NULL);
 
 	if (!arch_hugetlb_valid_size(size)) {
 		pr_err("HugeTLB: unsupported default_hugepagesz=%s\n", s);
-		return 0;
+		return 1;
 	}
 
 	hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
-- 
2.18.0.huawei.25

[PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[Peng Liu]

Use more generic functions to deal with issues related to online
nodes. The changes will make the code simplified.

Signed-off-by: Peng Liu <liupeng256@huawei.com>
---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 2e4d8d9fb7c6..4c529774cc08 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6953,7 +6953,7 @@ void __init hugetlb_cma_reserve(int order)
 		if (hugetlb_cma_size_in_node[nid] == 0)
 			continue;
 
-		if (!node_state(nid, N_ONLINE)) {
+		if (!node_online(nid)) {
 			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
 			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
 			hugetlb_cma_size_in_node[nid] = 0;
@@ -6992,7 +6992,7 @@ void __init hugetlb_cma_reserve(int order)
 	}
 
 	reserved = 0;
-	for_each_node_state(nid, N_ONLINE) {
+	for_each_online_node(nid) {
 		int res;
 		char name[CMA_MAX_NAME];
 
-- 
2.18.0.huawei.25

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Andrew Morton]

On Wed, 13 Apr 2022 03:29:12 +0000 Peng Liu <liupeng256@huawei.com> 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.
> 

What are the runtime effects of this shortcoming?

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[Muchun Song]

On Wed, Apr 13, 2022 at 03:29:15AM +0000, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>

LGTM.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[liupeng (DM)]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]


On 2022/4/13 12:42, Andrew Morton wrote:
> On Wed, 13 Apr 2022 03:29:12 +0000 Peng Liu<liupeng256@huawei.com>  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.
>>
> What are the runtime effects of this shortcoming?
> .

For sparse/discontiguous nodes, the current code may treat a valid node
as invalid, and will fail to allocate all hugepages on a valid node that
"nid >= nr_online_nodes".

As David suggested:
if (tmp >= nr_online_nodes)
	goto invalid;

Just imagine node 0 and node 2 are online, and node 1 is offline. Assuming
that "node < 2" is valid is wrong.

[-- Attachment #2: Type: text/html, Size: 1476 bytes --]

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Baolin Wang]

On 4/13/2022 11:29 AM, 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>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Baolin Wang]

On 4/13/2022 11:29 AM, Peng Liu wrote:
> When __setup() return '0', using invalid option values causes the
> entire kernel boot option string to be reported as Unknown. Hugetlb
> calls __setup() and will return '0' when set invalid parameter
> string.
> 
> The following phenomenon is observed:
>   cmdline:
>    hugepagesz=1Y hugepages=1
>   dmesg:
>    HugeTLB: unsupported hugepagesz=1Y
>    HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>    Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
> 
> Since hugetlb will print warning/error information before return for
> invalid parameter string, just use return '1' to avoid print again.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[Baolin Wang]

On 4/13/2022 11:29 AM, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>

Looks more consistent. Thanks.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/hugetlb.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 2e4d8d9fb7c6..4c529774cc08 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6953,7 +6953,7 @@ void __init hugetlb_cma_reserve(int order)
>   		if (hugetlb_cma_size_in_node[nid] == 0)
>   			continue;
>   
> -		if (!node_state(nid, N_ONLINE)) {
> +		if (!node_online(nid)) {
>   			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
>   			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
>   			hugetlb_cma_size_in_node[nid] = 0;
> @@ -6992,7 +6992,7 @@ void __init hugetlb_cma_reserve(int order)
>   	}
>   
>   	reserved = 0;
> -	for_each_node_state(nid, N_ONLINE) {
> +	for_each_online_node(nid) {
>   		int res;
>   		char name[CMA_MAX_NAME];
>   

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Muchun Song]

On Wed, Apr 13, 2022 at 03:29:14AM +0000, Peng Liu wrote:
> When __setup() return '0', using invalid option values causes the
> entire kernel boot option string to be reported as Unknown. Hugetlb
> calls __setup() and will return '0' when set invalid parameter
> string.
> 
> The following phenomenon is observed:
>  cmdline:
>   hugepagesz=1Y hugepages=1
>  dmesg:
>   HugeTLB: unsupported hugepagesz=1Y
>   HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>   Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
> 
> Since hugetlb will print warning/error information before return for
> invalid parameter string, just use return '1' to avoid print again.
>

Can't return -EINVAL? It is weird to return 1 on failure.

Thanks.

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[liupeng (DM)]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]


On 2022/4/13 15:55, Muchun Song wrote:
> On Wed, Apr 13, 2022 at 03:29:14AM +0000, Peng Liu wrote:
>> When __setup() return '0', using invalid option values causes the
>> entire kernel boot option string to be reported as Unknown. Hugetlb
>> calls __setup() and will return '0' when set invalid parameter
>> string.
>>
>> The following phenomenon is observed:
>>   cmdline:
>>    hugepagesz=1Y hugepages=1
>>   dmesg:
>>    HugeTLB: unsupported hugepagesz=1Y
>>    HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>>    Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
>>
>> Since hugetlb will print warning/error information before return for
>> invalid parameter string, just use return '1' to avoid print again.
>>
> Can't return -EINVAL? It is weird to return 1 on failure.
>
> Thanks.
>
> .

Not against "return -EINVAL", but consistent with:
1d02b444b8d1 ("tracing: Fix return value of __setup handlers")

[-- Attachment #2: Type: text/html, Size: 1545 bytes --]

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Muchun Song]

On Wed, Apr 13, 2022 at 04:16:11PM +0800, liupeng (DM) wrote:
> 
> On 2022/4/13 15:55, Muchun Song wrote:
> > On Wed, Apr 13, 2022 at 03:29:14AM +0000, Peng Liu wrote:
> > > When __setup() return '0', using invalid option values causes the
> > > entire kernel boot option string to be reported as Unknown. Hugetlb
> > > calls __setup() and will return '0' when set invalid parameter
> > > string.
> > > 
> > > The following phenomenon is observed:
> > >   cmdline:
> > >    hugepagesz=1Y hugepages=1
> > >   dmesg:
> > >    HugeTLB: unsupported hugepagesz=1Y
> > >    HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
> > >    Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
> > > 
> > > Since hugetlb will print warning/error information before return for
> > > invalid parameter string, just use return '1' to avoid print again.
> > > 
> > Can't return -EINVAL? It is weird to return 1 on failure.
> > 
> > Thanks.
> > 
> > .
> 
> Not against "return -EINVAL", but consistent with:
> 1d02b444b8d1 ("tracing: Fix return value of __setup handlers")

I think it is better not return 1.  I don't think it's a good habit we
should follow.

Thanks.

Thanks.

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Kefeng Wang]

On 2022/4/13 16:21, Muchun Song wrote:
> On Wed, Apr 13, 2022 at 04:16:11PM +0800, liupeng (DM) wrote:
>> On 2022/4/13 15:55, Muchun Song wrote:
>>> On Wed, Apr 13, 2022 at 03:29:14AM +0000, Peng Liu wrote:
>>>> When __setup() return '0', using invalid option values causes the
>>>> entire kernel boot option string to be reported as Unknown. Hugetlb
>>>> calls __setup() and will return '0' when set invalid parameter
>>>> string.
>>>>
>>>> The following phenomenon is observed:
>>>>    cmdline:
>>>>     hugepagesz=1Y hugepages=1
>>>>    dmesg:
>>>>     HugeTLB: unsupported hugepagesz=1Y
>>>>     HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>>>>     Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
>>>>
>>>> Since hugetlb will print warning/error information before return for
>>>> invalid parameter string, just use return '1' to avoid print again.
>>>>
>>> Can't return -EINVAL? It is weird to return 1 on failure.
>>>
>>> Thanks.
>>>
>>> .
>> Not against "return -EINVAL", but consistent with:
>> 1d02b444b8d1 ("tracing: Fix return value of __setup handlers")
> I think it is better not return 1.  I don't think it's a good habit we
> should follow.
/*
  * NOTE: __setup functions return values:
  * @fn returns 1 (or non-zero) if the option argument is "handled"
  * and returns 0 if the option argument is "not handled".
  */
#define __setup(str, fn)               \
        __setup_param(str, fn, fn, 0)


1 or -EINVAL should ok, and  most __setup return 1 for know ;)

> Thanks.
>
> Thanks.
>
> .

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Muchun Song]

On Wed, Apr 13, 2022 at 04:45:30PM +0800, Kefeng Wang wrote:
> 
> On 2022/4/13 16:21, Muchun Song wrote:
> > On Wed, Apr 13, 2022 at 04:16:11PM +0800, liupeng (DM) wrote:
> > > On 2022/4/13 15:55, Muchun Song wrote:
> > > > On Wed, Apr 13, 2022 at 03:29:14AM +0000, Peng Liu wrote:
> > > > > When __setup() return '0', using invalid option values causes the
> > > > > entire kernel boot option string to be reported as Unknown. Hugetlb
> > > > > calls __setup() and will return '0' when set invalid parameter
> > > > > string.
> > > > > 
> > > > > The following phenomenon is observed:
> > > > >    cmdline:
> > > > >     hugepagesz=1Y hugepages=1
> > > > >    dmesg:
> > > > >     HugeTLB: unsupported hugepagesz=1Y
> > > > >     HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
> > > > >     Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
> > > > > 
> > > > > Since hugetlb will print warning/error information before return for
> > > > > invalid parameter string, just use return '1' to avoid print again.
> > > > > 
> > > > Can't return -EINVAL? It is weird to return 1 on failure.
> > > > 
> > > > Thanks.
> > > > 
> > > > .
> > > Not against "return -EINVAL", but consistent with:
> > > 1d02b444b8d1 ("tracing: Fix return value of __setup handlers")
> > I think it is better not return 1.  I don't think it's a good habit we
> > should follow.
> /*
>  * NOTE: __setup functions return values:
>  * @fn returns 1 (or non-zero) if the option argument is "handled"
>  * and returns 0 if the option argument is "not handled".
>  */
> #define __setup(str, fn)               \
>        __setup_param(str, fn, fn, 0)
> 
> 
> 1 or -EINVAL should ok, and  most __setup return 1 for know ;)
>

Got it. Thanks.  Seems like a lot of users make mistakes in
this regard [1].

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

[1] https://lore.kernel.org/all/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru/ 

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Andrew Morton]

On Wed, 13 Apr 2022 14:27:54 +0800 "liupeng (DM)" <liupeng256@huawei.com> wrote:

> 
> On 2022/4/13 12:42, Andrew Morton wrote:
> > On Wed, 13 Apr 2022 03:29:12 +0000 Peng Liu<liupeng256@huawei.com>  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.
> >>
> > What are the runtime effects of this shortcoming?
> > .
> 
> For sparse/discontiguous nodes, the current code may treat a valid node
> as invalid, and will fail to allocate all hugepages on a valid node that
> "nid >= nr_online_nodes".
> 
> As David suggested:
> if (tmp >= nr_online_nodes)
> 	goto invalid;
> 
> Just imagine node 0 and node 2 are online, and node 1 is offline. Assuming
> that "node < 2" is valid is wrong.

So do you think we should backport thtis fix into earlier kernel releases?

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[liupeng (DM)]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]


On 2022/4/14 6:04, Andrew Morton wrote:
> On Wed, 13 Apr 2022 14:27:54 +0800 "liupeng (DM)"<liupeng256@huawei.com>  wrote:
>
>> On 2022/4/13 12:42, Andrew Morton wrote:
>>> On Wed, 13 Apr 2022 03:29:12 +0000 Peng Liu<liupeng256@huawei.com>   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.
>>>>
>>> What are the runtime effects of this shortcoming?
>>> .
>> For sparse/discontiguous nodes, the current code may treat a valid node
>> as invalid, and will fail to allocate all hugepages on a valid node that
>> "nid >= nr_online_nodes".
>>
>> As David suggested:
>> if (tmp >= nr_online_nodes)
>> 	goto invalid;
>>
>> Just imagine node 0 and node 2 are online, and node 1 is offline. Assuming
>> that "node < 2" is valid is wrong.
> So do you think we should backport thtis fix into earlier kernel releases?
> .

I think it is not an urgent bug, because:
1) Qemu does not support sparse node so far, although there are some sparse-node
issues to make qemu support sparse node.
2) I don't find an actual normal machine that reports sparse-node and need to
use hugepages so far.

[-- Attachment #2: Type: text/html, Size: 2460 bytes --]

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Mike Kravetz]

On 4/12/22 20:29, 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>
> ---
>  mm/hugetlb.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Thank you!

I am guessing that at one time nodes were contiguous at least at boot time.
When that changed, hugetlb was not updated. :(

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v3 2/4] hugetlb: Fix hugepages_setup when deal with pernode

[Mike Kravetz]

On 4/12/22 20:29, Peng Liu wrote:
> Hugepages can be specified to pernode since "hugetlbfs: extend
> the definition of hugepages parameter to support node allocation",
> but the following problem is observed.
> 
> Confusing behavior is observed when both 1G and 2M hugepage is set
> after "numa=off".
>  cmdline hugepage settings:
>   hugepagesz=1G hugepages=0:3,1:3
>   hugepagesz=2M hugepages=0:1024,1:1024
>  results:
>   HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
>   HugeTLB registered 2.00 MiB page size, pre-allocated 1024 pages
> 
> Furthermore, confusing behavior can be also observed when an
> invalid node behind a valid node. To fix this, never allocate any
> typical hugepage when an invalid parameter is received.
> 
> Fixes: b5389086ad7b ("hugetlbfs: extend the definition of hugepages parameter to support node allocation")
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> ---
>  mm/hugetlb.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Thanks!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Mike Kravetz]

On 4/12/22 20:29, Peng Liu wrote:
> When __setup() return '0', using invalid option values causes the
> entire kernel boot option string to be reported as Unknown. Hugetlb
> calls __setup() and will return '0' when set invalid parameter
> string.
> 
> The following phenomenon is observed:
>  cmdline:
>   hugepagesz=1Y hugepages=1
>  dmesg:
>   HugeTLB: unsupported hugepagesz=1Y
>   HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>   Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
> 
> Since hugetlb will print warning/error information before return for
> invalid parameter string, just use return '1' to avoid print again.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> ---
>  mm/hugetlb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Thank you for cleaning this up!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[Mike Kravetz]

On 4/12/22 20:29, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> ---
>  mm/hugetlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thank you!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Davidlohr Bueso]

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()?

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[Davidlohr Bueso]

On Wed, 13 Apr 2022, Peng Liu wrote:

>Use more generic functions to deal with issues related to online
>nodes. The changes will make the code simplified.
>
>Signed-off-by: Peng Liu <liupeng256@huawei.com>
>Reviewed-by: Muchun Song <songmuchun@bytedance.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>

>---
> mm/hugetlb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>index 2e4d8d9fb7c6..4c529774cc08 100644
>--- a/mm/hugetlb.c
>+++ b/mm/hugetlb.c
>@@ -6953,7 +6953,7 @@ void __init hugetlb_cma_reserve(int order)
>		if (hugetlb_cma_size_in_node[nid] == 0)
>			continue;
>
>-		if (!node_state(nid, N_ONLINE)) {
>+		if (!node_online(nid)) {

You could update mm/page_ext.c as well

>			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
>			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
>			hugetlb_cma_size_in_node[nid] = 0;
>@@ -6992,7 +6992,7 @@ void __init hugetlb_cma_reserve(int order)
>	}
>
>	reserved = 0;
>-	for_each_node_state(nid, N_ONLINE) {
>+	for_each_online_node(nid) {

... and arch/ia64/kernel/uncached.c for this.

Re: [PATCH v3 3/4] hugetlb: Fix return value of __setup handlers

[Davidlohr Bueso]

On Wed, 13 Apr 2022, Peng Liu wrote:

>When __setup() return '0', using invalid option values causes the
>entire kernel boot option string to be reported as Unknown. Hugetlb
>calls __setup() and will return '0' when set invalid parameter
>string.
>
>The following phenomenon is observed:
> cmdline:
>  hugepagesz=1Y hugepages=1
> dmesg:
>  HugeTLB: unsupported hugepagesz=1Y
>  HugeTLB: hugepages=1 does not follow a valid hugepagesz, ignoring
>  Unknown kernel command line parameters "hugepagesz=1Y hugepages=1"
>
>Since hugetlb will print warning/error information before return for
>invalid parameter string, just use return '1' to avoid print again.
>
>Signed-off-by: Peng Liu <liupeng256@huawei.com>
>Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Kefeng Wang]

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

2) due to for_each_online_node() usage, we can drop additional check of 
nid in __alloc_bootmem_huge_page()


$ git diff
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fb5a549169ce..5a3ddec181a0 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2986,8 +2986,6 @@ 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)
-               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),
@@ -3095,7 +3093,7 @@ static void __init 
hugetlb_hstate_alloc_pages(struct hstate *h)
         }

         /* do node specific alloc */
-       for (i = 0; i < nr_online_nodes; i++) {
+       for_each_online_node(i) {
                 if (h->max_huge_pages_node[i] > 0) {
                         hugetlb_hstate_alloc_pages_onenode(h, i);
                         node_specific_alloc = true;
@@ -4059,7 +4057,7 @@ static int __init hugetlb_init(void)
                         default_hstate.max_huge_pages =
                                 default_hstate_max_huge_pages;

-                       for (i = 0; i < nr_online_nodes; i++)
+                       for_each_online_node(i)
default_hstate.max_huge_pages_node[i] =
default_hugepages_in_node[i];
                 }
@@ -4168,15 +4166,15 @@ static int __init hugepages_setup(char *s)
                 count = 0;
                 if (sscanf(p, "%lu%n", &tmp, &count) != 1)
                         goto invalid;
+               if (tmp > MAX_NUMNODES || !node_online(tmp))
+                       goto invalid;
                 /* Parameter is node format */
                 if (p[count] == ':') {
                         if (!hugetlb_node_alloc_supported()) {
                                 pr_warn("HugeTLB: architecture can't 
support node specific alloc, ignoring!\n");
                                 return 0;
                         }
-                       if (tmp >= nr_online_nodes)
-                               goto invalid;
-                       node = array_index_nospec(tmp, nr_online_nodes);
+                       node = array_index_nospec(tmp, MAX_NUMNODES);
                         p += count + 1;
                         /* Parse hugepages */
                         if (sscanf(p, "%lu%n", &tmp, &count) != 1)
@@ -4304,7 +4302,7 @@ static int __init default_hugepagesz_setup(char *s)
          */
         if (default_hstate_max_huge_pages) {
                 default_hstate.max_huge_pages = 
default_hstate_max_huge_pages;
-               for (i = 0; i < nr_online_nodes; i++)
+               for_each_online_node(i)
                         default_hstate.max_huge_pages_node[i] =
                                 default_hugepages_in_node[i];
                 if (hstate_is_gigantic(&default_hstate))


> .

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[liupeng (DM)]

[-- Attachment #1: Type: text/plain, Size: 6199 bytes --]


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
>
> 2) due to for_each_online_node() usage, we can drop additional check 
> of nid in __alloc_bootmem_huge_page()
>
>
> $ git diff
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index fb5a549169ce..5a3ddec181a0 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2986,8 +2986,6 @@ 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)
> -               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),
> @@ -3095,7 +3093,7 @@ static void __init 
> hugetlb_hstate_alloc_pages(struct hstate *h)
>         }
>
>         /* do node specific alloc */
> -       for (i = 0; i < nr_online_nodes; i++) {
> +       for_each_online_node(i) {
>                 if (h->max_huge_pages_node[i] > 0) {
>                         hugetlb_hstate_alloc_pages_onenode(h, i);
>                         node_specific_alloc = true;
> @@ -4059,7 +4057,7 @@ static int __init hugetlb_init(void)
>                         default_hstate.max_huge_pages =
>                                 default_hstate_max_huge_pages;
>
> -                       for (i = 0; i < nr_online_nodes; i++)
> +                       for_each_online_node(i)
> default_hstate.max_huge_pages_node[i] =
> default_hugepages_in_node[i];
>                 }
> @@ -4168,15 +4166,15 @@ static int __init hugepages_setup(char *s)
>                 count = 0;
>                 if (sscanf(p, "%lu%n", &tmp, &count) != 1)
>                         goto invalid;
> +               if (tmp > MAX_NUMNODES || !node_online(tmp))
> +                       goto invalid;
>                 /* Parameter is node format */
>                 if (p[count] == ':') {
>                         if (!hugetlb_node_alloc_supported()) {
>                                 pr_warn("HugeTLB: architecture can't 
> support node specific alloc, ignoring!\n");
>                                 return 0;
>                         }
> -                       if (tmp >= nr_online_nodes)
> -                               goto invalid;
> -                       node = array_index_nospec(tmp, nr_online_nodes);
> +                       node = array_index_nospec(tmp, MAX_NUMNODES);
>                         p += count + 1;
>                         /* Parse hugepages */
>                         if (sscanf(p, "%lu%n", &tmp, &count) != 1)
> @@ -4304,7 +4302,7 @@ static int __init default_hugepagesz_setup(char *s)
>          */
>         if (default_hstate_max_huge_pages) {
>                 default_hstate.max_huge_pages = 
> default_hstate_max_huge_pages;
> -               for (i = 0; i < nr_online_nodes; i++)
> +               for_each_online_node(i)
>                         default_hstate.max_huge_pages_node[i] =
>                                 default_hugepages_in_node[i];
>                 if (hstate_is_gigantic(&default_hstate))
>
>
Yes, node_online is not a safe function which will cause panic if a very
big number nid is received. So, this patch needs to be modified.
Thanks.

[-- Attachment #2: Type: text/html, Size: 9066 bytes --]

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[liupeng (DM)]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]


On 2022/4/15 10:15, Davidlohr Bueso wrote:
> On Wed, 13 Apr 2022, Peng Liu wrote:
>
>> Use more generic functions to deal with issues related to online
>> nodes. The changes will make the code simplified.
>>
>> Signed-off-by: Peng Liu <liupeng256@huawei.com>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.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>
>
>> ---
>> mm/hugetlb.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 2e4d8d9fb7c6..4c529774cc08 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -6953,7 +6953,7 @@ void __init hugetlb_cma_reserve(int order)
>>         if (hugetlb_cma_size_in_node[nid] == 0)
>>             continue;
>>
>> -        if (!node_state(nid, N_ONLINE)) {
>> +        if (!node_online(nid)) {
>
> You could update mm/page_ext.c as well
>
>>             pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
>>             hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
>>             hugetlb_cma_size_in_node[nid] = 0;
>> @@ -6992,7 +6992,7 @@ void __init hugetlb_cma_reserve(int order)
>>     }
>>
>>     reserved = 0;
>> -    for_each_node_state(nid, N_ONLINE) {
>> +    for_each_online_node(nid) {
>
> ... and arch/ia64/kernel/uncached.c for this.
> .

Thank you very much. I catch this.


[-- Attachment #2: Type: text/html, Size: 3038 bytes --]

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Kefeng Wang]

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

>
> 2) due to for_each_online_node() usage, we can drop additional check 
> of nid in __alloc_bootmem_huge_page()
>

[PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[Peng Liu]

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>
---
v3->v4:
 Make sure nid is valid before use node_online, and __alloc_bootmem_huge_page
 is no need to check node_online, which is suggested by Kefeng.

 mm/hugetlb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b34f50156f7e..a386c5f94932 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2979,8 +2979,6 @@ 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)
-		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),
@@ -3088,7 +3086,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
 	}
 
 	/* do node specific alloc */
-	for (i = 0; i < nr_online_nodes; i++) {
+	for_each_online_node(i) {
 		if (h->max_huge_pages_node[i] > 0) {
 			hugetlb_hstate_alloc_pages_onenode(h, i);
 			node_specific_alloc = true;
@@ -4049,7 +4047,7 @@ static int __init hugetlb_init(void)
 			default_hstate.max_huge_pages =
 				default_hstate_max_huge_pages;
 
-			for (i = 0; i < nr_online_nodes; i++)
+			for_each_online_node(i)
 				default_hstate.max_huge_pages_node[i] =
 					default_hugepages_in_node[i];
 		}
@@ -4164,9 +4162,9 @@ static int __init hugepages_setup(char *s)
 				pr_warn("HugeTLB: architecture can't support node specific alloc, ignoring!\n");
 				return 0;
 			}
-			if (tmp >= nr_online_nodes)
+			if (tmp >= MAX_NUMNODES || !node_online(tmp))
 				goto invalid;
-			node = array_index_nospec(tmp, nr_online_nodes);
+			node = array_index_nospec(tmp, MAX_NUMNODES);
 			p += count + 1;
 			/* Parse hugepages */
 			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
@@ -4294,7 +4292,7 @@ static int __init default_hugepagesz_setup(char *s)
 	 */
 	if (default_hstate_max_huge_pages) {
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
-		for (i = 0; i < nr_online_nodes; i++)
+		for_each_online_node(i)
 			default_hstate.max_huge_pages_node[i] =
 				default_hugepages_in_node[i];
 		if (hstate_is_gigantic(&default_hstate))
-- 
2.25.1

Re: [PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[Kefeng Wang]

On 2022/4/16 18:35, 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>
> ---
> v3->v4:
>   Make sure nid is valid before use node_online, and __alloc_bootmem_huge_page
>   is no need to check node_online, which is suggested by Kefeng.
  Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

>   mm/hugetlb.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index b34f50156f7e..a386c5f94932 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2979,8 +2979,6 @@ 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)
> -		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),
> @@ -3088,7 +3086,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h)
>   	}
>   
>   	/* do node specific alloc */
> -	for (i = 0; i < nr_online_nodes; i++) {
> +	for_each_online_node(i) {
>   		if (h->max_huge_pages_node[i] > 0) {
>   			hugetlb_hstate_alloc_pages_onenode(h, i);
>   			node_specific_alloc = true;
> @@ -4049,7 +4047,7 @@ static int __init hugetlb_init(void)
>   			default_hstate.max_huge_pages =
>   				default_hstate_max_huge_pages;
>   
> -			for (i = 0; i < nr_online_nodes; i++)
> +			for_each_online_node(i)
>   				default_hstate.max_huge_pages_node[i] =
>   					default_hugepages_in_node[i];
>   		}
> @@ -4164,9 +4162,9 @@ static int __init hugepages_setup(char *s)
>   				pr_warn("HugeTLB: architecture can't support node specific alloc, ignoring!\n");
>   				return 0;
>   			}
> -			if (tmp >= nr_online_nodes)
> +			if (tmp >= MAX_NUMNODES || !node_online(tmp))
>   				goto invalid;
> -			node = array_index_nospec(tmp, nr_online_nodes);
> +			node = array_index_nospec(tmp, MAX_NUMNODES);
>   			p += count + 1;
>   			/* Parse hugepages */
>   			if (sscanf(p, "%lu%n", &tmp, &count) != 1)
> @@ -4294,7 +4292,7 @@ static int __init default_hugepagesz_setup(char *s)
>   	 */
>   	if (default_hstate_max_huge_pages) {
>   		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
> -		for (i = 0; i < nr_online_nodes; i++)
> +		for_each_online_node(i)
>   			default_hstate.max_huge_pages_node[i] =
>   				default_hugepages_in_node[i];
>   		if (hstate_is_gigantic(&default_hstate))

Re: [PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[Andrew Morton]

On Sat, 16 Apr 2022 10:35:26 +0000 Peng Liu <liupeng256@huawei.com> 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.

oops.

What are the user-visible runtime effects of this flaw?

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Andrew Morton]

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

Re: [PATCH v3 1/4] hugetlb: Fix wrong use of nr_online_nodes

[Kefeng Wang]

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

Re: [PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[Kefeng Wang]

On 2022/4/19 12:03, Andrew Morton wrote:
> On Sat, 16 Apr 2022 10:35:26 +0000 Peng Liu <liupeng256@huawei.com> 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.
> oops.
>
> What are the user-visible runtime effects of this flaw?

For example, there are four node=0,1,2,3, and nid = 1 is offline 
node，nr_online_nodes = 3

1) per-node alloc (hugepages=1:2) fails,

2) per-node alloc (hugepages=3:2) fails, but it could succeed.

I assume that there is no user-visible runtime effects.

> .

Re: [PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[liupeng (DM)]

On 2022/4/19 22:07, Kefeng Wang wrote:
>
> On 2022/4/19 12:03, Andrew Morton wrote:
>> On Sat, 16 Apr 2022 10:35:26 +0000 Peng Liu <liupeng256@huawei.com> 
>> 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.
>> oops.
>>
>> What are the user-visible runtime effects of this flaw?
>
> For example, there are four node=0,1,2,3, and nid = 1 is offline 
> node，nr_online_nodes = 3
>
> 1) per-node alloc (hugepages=1:2) fails,
>
> 2) per-node alloc (hugepages=3:2) fails, but it could succeed.
>
> I assume that there is no user-visible runtime effects.
>
Thanks, you are right.

I have constructed node =0, 1, 3, 4, and requested huge pages as:

hugepagesz=1G hugepages=0:1,4:1 hugepagesz=2M hugepages=0:1024,4:1024

Without this patch:

  HugeTLB: Invalid hugepages parameter 4:1
  HugeTLB: Invalid hugepages parameter 4:1024
  HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
  HugeTLB registered 2.00 MiB page size, pre-allocated 1024 pages

With this patch:

   HugeTLB registered 1.00 GiB page size, pre-allocated 2 pages
   HugeTLB registered 2.00 MiB page size, pre-allocated 2048 pages

>> .
> .

[PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[Peng Liu]

Use more generic functions to deal with issues related to online
nodes. The changes will make the code simplified.

Signed-off-by: Peng Liu <liupeng256@huawei.com>
Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
Suggested-by: Andrew Morton <akpm@linux-foundation.org>
---
v4:
 Clean up all the related issues in one patch as suggested by Andrew.

 arch/ia64/kernel/uncached.c | 2 +-
 mm/hugetlb.c                | 4 ++--
 mm/page_ext.c               | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/ia64/kernel/uncached.c b/arch/ia64/kernel/uncached.c
index 816803636a75..a0fec82c56b8 100644
--- a/arch/ia64/kernel/uncached.c
+++ b/arch/ia64/kernel/uncached.c
@@ -261,7 +261,7 @@ static int __init uncached_init(void)
 {
 	int nid;
 
-	for_each_node_state(nid, N_ONLINE) {
+	for_each_online_node(nid) {
 		uncached_pools[nid].pool = gen_pool_create(PAGE_SHIFT, nid);
 		mutex_init(&uncached_pools[nid].add_chunk_mutex);
 	}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 30e1099fd99a..0e5a7764efaa 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6951,7 +6951,7 @@ void __init hugetlb_cma_reserve(int order)
 		if (hugetlb_cma_size_in_node[nid] = 0)
 			continue;
 
-		if (!node_state(nid, N_ONLINE)) {
+		if (!node_online(nid)) {
 			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
 			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
 			hugetlb_cma_size_in_node[nid] = 0;
@@ -6990,7 +6990,7 @@ void __init hugetlb_cma_reserve(int order)
 	}
 
 	reserved = 0;
-	for_each_node_state(nid, N_ONLINE) {
+	for_each_online_node(nid) {
 		int res;
 		char name[CMA_MAX_NAME];
 
diff --git a/mm/page_ext.c b/mm/page_ext.c
index 2e66d934d63f..3dc715d7ac29 100644
--- a/mm/page_ext.c
+++ b/mm/page_ext.c
@@ -320,7 +320,7 @@ static int __meminit online_page_ext(unsigned long start_pfn,
 		 * online__pages(), and start_pfn should exist.
 		 */
 		nid = pfn_to_nid(start_pfn);
-		VM_BUG_ON(!node_state(nid, N_ONLINE));
+		VM_BUG_ON(!node_online(nid));
 	}
 
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION)
-- 
2.25.1

[PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[Peng Liu]

Use more generic functions to deal with issues related to online
nodes. The changes will make the code simplified.

Signed-off-by: Peng Liu <liupeng256@huawei.com>
Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
Suggested-by: Andrew Morton <akpm@linux-foundation.org>
---
v4:
 Clean up all the related issues in one patch as suggested by Andrew.

 arch/ia64/kernel/uncached.c | 2 +-
 mm/hugetlb.c                | 4 ++--
 mm/page_ext.c               | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/ia64/kernel/uncached.c b/arch/ia64/kernel/uncached.c
index 816803636a75..a0fec82c56b8 100644
--- a/arch/ia64/kernel/uncached.c
+++ b/arch/ia64/kernel/uncached.c
@@ -261,7 +261,7 @@ static int __init uncached_init(void)
 {
 	int nid;
 
-	for_each_node_state(nid, N_ONLINE) {
+	for_each_online_node(nid) {
 		uncached_pools[nid].pool = gen_pool_create(PAGE_SHIFT, nid);
 		mutex_init(&uncached_pools[nid].add_chunk_mutex);
 	}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 30e1099fd99a..0e5a7764efaa 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6951,7 +6951,7 @@ void __init hugetlb_cma_reserve(int order)
 		if (hugetlb_cma_size_in_node[nid] == 0)
 			continue;
 
-		if (!node_state(nid, N_ONLINE)) {
+		if (!node_online(nid)) {
 			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
 			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
 			hugetlb_cma_size_in_node[nid] = 0;
@@ -6990,7 +6990,7 @@ void __init hugetlb_cma_reserve(int order)
 	}
 
 	reserved = 0;
-	for_each_node_state(nid, N_ONLINE) {
+	for_each_online_node(nid) {
 		int res;
 		char name[CMA_MAX_NAME];
 
diff --git a/mm/page_ext.c b/mm/page_ext.c
index 2e66d934d63f..3dc715d7ac29 100644
--- a/mm/page_ext.c
+++ b/mm/page_ext.c
@@ -320,7 +320,7 @@ static int __meminit online_page_ext(unsigned long start_pfn,
 		 * online__pages(), and start_pfn should exist.
 		 */
 		nid = pfn_to_nid(start_pfn);
-		VM_BUG_ON(!node_state(nid, N_ONLINE));
+		VM_BUG_ON(!node_online(nid));
 	}
 
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION)
-- 
2.25.1

Re: [PATCH v3 4/4] hugetlb: Clean up hugetlb_cma_reserve

[David Hildenbrand]

On 13.04.22 05:29, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> ---
>  mm/hugetlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 2e4d8d9fb7c6..4c529774cc08 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6953,7 +6953,7 @@ void __init hugetlb_cma_reserve(int order)
>  		if (hugetlb_cma_size_in_node[nid] == 0)
>  			continue;
>  
> -		if (!node_state(nid, N_ONLINE)) {
> +		if (!node_online(nid)) {
>  			pr_warn("hugetlb_cma: invalid node %d specified\n", nid);
>  			hugetlb_cma_size -= hugetlb_cma_size_in_node[nid];
>  			hugetlb_cma_size_in_node[nid] = 0;
> @@ -6992,7 +6992,7 @@ void __init hugetlb_cma_reserve(int order)
>  	}
>  
>  	reserved = 0;
> -	for_each_node_state(nid, N_ONLINE) {
> +	for_each_online_node(nid) {
>  		int res;
>  		char name[CMA_MAX_NAME];
>  

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[David Hildenbrand]

On 29.04.22 05:02, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[David Hildenbrand]

On 29.04.22 05:02, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/4] hugetlb: Fix hugepages_setup when deal with pernode

[David Hildenbrand]

On 13.04.22 05:29, Peng Liu wrote:
> Hugepages can be specified to pernode since "hugetlbfs: extend
> the definition of hugepages parameter to support node allocation",
> but the following problem is observed.
> 
> Confusing behavior is observed when both 1G and 2M hugepage is set
> after "numa=off".
>  cmdline hugepage settings:
>   hugepagesz=1G hugepages=0:3,1:3
>   hugepagesz=2M hugepages=0:1024,1:1024
>  results:
>   HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
>   HugeTLB registered 2.00 MiB page size, pre-allocated 1024 pages
> 
> Furthermore, confusing behavior can be also observed when an
> invalid node behind a valid node. To fix this, never allocate any
> typical hugepage when an invalid parameter is received.
> 
> Fixes: b5389086ad7b ("hugetlbfs: extend the definition of hugepages parameter to support node allocation")
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> ---
>  mm/hugetlb.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 5b5a2a5a742f..1930b6341f7e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4116,6 +4116,20 @@ bool __init __weak hugetlb_node_alloc_supported(void)
>  {
>  	return true;
>  }
> +
> +static void __init hugepages_clear_pages_in_node(void)

I think the name is a bit imprecise, but I have no better suggestion
right now.

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v4] hugetlb: Fix wrong use of nr_online_nodes

[David Hildenbrand]

On 16.04.22 12:35, 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>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[Muchun Song]

On Fri, Apr 29, 2022 at 03:02:18AM +0000, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH v4] mm: Using for_each_online_node and node_online instead of open coding

[Muchun Song]

On Fri, Apr 29, 2022 at 03:02:18AM +0000, Peng Liu wrote:
> Use more generic functions to deal with issues related to online
> nodes. The changes will make the code simplified.
> 
> Signed-off-by: Peng Liu <liupeng256@huawei.com>
> Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.