[PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Hoan Tran OS]

In NUMA layout which nodes have memory ranges that span across other nodes,
the mm driver can detect the memory node id incorrectly.

For example, with layout below
Node 0 address: 0000 xxxx 0000 xxxx
Node 1 address: xxxx 1111 xxxx 1111

Note:
 - Memory from low to high
 - 0/1: Node id
 - x: Invalid memory of a node

When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
config, mm only checks the memory validity but not the node id.
Because of that, Node 1 also detects the memory from node 0 as below
when it scans from the start address to the end address of node 1.

Node 0 address: 0000 xxxx xxxx xxxx
Node 1 address: xxxx 1111 1111 1111

This layout could occur on any architecture. This patch enables
CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.

V2:
 * Revise the patch description

Hoan Tran (5):
  mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
  powerpc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
  x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
  sparc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
  s390: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

 arch/powerpc/Kconfig | 9 ---------
 arch/s390/Kconfig    | 8 --------
 arch/sparc/Kconfig   | 9 ---------
 arch/x86/Kconfig     | 9 ---------
 mm/page_alloc.c      | 2 +-
 5 files changed, 1 insertion(+), 36 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Hoan Tran OS]

In NUMA layout which nodes have memory ranges that span across other nodes,
the mm driver can detect the memory node id incorrectly.

For example, with layout below
Node 0 address: 0000 xxxx 0000 xxxx
Node 1 address: xxxx 1111 xxxx 1111

Note:
 - Memory from low to high
 - 0/1: Node id
 - x: Invalid memory of a node

When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
config, mm only checks the memory validity but not the node id.
Because of that, Node 1 also detects the memory from node 0 as below
when it scans from the start address to the end address of node 1.

Node 0 address: 0000 xxxx xxxx xxxx
Node 1 address: xxxx 1111 1111 1111

This layout could occur on any architecture. This patch enables
CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.

Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d66bc8a..6335505 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1413,7 +1413,7 @@ int __meminit early_pfn_to_nid(unsigned long pfn)
 }
 #endif
 
-#ifdef CONFIG_NODES_SPAN_OTHER_NODES
+#ifdef CONFIG_NUMA
 /* Only safe to use early in boot when initialisation is single-threaded */
 static inline bool __meminit early_pfn_in_nid(unsigned long pfn, int node)
 {
-- 
2.7.4

[PATCH v2 2/5] powerpc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS]

Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled by
default with NUMA.

Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com>
---
 arch/powerpc/Kconfig | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c1c636..bdde8bc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -629,15 +629,6 @@ config ARCH_MEMORY_PROBE
 	def_bool y
 	depends on MEMORY_HOTPLUG
 
-# Some NUMA nodes have memory ranges that span
-# other nodes.  Even though a pfn is valid and
-# between a node's start and end pfns, it may not
-# reside on that node.  See memmap_init_zone()
-# for details.
-config NODES_SPAN_OTHER_NODES
-	def_bool y
-	depends on NEED_MULTIPLE_NODES
-
 config STDBINUTILS
 	bool "Using standard binutils settings"
 	depends on 44x
-- 
2.7.4

[PATCH v2 3/5] x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS]

Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled
by default with NUMA.

Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com>
---
 arch/x86/Kconfig | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2bbbd4d..fa9318c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1567,15 +1567,6 @@ config X86_64_ACPI_NUMA
 	---help---
 	  Enable ACPI SRAT based node topology detection.
 
-# Some NUMA nodes have memory ranges that span
-# other nodes.  Even though a pfn is valid and
-# between a node's start and end pfns, it may not
-# reside on that node.  See memmap_init_zone()
-# for details.
-config NODES_SPAN_OTHER_NODES
-	def_bool y
-	depends on X86_64_ACPI_NUMA
-
 config NUMA_EMU
 	bool "NUMA emulation"
 	depends on NUMA
-- 
2.7.4

Re: [PATCH v2 3/5] x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Thomas Gleixner]

On Thu, 11 Jul 2019, Hoan Tran OS wrote:

> Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled
> by default with NUMA.

As I told you before this does not mention that the option is now enabled
even for x86(32bit) configurations which did not enable it before and does
not longer depend on X86_64_ACPI_NUMA.

And there is still no rationale why this makes sense.

Thanks,

	tglx

Re: [PATCH v2 3/5] x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS]

Hi Thomas,


On 7/15/19 11:43 AM, Thomas Gleixner wrote:
> On Thu, 11 Jul 2019, Hoan Tran OS wrote:
> 
>> Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled
>> by default with NUMA.
> 
> As I told you before this does not mention that the option is now enabled
> even for x86(32bit) configurations which did not enable it before and does
> not longer depend on X86_64_ACPI_NUMA.

Agreed, let me add it into this patch description.

> 
> And there is still no rationale why this makes sense.
> 

As we know about the memmap_init_zone() function, it is used to 
initialize all pages. During initializing, early_pfn_in_nid() function 
makes sure the page is in the same node id. Otherwise, 
memmap_init_zone() only checks the page validity. It won't work with 
node memory spans across the others.

The option CONFIG_NODES_SPAN_OTHER_NODES is only used to enable 
early_pfn_in_nid() function.

It occurs during boot-time and won't affect the run-time performance.
And I saw the majority NUMA architectures enable this option by default 
with NUMA.

Thanks and Regards
Hoan


> Thanks,
> 
> 	tglx
> 

[PATCH v2 4/5] sparc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS]

Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled
by default with NUMA.

Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com>
---
 arch/sparc/Kconfig | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 26ab6f5..13449ea 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -291,15 +291,6 @@ config NODES_SHIFT
 	  Specify the maximum number of NUMA Nodes available on the target
 	  system.  Increases memory reserved to accommodate various tables.
 
-# Some NUMA nodes have memory ranges that span
-# other nodes.  Even though a pfn is valid and
-# between a node's start and end pfns, it may not
-# reside on that node.  See memmap_init_zone()
-# for details.
-config NODES_SPAN_OTHER_NODES
-	def_bool y
-	depends on NEED_MULTIPLE_NODES
-
 config ARCH_SELECT_MEMORY_MODEL
 	def_bool y if SPARC64
 
-- 
2.7.4

[PATCH v2 5/5] s390: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES

