Inverted NUMA test in setup_pcpu_remap()?

Inverted NUMA test in setup_pcpu_remap()?

[David Miller]

The test currently is:

	if (!cpu_has_pse || pcpu_need_numa())
		return -EINVAL;

Don't we really mean "!pcpu_need_numa()"?

The way I read the intent, setup_pcpu_remap() should be used in the
NUMA case.  But that's not what's happening because of how this test
is coded.

In fact, the test here is identical to the one used in
setup_pcpu_embed()

Re: Inverted NUMA test in setup_pcpu_remap()?

[Yinghai Lu]

On Tue, Mar 31, 2009 at 10:31 PM, David Miller <davem@davemloft.net> wrote:
>
> The test currently is:
>
>        if (!cpu_has_pse || pcpu_need_numa())
>                return -EINVAL;
>
> Don't we really mean "!pcpu_need_numa()"?
>
> The way I read the intent, setup_pcpu_remap() should be used in the
> NUMA case.  But that's not what's happening because of how this test
> is coded.
>
> In fact, the test here is identical to the one used in
> setup_pcpu_embed()
> --

you are right. on one 4 sockets system got

[    0.000000] PERCPU: Allocated 471 4k pages, static data 1925472 bytes

that is from setup_pcpu_4k...

YH

[PATCH] x86,percpu: fix inverted NUMA test in setup_pcpu_remap()

[Tejun Heo]

setup_percpu_remap() is for NUMA machines yet it bailed out with
-EINVAL if pcpu_need_numa().  Fix the inverted condition.

This problem was reported by David Miller and verified by Yinhai Lu.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: David Miller <davem@davemloft.net>
Reported-by: Yinghai Lu <yinghai@kernel.org>
---
Oops, thanks for spotting it.  I don't have a numa machine so the
condition was never tested on actual machines.  :-)

 arch/x86/kernel/setup_percpu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 400331b..876b127 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -162,7 +162,7 @@ static ssize_t __init setup_pcpu_remap(size_t static_size)
 	 * If large page isn't supported, there's no benefit in doing
 	 * this.  Also, on non-NUMA, embedding is better.
 	 */
-	if (!cpu_has_pse || pcpu_need_numa())
+	if (!cpu_has_pse || !pcpu_need_numa())
 		return -EINVAL;
 
 	last = NULL;

[PATCH] x86: remove duplicated code with pcpu_need_numa()

[Yinghai Lu]

Impact: clean up

those code pcpu_need_numa(), should be removed.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/setup_percpu.c |   14 --------------
 1 file changed, 14 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup_percpu.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup_percpu.c
+++ linux-2.6/arch/x86/kernel/setup_percpu.c
@@ -153,7 +153,6 @@ static struct page * __init pcpur_get_pa
 static ssize_t __init setup_pcpu_remap(size_t static_size)
 {
 	static struct vm_struct vm;
-	pg_data_t *last;
 	size_t ptrs_size, dyn_size;
 	unsigned int cpu;
 	ssize_t ret;
@@ -165,19 +164,6 @@ static ssize_t __init setup_pcpu_remap(s
 	if (!cpu_has_pse || !pcpu_need_numa())
 		return -EINVAL;
 
-	last = NULL;
-	for_each_possible_cpu(cpu) {
-		int node = early_cpu_to_node(cpu);
-
-		if (node_online(node) && NODE_DATA(node) &&
-		    last && last != NODE_DATA(node))
-			goto proceed;
-
-		last = NODE_DATA(node);
-	}
-	return -EINVAL;
-
-proceed:
 	/*
 	 * Currently supports only single page.  Supporting multiple
 	 * pages won't be too difficult if it ever becomes necessary.

Re: [PATCH] x86: remove duplicated code with pcpu_need_numa()

[Tejun Heo]

Yinghai Lu wrote:
> Impact: clean up
> 
> those code pcpu_need_numa(), should be removed.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.  Heh... that was a mess.

-- 
tejun

Re: [PATCH] x86: remove duplicated code with pcpu_need_numa()

[David Miller]

From: Yinghai Lu <yinghai@kernel.org>
Date: Wed, 01 Apr 2009 00:27:44 -0700

> 
> Impact: clean up
> 
> those code pcpu_need_numa(), should be removed.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Acked-by: David S. Miller <davem@davemloft.net>

[tip:x86/urgent] x86,percpu: fix inverted NUMA test in setup_pcpu_remap()

[Tejun Heo]

Commit-ID:  eb12ce60c81826a99eadbc56401e08ceb37a0cc2
Gitweb:     http://git.kernel.org/tip/eb12ce60c81826a99eadbc56401e08ceb37a0cc2
Author:     Tejun Heo <tj@kernel.org>
AuthorDate: Wed, 1 Apr 2009 15:06:33 +0900
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 2 Apr 2009 06:08:05 +0200

x86,percpu: fix inverted NUMA test in setup_pcpu_remap()

setup_percpu_remap() is for NUMA machines yet it bailed out with
-EINVAL if pcpu_need_numa().  Fix the inverted condition.

This problem was reported by David Miller and verified by Yinhai Lu.

Reported-by: David Miller <davem@davemloft.net>
Reported-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Tejun Heo <tj@kernel.org>
LKML-Reference: <49D30469.8020006@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/setup_percpu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 400331b..876b127 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -162,7 +162,7 @@ static ssize_t __init setup_pcpu_remap(size_t static_size)
 	 * If large page isn't supported, there's no benefit in doing
 	 * this.  Also, on non-NUMA, embedding is better.
 	 */
-	if (!cpu_has_pse || pcpu_need_numa())
+	if (!cpu_has_pse || !pcpu_need_numa())
 		return -EINVAL;
 
 	last = NULL;

[tip:x86/urgent] x86: remove duplicated code with pcpu_need_numa()

[Yinghai Lu]

Commit-ID:  3de46fda4c104deef17ec70f85361f5c6b84ce0e
Gitweb:     http://git.kernel.org/tip/3de46fda4c104deef17ec70f85361f5c6b84ce0e
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Wed, 1 Apr 2009 00:27:44 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 2 Apr 2009 06:08:05 +0200

x86: remove duplicated code with pcpu_need_numa()

Impact: clean up

those code pcpu_need_numa(), should be removed.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: David Miller <davem@davemloft.net>
LKML-Reference: <49D31770.9090502@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/setup_percpu.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 876b127..3a97a4c 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -153,7 +153,6 @@ static struct page * __init pcpur_get_page(unsigned int cpu, int pageno)
 static ssize_t __init setup_pcpu_remap(size_t static_size)
 {
 	static struct vm_struct vm;
-	pg_data_t *last;
 	size_t ptrs_size, dyn_size;
 	unsigned int cpu;
 	ssize_t ret;
@@ -165,19 +164,6 @@ static ssize_t __init setup_pcpu_remap(size_t static_size)
 	if (!cpu_has_pse || !pcpu_need_numa())
 		return -EINVAL;
 
-	last = NULL;
-	for_each_possible_cpu(cpu) {
-		int node = early_cpu_to_node(cpu);
-
-		if (node_online(node) && NODE_DATA(node) &&
-		    last && last != NODE_DATA(node))
-			goto proceed;
-
-		last = NODE_DATA(node);
-	}
-	return -EINVAL;
-
-proceed:
 	/*
 	 * Currently supports only single page.  Supporting multiple
 	 * pages won't be too difficult if it ever becomes necessary.