[Hoan Tran OS]

Remove CONFIG_NODES_SPAN_OTHER_NODES as it's enabled
by default with NUMA.

Signed-off-by: Hoan Tran <Hoan@os.amperecomputing.com>
---
 arch/s390/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 109243f..788a8e9 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -438,14 +438,6 @@ config HOTPLUG_CPU
 	  can be controlled through /sys/devices/system/cpu/cpu#.
 	  Say N if you want to disable CPU hotplug.
 
-# Some NUMA nodes have memory ranges that span
-# other nodes.	Even though a pfn is valid and
-# between a node's start and end pfns, it may not
-# reside on that node.	See memmap_init_zone()
-# for details. <- They meant memory holes!
-config NODES_SPAN_OTHER_NODES
-	def_bool NUMA
-
 config NUMA
 	bool "NUMA support"
 	depends on SMP && SCHED_TOPOLOGY
-- 
2.7.4

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Matthew Wilcox]

On Thu, Jul 11, 2019 at 11:25:44PM +0000, Hoan Tran OS wrote:
> In NUMA layout which nodes have memory ranges that span across other nodes,
> the mm driver can detect the memory node id incorrectly.
> 
> For example, with layout below
> Node 0 address: 0000 xxxx 0000 xxxx
> Node 1 address: xxxx 1111 xxxx 1111
> 
> Note:
>  - Memory from low to high
>  - 0/1: Node id
>  - x: Invalid memory of a node
> 
> When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
> config, mm only checks the memory validity but not the node id.
> Because of that, Node 1 also detects the memory from node 0 as below
> when it scans from the start address to the end address of node 1.
> 
> Node 0 address: 0000 xxxx xxxx xxxx
> Node 1 address: xxxx 1111 1111 1111
> 
> This layout could occur on any architecture. This patch enables
> CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.

How do you know it could occur on any architecture?  Surely you should
just enable this for the architecture where you've noticed the problem.

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

On Thu 11-07-19 23:25:44, Hoan Tran OS wrote:
> In NUMA layout which nodes have memory ranges that span across other nodes,
> the mm driver can detect the memory node id incorrectly.
> 
> For example, with layout below
> Node 0 address: 0000 xxxx 0000 xxxx
> Node 1 address: xxxx 1111 xxxx 1111
> 
> Note:
>  - Memory from low to high
>  - 0/1: Node id
>  - x: Invalid memory of a node
> 
> When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
> config, mm only checks the memory validity but not the node id.
> Because of that, Node 1 also detects the memory from node 0 as below
> when it scans from the start address to the end address of node 1.
> 
> Node 0 address: 0000 xxxx xxxx xxxx
> Node 1 address: xxxx 1111 1111 1111
> 
> This layout could occur on any architecture. This patch enables
> CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.

Yes it can occur on any arch but most sane platforms simply do not
overlap physical ranges. So I do not really see any reason to
unconditionally enable the config for everybody. What is an advantage?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Hoan Tran OS]

Hi,

On 7/12/19 2:02 PM, Michal Hocko wrote:
> On Thu 11-07-19 23:25:44, Hoan Tran OS wrote:
>> In NUMA layout which nodes have memory ranges that span across other nodes,
>> the mm driver can detect the memory node id incorrectly.
>>
>> For example, with layout below
>> Node 0 address: 0000 xxxx 0000 xxxx
>> Node 1 address: xxxx 1111 xxxx 1111
>>
>> Note:
>>   - Memory from low to high
>>   - 0/1: Node id
>>   - x: Invalid memory of a node
>>
>> When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
>> config, mm only checks the memory validity but not the node id.
>> Because of that, Node 1 also detects the memory from node 0 as below
>> when it scans from the start address to the end address of node 1.
>>
>> Node 0 address: 0000 xxxx xxxx xxxx
>> Node 1 address: xxxx 1111 1111 1111
>>
>> This layout could occur on any architecture. This patch enables
>> CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.
> 
> Yes it can occur on any arch but most sane platforms simply do not
> overlap physical ranges. So I do not really see any reason to
> unconditionally enable the config for everybody. What is an advantage?
> 

As I observed from arch folder, there are 9 arch support NUMA config.

./arch/ia64/Kconfig:387:config NUMA
./arch/powerpc/Kconfig:582:config NUMA
./arch/sparc/Kconfig:281:config NUMA
./arch/alpha/Kconfig:557:config NUMA
./arch/sh/mm/Kconfig:112:config NUMA
./arch/arm64/Kconfig:841:config NUMA
./arch/x86/Kconfig:1531:config NUMA
./arch/mips/Kconfig:2646:config NUMA
./arch/s390/Kconfig:441:config NUMA

And there are 5 arch enables CONFIG_NODES_SPAN_OTHER_NODES with NUMA

arch/powerpc/Kconfig:637:config NODES_SPAN_OTHER_NODES
arch/sparc/Kconfig:299:config NODES_SPAN_OTHER_NODES
arch/x86/Kconfig:1575:config NODES_SPAN_OTHER_NODES
arch/s390/Kconfig:446:config NODES_SPAN_OTHER_NODES
arch/arm64 (which I intended to enable in the original patch)

It would be good if we can enable it by-default. Otherwise, let arch 
enables it by them-self. Do you have any suggestions?

Thanks
Hoan

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

On Fri 12-07-19 10:56:47, Hoan Tran OS wrote:
[...]
> It would be good if we can enable it by-default. Otherwise, let arch 
> enables it by them-self. Do you have any suggestions?

I can hardly make any suggestions when it is not really clear _why_ you
want to remove this config option in the first place. Please explain
what motivated you to make this change.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Will Deacon]

Hi all,

On Fri, Jul 12, 2019 at 02:12:23PM +0200, Michal Hocko wrote:
> On Fri 12-07-19 10:56:47, Hoan Tran OS wrote:
> [...]
> > It would be good if we can enable it by-default. Otherwise, let arch 
> > enables it by them-self. Do you have any suggestions?
> 
> I can hardly make any suggestions when it is not really clear _why_ you
> want to remove this config option in the first place. Please explain
> what motivated you to make this change.

Sorry, I think this confusion might actually be my fault and Hoan has just
been implementing my vague suggestion here:

https://lore.kernel.org/linux-arm-kernel/20190625101245.s4vxfosoop52gl4e@willie-the-truck/

If the preference of the mm folks is to leave CONFIG_NODES_SPAN_OTHER_NODES
as it is, then we can define it for arm64. I just find it a bit weird that
the majority of NUMA-capable architectures have to add a symbol in the arch
Kconfig file, for what appears to be a performance optimisation applicable
only to ia64, mips and sh.

At the very least we could make the thing selectable.

Will

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

On Fri 12-07-19 15:37:30, Will Deacon wrote:
> Hi all,
> 
> On Fri, Jul 12, 2019 at 02:12:23PM +0200, Michal Hocko wrote:
> > On Fri 12-07-19 10:56:47, Hoan Tran OS wrote:
> > [...]
> > > It would be good if we can enable it by-default. Otherwise, let arch 
> > > enables it by them-self. Do you have any suggestions?
> > 
> > I can hardly make any suggestions when it is not really clear _why_ you
> > want to remove this config option in the first place. Please explain
> > what motivated you to make this change.
> 
> Sorry, I think this confusion might actually be my fault and Hoan has just
> been implementing my vague suggestion here:
> 
> https://lore.kernel.org/linux-arm-kernel/20190625101245.s4vxfosoop52gl4e@willie-the-truck/
> 
> If the preference of the mm folks is to leave CONFIG_NODES_SPAN_OTHER_NODES
> as it is, then we can define it for arm64. I just find it a bit weird that
> the majority of NUMA-capable architectures have to add a symbol in the arch
> Kconfig file, for what appears to be a performance optimisation applicable
> only to ia64, mips and sh.
> 
> At the very least we could make the thing selectable.

Hmm, I thought this was selectable. But I am obviously wrong here.
Looking more closely, it seems that this is indeed only about
__early_pfn_to_nid and as such not something that should add a config
symbol. This should have been called out in the changelog though.

Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
bucket? Do we have any NUMA architecture that doesn't enable it?

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Hoan Tran OS]

Hi,

On 7/12/19 10:00 PM, Michal Hocko wrote:
> On Fri 12-07-19 15:37:30, Will Deacon wrote:
>> Hi all,
>>
>> On Fri, Jul 12, 2019 at 02:12:23PM +0200, Michal Hocko wrote:
>>> On Fri 12-07-19 10:56:47, Hoan Tran OS wrote:
>>> [...]
>>>> It would be good if we can enable it by-default. Otherwise, let arch
>>>> enables it by them-self. Do you have any suggestions?
>>>
>>> I can hardly make any suggestions when it is not really clear _why_ you
>>> want to remove this config option in the first place. Please explain
>>> what motivated you to make this change.
>>
>> Sorry, I think this confusion might actually be my fault and Hoan has just
>> been implementing my vague suggestion here:
>>
>> https://lore.kernel.org/linux-arm-kernel/20190625101245.s4vxfosoop52gl4e@willie-the-truck/
>>
>> If the preference of the mm folks is to leave CONFIG_NODES_SPAN_OTHER_NODES
>> as it is, then we can define it for arm64. I just find it a bit weird that
>> the majority of NUMA-capable architectures have to add a symbol in the arch
>> Kconfig file, for what appears to be a performance optimisation applicable
>> only to ia64, mips and sh.
>>
>> At the very least we could make the thing selectable.
> 
> Hmm, I thought this was selectable. But I am obviously wrong here.
> Looking more closely, it seems that this is indeed only about
> __early_pfn_to_nid and as such not something that should add a config
> symbol. This should have been called out in the changelog though.

Yes, do you have any other comments about my patch?

> 
> Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> bucket? Do we have any NUMA architecture that doesn't enable it?
> 

As I checked with arch Kconfig files, there are 2 architectures, riscv 
and microblaze, do not support NUMA but enable this config.

And 1 architecture, alpha, supports NUMA but does not enable this config.

Thanks and Regards
Hoan

> Thanks!
> 

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

[Sorry for a late reply]

On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> Hi,
> 
> On 7/12/19 10:00 PM, Michal Hocko wrote:
[...]
> > Hmm, I thought this was selectable. But I am obviously wrong here.
> > Looking more closely, it seems that this is indeed only about
> > __early_pfn_to_nid and as such not something that should add a config
> > symbol. This should have been called out in the changelog though.
> 
> Yes, do you have any other comments about my patch?

Not really. Just make sure to explicitly state that
CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
doesn't really deserve it's own config and can be pulled under NUMA.

> > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > bucket? Do we have any NUMA architecture that doesn't enable it?
> > 
> 
> As I checked with arch Kconfig files, there are 2 architectures, riscv 
> and microblaze, do not support NUMA but enable this config.
> 
> And 1 architecture, alpha, supports NUMA but does not enable this config.

Care to have a look and clean this up please?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Hoan Tran OS]

Hi,

On 7/30/19 1:14 AM, Michal Hocko wrote:
> [Sorry for a late reply]
> 
> On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
>> Hi,
>>
>> On 7/12/19 10:00 PM, Michal Hocko wrote:
> [...]
>>> Hmm, I thought this was selectable. But I am obviously wrong here.
>>> Looking more closely, it seems that this is indeed only about
>>> __early_pfn_to_nid and as such not something that should add a config
>>> symbol. This should have been called out in the changelog though.
>>
>> Yes, do you have any other comments about my patch?
> 
> Not really. Just make sure to explicitly state that
> CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> doesn't really deserve it's own config and can be pulled under NUMA.

Yes, I will add this info into the patch description.

> 
>>> Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
>>> bucket? Do we have any NUMA architecture that doesn't enable it?
>>>
>>
>> As I checked with arch Kconfig files, there are 2 architectures, riscv
>> and microblaze, do not support NUMA but enable this config.
>>
>> And 1 architecture, alpha, supports NUMA but does not enable this config.
> 
> Care to have a look and clean this up please?

Sure, I'll take a look.

Thanks
Hoan
> 

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Mike Rapoport]

[ sorry for a late reply too, somehow I missed this thread before ]

On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> [Sorry for a late reply]
> 
> On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > Hi,
> > 
> > On 7/12/19 10:00 PM, Michal Hocko wrote:
> [...]
> > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > Looking more closely, it seems that this is indeed only about
> > > __early_pfn_to_nid and as such not something that should add a config
> > > symbol. This should have been called out in the changelog though.
> > 
> > Yes, do you have any other comments about my patch?
> 
> Not really. Just make sure to explicitly state that
> CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> doesn't really deserve it's own config and can be pulled under NUMA.
> 
> > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > 

HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
sequence so it's not only about a singe function.

> > As I checked with arch Kconfig files, there are 2 architectures, riscv 
> > and microblaze, do not support NUMA but enable this config.

My take would be that riscv will support NUMA some day.
 
> > And 1 architecture, alpha, supports NUMA but does not enable this config.

alpha's NUMA support is BROKEN for more than a decade now, I doubt it'll
ever get fixed.
 
> Care to have a look and clean this up please?
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> [ sorry for a late reply too, somehow I missed this thread before ]
> 
> On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > [Sorry for a late reply]
> > 
> > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > Hi,
> > > 
> > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > [...]
> > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > Looking more closely, it seems that this is indeed only about
> > > > __early_pfn_to_nid and as such not something that should add a config
> > > > symbol. This should have been called out in the changelog though.
> > > 
> > > Yes, do you have any other comments about my patch?
> > 
> > Not really. Just make sure to explicitly state that
> > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > doesn't really deserve it's own config and can be pulled under NUMA.
> > 
> > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > 
> 
> HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> sequence so it's not only about a singe function.

The question is whether we want to have this a config option or enable
it unconditionally for each NUMA system.

> > > As I checked with arch Kconfig files, there are 2 architectures, riscv 
> > > and microblaze, do not support NUMA but enable this config.
> 
> My take would be that riscv will support NUMA some day.
>  
> > > And 1 architecture, alpha, supports NUMA but does not enable this config.
> 
> alpha's NUMA support is BROKEN for more than a decade now, I doubt it'll
> ever get fixed.

I can see Al has marked it BROKEN in 2005. Maybe time to rip it out?
Although it doesn't seem to be a lot of code in arch/alpha at first
glance so maybe not worth an effort.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Mike Rapoport]

On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > [ sorry for a late reply too, somehow I missed this thread before ]
> > 
> > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > [Sorry for a late reply]
> > > 
> > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > Hi,
> > > > 
> > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > [...]
> > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > Looking more closely, it seems that this is indeed only about
> > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > symbol. This should have been called out in the changelog though.
> > > > 
> > > > Yes, do you have any other comments about my patch?
> > > 
> > > Not really. Just make sure to explicitly state that
> > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > 
> > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > 
> > 
> > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > sequence so it's not only about a singe function.
> 
> The question is whether we want to have this a config option or enable
> it unconditionally for each NUMA system.

We can make it 'default NUMA', but we can't drop it completely because
microblaze uses sparse_memory_present_with_active_regions() which is
unavailable when HAVE_MEMBLOCK_NODE_MAP=n.

> > > > As I checked with arch Kconfig files, there are 2 architectures, riscv 
> > > > and microblaze, do not support NUMA but enable this config.
> > 
> > My take would be that riscv will support NUMA some day.
> >  
> > > > And 1 architecture, alpha, supports NUMA but does not enable this config.
> > 
> > alpha's NUMA support is BROKEN for more than a decade now, I doubt it'll
> > ever get fixed.
> 
> I can see Al has marked it BROKEN in 2005. Maybe time to rip it out?
> Although it doesn't seem to be a lot of code in arch/alpha at first
> glance so maybe not worth an effort.
> -- 
> Michal Hocko
> SUSE Labs
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Michal Hocko]

On Wed 31-07-19 14:14:22, Mike Rapoport wrote:
> On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> > On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > > [ sorry for a late reply too, somehow I missed this thread before ]
> > > 
> > > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > > [Sorry for a late reply]
> > > > 
> > > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > > Hi,
> > > > > 
> > > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > > [...]
> > > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > > Looking more closely, it seems that this is indeed only about
> > > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > > symbol. This should have been called out in the changelog though.
> > > > > 
> > > > > Yes, do you have any other comments about my patch?
> > > > 
> > > > Not really. Just make sure to explicitly state that
> > > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > > 
> > > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > > 
> > > 
> > > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > > sequence so it's not only about a singe function.
> > 
> > The question is whether we want to have this a config option or enable
> > it unconditionally for each NUMA system.
> 
> We can make it 'default NUMA', but we can't drop it completely because
> microblaze uses sparse_memory_present_with_active_regions() which is
> unavailable when HAVE_MEMBLOCK_NODE_MAP=n.

I suppose you mean that microblaze is using
sparse_memory_present_with_active_regions even without CONFIG_NUMA,
right? I have to confess I do not understand that code. What is the deal
with setting node id there?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA

[Mike Rapoport]

On Wed, Jul 31, 2019 at 01:40:16PM +0200, Michal Hocko wrote:
> On Wed 31-07-19 14:14:22, Mike Rapoport wrote:
> > On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> > > On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > > > [ sorry for a late reply too, somehow I missed this thread before ]
> > > > 
> > > > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > > > [Sorry for a late reply]
> > > > > 
> > > > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > > > [...]
> > > > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > > > Looking more closely, it seems that this is indeed only about
> > > > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > > > symbol. This should have been called out in the changelog though.
> > > > > > 
> > > > > > Yes, do you have any other comments about my patch?
> > > > > 
> > > > > Not really. Just make sure to explicitly state that
> > > > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > > > 
> > > > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > > > 
> > > > 
> > > > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > > > sequence so it's not only about a singe function.
> > > 
> > > The question is whether we want to have this a config option or enable
> > > it unconditionally for each NUMA system.
> > 
> > We can make it 'default NUMA', but we can't drop it completely because
> > microblaze uses sparse_memory_present_with_active_regions() which is
> > unavailable when HAVE_MEMBLOCK_NODE_MAP=n.
> 
> I suppose you mean that microblaze is using
> sparse_memory_present_with_active_regions even without CONFIG_NUMA,
> right?

Yes.

> I have to confess I do not understand that code. What is the deal
> with setting node id there?

The sparse_memory_present_with_active_regions() iterates over
memblock.memory regions and uses the node id of each region as the
parameter to memory_present(). The assumption here is that sometime before
each region was assigned a proper non-negative node id. 

microblaze uses device tree for memory enumeration and the current FDT code
does memblock_add() that implicitly sets nid in memblock.memory regions to -1.

So in order to have proper node id passed to memory_present() microblaze
has to call memblock_set_node() before it can use
sparse_memory_present_with_active_regions().

> -- 
> Michal Hocko
> SUSE Labs

-- 
Sincerely yours,
Mike.

microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Michal Hocko]

On Wed 31-07-19 15:26:32, Mike Rapoport wrote:
> On Wed, Jul 31, 2019 at 01:40:16PM +0200, Michal Hocko wrote:
> > On Wed 31-07-19 14:14:22, Mike Rapoport wrote:
> > > On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> > > > On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > > > > [ sorry for a late reply too, somehow I missed this thread before ]
> > > > > 
> > > > > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > > > > [Sorry for a late reply]
> > > > > > 
> > > > > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > > > > [...]
> > > > > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > > > > Looking more closely, it seems that this is indeed only about
> > > > > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > > > > symbol. This should have been called out in the changelog though.
> > > > > > > 
> > > > > > > Yes, do you have any other comments about my patch?
> > > > > > 
> > > > > > Not really. Just make sure to explicitly state that
> > > > > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > > > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > > > > 
> > > > > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > > > > 
> > > > > 
> > > > > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > > > > sequence so it's not only about a singe function.
> > > > 
> > > > The question is whether we want to have this a config option or enable
> > > > it unconditionally for each NUMA system.
> > > 
> > > We can make it 'default NUMA', but we can't drop it completely because
> > > microblaze uses sparse_memory_present_with_active_regions() which is
> > > unavailable when HAVE_MEMBLOCK_NODE_MAP=n.
> > 
> > I suppose you mean that microblaze is using
> > sparse_memory_present_with_active_regions even without CONFIG_NUMA,
> > right?
> 
> Yes.
> 
> > I have to confess I do not understand that code. What is the deal
> > with setting node id there?
> 
> The sparse_memory_present_with_active_regions() iterates over
> memblock.memory regions and uses the node id of each region as the
> parameter to memory_present(). The assumption here is that sometime before
> each region was assigned a proper non-negative node id. 
> 
> microblaze uses device tree for memory enumeration and the current FDT code
> does memblock_add() that implicitly sets nid in memblock.memory regions to -1.
> 
> So in order to have proper node id passed to memory_present() microblaze
> has to call memblock_set_node() before it can use
> sparse_memory_present_with_active_regions().

I am sorry, but I still do not follow. Who is consuming that node id
information when NUMA=n. In other words why cannot we simply do

diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
index a015a951c8b7..3a47e8db8d1c 100644
--- a/arch/microblaze/mm/init.c
+++ b/arch/microblaze/mm/init.c
@@ -175,14 +175,9 @@ void __init setup_memory(void)
 
 		start_pfn = memblock_region_memory_base_pfn(reg);
 		end_pfn = memblock_region_memory_end_pfn(reg);
-		memblock_set_node(start_pfn << PAGE_SHIFT,
-				  (end_pfn - start_pfn) << PAGE_SHIFT,
-				  &memblock.memory, 0);
+		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
 	}
 
-	/* XXX need to clip this if using highmem? */
-	sparse_memory_present_with_active_regions(0);
-
 	paging_init();
 }
 
-- 
Michal Hocko
SUSE Labs

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Mike Rapoport]

On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
> On Wed 31-07-19 15:26:32, Mike Rapoport wrote:
> > On Wed, Jul 31, 2019 at 01:40:16PM +0200, Michal Hocko wrote:
> > > On Wed 31-07-19 14:14:22, Mike Rapoport wrote:
> > > > On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> > > > > On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > > > > > [ sorry for a late reply too, somehow I missed this thread before ]
> > > > > > 
> > > > > > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > > > > > [Sorry for a late reply]
> > > > > > > 
> > > > > > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > > > > > [...]
> > > > > > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > > > > > Looking more closely, it seems that this is indeed only about
> > > > > > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > > > > > symbol. This should have been called out in the changelog though.
> > > > > > > > 
> > > > > > > > Yes, do you have any other comments about my patch?
> > > > > > > 
> > > > > > > Not really. Just make sure to explicitly state that
> > > > > > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > > > > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > > > > > 
> > > > > > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > > > > > 
> > > > > > 
> > > > > > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > > > > > sequence so it's not only about a singe function.
> > > > > 
> > > > > The question is whether we want to have this a config option or enable
> > > > > it unconditionally for each NUMA system.
> > > > 
> > > > We can make it 'default NUMA', but we can't drop it completely because
> > > > microblaze uses sparse_memory_present_with_active_regions() which is
> > > > unavailable when HAVE_MEMBLOCK_NODE_MAP=n.
> > > 
> > > I suppose you mean that microblaze is using
> > > sparse_memory_present_with_active_regions even without CONFIG_NUMA,
> > > right?
> > 
> > Yes.
> > 
> > > I have to confess I do not understand that code. What is the deal
> > > with setting node id there?
> > 
> > The sparse_memory_present_with_active_regions() iterates over
> > memblock.memory regions and uses the node id of each region as the
> > parameter to memory_present(). The assumption here is that sometime before
> > each region was assigned a proper non-negative node id. 
> > 
> > microblaze uses device tree for memory enumeration and the current FDT code
> > does memblock_add() that implicitly sets nid in memblock.memory regions to -1.
> > 
> > So in order to have proper node id passed to memory_present() microblaze
> > has to call memblock_set_node() before it can use
> > sparse_memory_present_with_active_regions().
> 
> I am sorry, but I still do not follow. Who is consuming that node id
> information when NUMA=n. In other words why cannot we simply do
 
We can, I think nobody cared to change it.

> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> index a015a951c8b7..3a47e8db8d1c 100644
> --- a/arch/microblaze/mm/init.c
> +++ b/arch/microblaze/mm/init.c
> @@ -175,14 +175,9 @@ void __init setup_memory(void)
>  
>  		start_pfn = memblock_region_memory_base_pfn(reg);
>  		end_pfn = memblock_region_memory_end_pfn(reg);
> -		memblock_set_node(start_pfn << PAGE_SHIFT,
> -				  (end_pfn - start_pfn) << PAGE_SHIFT,
> -				  &memblock.memory, 0);
> +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);

memory_present() expects pfns, the shift is not needed.

>  	}
>  
> -	/* XXX need to clip this if using highmem? */
> -	sparse_memory_present_with_active_regions(0);
> -
>  	paging_init();
>  }
>  
> -- 
> Michal Hocko
> SUSE Labs

-- 
Sincerely yours,
Mike.

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Michal Hocko]

On Wed 31-07-19 17:21:29, Mike Rapoport wrote:
> On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
> > On Wed 31-07-19 15:26:32, Mike Rapoport wrote:
> > > On Wed, Jul 31, 2019 at 01:40:16PM +0200, Michal Hocko wrote:
> > > > On Wed 31-07-19 14:14:22, Mike Rapoport wrote:
> > > > > On Wed, Jul 31, 2019 at 10:03:09AM +0200, Michal Hocko wrote:
> > > > > > On Wed 31-07-19 09:24:21, Mike Rapoport wrote:
> > > > > > > [ sorry for a late reply too, somehow I missed this thread before ]
> > > > > > > 
> > > > > > > On Tue, Jul 30, 2019 at 10:14:15AM +0200, Michal Hocko wrote:
> > > > > > > > [Sorry for a late reply]
> > > > > > > > 
> > > > > > > > On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On 7/12/19 10:00 PM, Michal Hocko wrote:
> > > > > > > > [...]
> > > > > > > > > > Hmm, I thought this was selectable. But I am obviously wrong here.
> > > > > > > > > > Looking more closely, it seems that this is indeed only about
> > > > > > > > > > __early_pfn_to_nid and as such not something that should add a config
> > > > > > > > > > symbol. This should have been called out in the changelog though.
> > > > > > > > > 
> > > > > > > > > Yes, do you have any other comments about my patch?
> > > > > > > > 
> > > > > > > > Not really. Just make sure to explicitly state that
> > > > > > > > CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> > > > > > > > doesn't really deserve it's own config and can be pulled under NUMA.
> > > > > > > > 
> > > > > > > > > > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > > > > > > > > > bucket? Do we have any NUMA architecture that doesn't enable it?
> > > > > > > > > > 
> > > > > > > 
> > > > > > > HAVE_MEMBLOCK_NODE_MAP makes huge difference in node/zone initialization
> > > > > > > sequence so it's not only about a singe function.
> > > > > > 
> > > > > > The question is whether we want to have this a config option or enable
> > > > > > it unconditionally for each NUMA system.
> > > > > 
> > > > > We can make it 'default NUMA', but we can't drop it completely because
> > > > > microblaze uses sparse_memory_present_with_active_regions() which is
> > > > > unavailable when HAVE_MEMBLOCK_NODE_MAP=n.
> > > > 
> > > > I suppose you mean that microblaze is using
> > > > sparse_memory_present_with_active_regions even without CONFIG_NUMA,
> > > > right?
> > > 
> > > Yes.
> > > 
> > > > I have to confess I do not understand that code. What is the deal
> > > > with setting node id there?
> > > 
> > > The sparse_memory_present_with_active_regions() iterates over
> > > memblock.memory regions and uses the node id of each region as the
> > > parameter to memory_present(). The assumption here is that sometime before
> > > each region was assigned a proper non-negative node id. 
> > > 
> > > microblaze uses device tree for memory enumeration and the current FDT code
> > > does memblock_add() that implicitly sets nid in memblock.memory regions to -1.
> > > 
> > > So in order to have proper node id passed to memory_present() microblaze
> > > has to call memblock_set_node() before it can use
> > > sparse_memory_present_with_active_regions().
> > 
> > I am sorry, but I still do not follow. Who is consuming that node id
> > information when NUMA=n. In other words why cannot we simply do
>  
> We can, I think nobody cared to change it.

It would be great if somebody with the actual HW could try it out.
I can throw a patch but I do not even have a cross compiler in my
toolbox.

> 
> > diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> > index a015a951c8b7..3a47e8db8d1c 100644
> > --- a/arch/microblaze/mm/init.c
> > +++ b/arch/microblaze/mm/init.c
> > @@ -175,14 +175,9 @@ void __init setup_memory(void)
> >  
> >  		start_pfn = memblock_region_memory_base_pfn(reg);
> >  		end_pfn = memblock_region_memory_end_pfn(reg);
> > -		memblock_set_node(start_pfn << PAGE_SHIFT,
> > -				  (end_pfn - start_pfn) << PAGE_SHIFT,
> > -				  &memblock.memory, 0);
> > +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
> 
> memory_present() expects pfns, the shift is not needed.

Right.

-- 
Michal Hocko
SUSE Labs

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Mike Rapoport]

On Wed, Jul 31, 2019 at 04:41:14PM +0200, Michal Hocko wrote:
> On Wed 31-07-19 17:21:29, Mike Rapoport wrote:
> > On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
> > > 
> > > I am sorry, but I still do not follow. Who is consuming that node id
> > > information when NUMA=n. In other words why cannot we simply do
> >  
> > We can, I think nobody cared to change it.
> 
> It would be great if somebody with the actual HW could try it out.
> I can throw a patch but I do not even have a cross compiler in my
> toolbox.

Well, it compiles :)
 
> > > diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> > > index a015a951c8b7..3a47e8db8d1c 100644
> > > --- a/arch/microblaze/mm/init.c
> > > +++ b/arch/microblaze/mm/init.c
> > > @@ -175,14 +175,9 @@ void __init setup_memory(void)
> > >  
> > >  		start_pfn = memblock_region_memory_base_pfn(reg);
> > >  		end_pfn = memblock_region_memory_end_pfn(reg);
> > > -		memblock_set_node(start_pfn << PAGE_SHIFT,
> > > -				  (end_pfn - start_pfn) << PAGE_SHIFT,
> > > -				  &memblock.memory, 0);
> > > +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
> > 
> > memory_present() expects pfns, the shift is not needed.
> 
> Right.
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

-- 
Sincerely yours,
Mike.

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Randy Dunlap]

On 7/31/19 10:15 AM, Mike Rapoport wrote:
> On Wed, Jul 31, 2019 at 04:41:14PM +0200, Michal Hocko wrote:
>> On Wed 31-07-19 17:21:29, Mike Rapoport wrote:
>>> On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
>>>>
>>>> I am sorry, but I still do not follow. Who is consuming that node id
>>>> information when NUMA=n. In other words why cannot we simply do
>>>  
>>> We can, I think nobody cared to change it.
>>
>> It would be great if somebody with the actual HW could try it out.
>> I can throw a patch but I do not even have a cross compiler in my
>> toolbox.
> 
> Well, it compiles :)

Adding Michal Simek <monstr@monstr.eu>.

It's not clear that the MICROBLAZE maintainer is still supporting MICROBLAZE.

>>>> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
>>>> index a015a951c8b7..3a47e8db8d1c 100644
>>>> --- a/arch/microblaze/mm/init.c
>>>> +++ b/arch/microblaze/mm/init.c
>>>> @@ -175,14 +175,9 @@ void __init setup_memory(void)
>>>>  
>>>>  		start_pfn = memblock_region_memory_base_pfn(reg);
>>>>  		end_pfn = memblock_region_memory_end_pfn(reg);
>>>> -		memblock_set_node(start_pfn << PAGE_SHIFT,
>>>> -				  (end_pfn - start_pfn) << PAGE_SHIFT,
>>>> -				  &memblock.memory, 0);
>>>> +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
>>>
>>> memory_present() expects pfns, the shift is not needed.
>>
>> Right.
>>
>> -- 
>> Michal Hocko
>> SUSE Labs


-- 
~Randy

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 4592 bytes --]

On 31. 07. 19 19:15, Mike Rapoport wrote:
> On Wed, Jul 31, 2019 at 04:41:14PM +0200, Michal Hocko wrote:
>> On Wed 31-07-19 17:21:29, Mike Rapoport wrote:
>>> On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
>>>>
>>>> I am sorry, but I still do not follow. Who is consuming that node id
>>>> information when NUMA=n. In other words why cannot we simply do
>>>  
>>> We can, I think nobody cared to change it.
>>
>> It would be great if somebody with the actual HW could try it out.
>> I can throw a patch but I do not even have a cross compiler in my
>> toolbox.
> 
> Well, it compiles :)
>  
>>>> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
>>>> index a015a951c8b7..3a47e8db8d1c 100644
>>>> --- a/arch/microblaze/mm/init.c
>>>> +++ b/arch/microblaze/mm/init.c
>>>> @@ -175,14 +175,9 @@ void __init setup_memory(void)
>>>>  
>>>>  		start_pfn = memblock_region_memory_base_pfn(reg);
>>>>  		end_pfn = memblock_region_memory_end_pfn(reg);
>>>> -		memblock_set_node(start_pfn << PAGE_SHIFT,
>>>> -				  (end_pfn - start_pfn) << PAGE_SHIFT,
>>>> -				  &memblock.memory, 0);
>>>> +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
>>>
>>> memory_present() expects pfns, the shift is not needed.
>>
>> Right.

Sorry for slow response on this. In general regarding this topic.
Microblaze is soft core CPU (now there are hardcore versions too but not
running Linux). I believe there could be Numa system with
microblaze/microblazes (SMP is not supported in mainline).

This code was added in 2011 which is pretty hard to remember why it was
done in this way.

It compiles but not working on HW. Please take a look at log below.

Thanks,
Michal


[    0.000000] Linux version 5.3.0-rc6-00007-g54b01939182f-dirty
(monstr@monstr-desktop3) (gcc version 8.2.0 (crosstool-NG 1.20.0)) #101
Mon Sep 2 15:44:05 CEST 2019
[    0.000000] setup_memory: max_mapnr: 0x40000
[    0.000000] setup_memory: min_low_pfn: 0x80000
[    0.000000] setup_memory: max_low_pfn: 0xb0000
[    0.000000] setup_memory: max_pfn: 0xc0000
[    0.000000] start pfn 0x80000
[    0.000000] end pfn 0xc0000
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x0000000080000000-0x00000000afffffff]
[    0.000000]   Normal   empty
[    0.000000]   HighMem  [mem 0x00000000b0000000-0x00000000bfffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   1: [mem 0x0000000080000000-0x00000000bfffffff]
[    0.000000] Could not find start_pfn for node 0
[    0.000000] Initmem setup node 0 [mem
0x0000000000000000-0x0000000000000000]
[    0.000000] earlycon: ns16550a0 at MMIO 0x44a01000 (options '115200n8')
[    0.000000] printk: bootconsole [ns16550a0] enabled
[    0.000000] setup_cpuinfo: initialising
[    0.000000] setup_cpuinfo: Using full CPU PVR support
[    0.000000] wt_msr_noirq
[    0.000000] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768
[    0.000000] pcpu-alloc: [0] 0
[    0.000000] Built 1 zonelists, mobility grouping off.  Total pages: 0
[    0.000000] Kernel command line: earlycon
[    0.000000] Dentry cache hash table entries: -2147483648 (order: -13,
0 bytes, linear)
[    0.000000] Inode-cache hash table entries: -2147483648 (order: -13,
0 bytes, linear)
[    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.000000] Oops: kernel access of bad area, sig: 11
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
5.3.0-rc6-00007-g54b01939182f-dirty #101
[    0.000000]  Registers dump: mode=805B9EA8
[    0.000000]  r1=000065A0, r2=C05B7AE6, r3=00000000, r4=00000000
[    0.000000]  r5=00080000, r6=00080B50, r7=00000000, r8=00000004
[    0.000000]  r9=00000000, r10=0000001F, r11=00000000, r12=00006666
[    0.000000]  r13=4119DCC0, r14=00000000, r15=C05EFF8C, r16=00000000
[    0.000000]  r17=C0604408, r18=FFFC0000, r19=C05B9F6C, r20=BFFEC168
[    0.000000]  r21=BFFEC168, r22=EFFF9AC0, r23=00000001, r24=C0606874
[    0.000000]  r25=BFE6B74C, r26=80000000, r27=00000000, r28=90000040
[    0.000000]  r29=01000000, r30=00000380, r31=C05C02F0, rPC=C0604408
[    0.000000]  msr=000046A0, ear=00000004, esr=00000D12, fsr=FFFFFFFF
[    0.000000] Oops: kernel access of bad area, sig: 11


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: microblaze HAVE_MEMBLOCK_NODE_MAP dependency (was Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA)

[Mike Rapoport]

On Mon, Sep 02, 2019 at 03:51:25PM +0200, Michal Simek wrote:
> On 31. 07. 19 19:15, Mike Rapoport wrote:
> > On Wed, Jul 31, 2019 at 04:41:14PM +0200, Michal Hocko wrote:
> >> On Wed 31-07-19 17:21:29, Mike Rapoport wrote:
> >>> On Wed, Jul 31, 2019 at 03:00:37PM +0200, Michal Hocko wrote:
> >>>>
> >>>> I am sorry, but I still do not follow. Who is consuming that node id
> >>>> information when NUMA=n. In other words why cannot we simply do
> >>>  
> >>> We can, I think nobody cared to change it.
> >>
> >> It would be great if somebody with the actual HW could try it out.
> >> I can throw a patch but I do not even have a cross compiler in my
> >> toolbox.
> > 
> > Well, it compiles :)
> >  
> >>>> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> >>>> index a015a951c8b7..3a47e8db8d1c 100644
> >>>> --- a/arch/microblaze/mm/init.c
> >>>> +++ b/arch/microblaze/mm/init.c
> >>>> @@ -175,14 +175,9 @@ void __init setup_memory(void)
> >>>>  
> >>>>  		start_pfn = memblock_region_memory_base_pfn(reg);
> >>>>  		end_pfn = memblock_region_memory_end_pfn(reg);
> >>>> -		memblock_set_node(start_pfn << PAGE_SHIFT,
> >>>> -				  (end_pfn - start_pfn) << PAGE_SHIFT,
> >>>> -				  &memblock.memory, 0);
> >>>> +		memory_present(0, start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
> >>>
> >>> memory_present() expects pfns, the shift is not needed.
> >>
> >> Right.
> 
> Sorry for slow response on this. In general regarding this topic.
> Microblaze is soft core CPU (now there are hardcore versions too but not
> running Linux). I believe there could be Numa system with
> microblaze/microblazes (SMP is not supported in mainline).
> 
> This code was added in 2011 which is pretty hard to remember why it was
> done in this way.
> 
> It compiles but not working on HW. Please take a look at log below.
> 
> Thanks,
> Michal
> 
> 
> [    0.000000] Linux version 5.3.0-rc6-00007-g54b01939182f-dirty
> (monstr@monstr-desktop3) (gcc version 8.2.0 (crosstool-NG 1.20.0)) #101
> Mon Sep 2 15:44:05 CEST 2019
> [    0.000000] setup_memory: max_mapnr: 0x40000
> [    0.000000] setup_memory: min_low_pfn: 0x80000
> [    0.000000] setup_memory: max_low_pfn: 0xb0000
> [    0.000000] setup_memory: max_pfn: 0xc0000
> [    0.000000] start pfn 0x80000
> [    0.000000] end pfn 0xc0000
> [    0.000000] Zone ranges:
> [    0.000000]   DMA      [mem 0x0000000080000000-0x00000000afffffff]
> [    0.000000]   Normal   empty
> [    0.000000]   HighMem  [mem 0x00000000b0000000-0x00000000bfffffff]
> [    0.000000] Movable zone start for each node
> [    0.000000] Early memory node ranges
> [    0.000000]   node   1: [mem 0x0000000080000000-0x00000000bfffffff]
> [    0.000000] Could not find start_pfn for node 0
> [    0.000000] Initmem setup node 0 [mem
> 0x0000000000000000-0x0000000000000000]

This does not look good :)

I think the problem is that without an explicit call to memblock_set_node()
the ->nid in memblock is MAX_NUMNODES but free_area_init_nodes() presumes
actual node ids are properly set.

> [    0.000000] earlycon: ns16550a0 at MMIO 0x44a01000 (options '115200n8')
> [    0.000000] printk: bootconsole [ns16550a0] enabled
> [    0.000000] setup_cpuinfo: initialising
> [    0.000000] setup_cpuinfo: Using full CPU PVR support
> [    0.000000] wt_msr_noirq
> [    0.000000] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768
> [    0.000000] pcpu-alloc: [0] 0
> [    0.000000] Built 1 zonelists, mobility grouping off.  Total pages: 0
> [    0.000000] Kernel command line: earlycon
> [    0.000000] Dentry cache hash table entries: -2147483648 (order: -13,
> 0 bytes, linear)
> [    0.000000] Inode-cache hash table entries: -2147483648 (order: -13,
> 0 bytes, linear)
> [    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
> [    0.000000] Oops: kernel access of bad area, sig: 11
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 5.3.0-rc6-00007-g54b01939182f-dirty #101
> [    0.000000]  Registers dump: mode=805B9EA8
> [    0.000000]  r1=000065A0, r2=C05B7AE6, r3=00000000, r4=00000000
> [    0.000000]  r5=00080000, r6=00080B50, r7=00000000, r8=00000004
> [    0.000000]  r9=00000000, r10=0000001F, r11=00000000, r12=00006666
> [    0.000000]  r13=4119DCC0, r14=00000000, r15=C05EFF8C, r16=00000000
> [    0.000000]  r17=C0604408, r18=FFFC0000, r19=C05B9F6C, r20=BFFEC168
> [    0.000000]  r21=BFFEC168, r22=EFFF9AC0, r23=00000001, r24=C0606874
> [    0.000000]  r25=BFE6B74C, r26=80000000, r27=00000000, r28=90000040
> [    0.000000]  r29=01000000, r30=00000380, r31=C05C02F0, rPC=C0604408
> [    0.000000]  msr=000046A0, ear=00000004, esr=00000D12, fsr=FFFFFFFF
> [    0.000000] Oops: kernel access of bad area, sig: 11
> 
> 
> -- 
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
> 
> 




-- 
Sincerely yours,
Mike.