On 03/30/20 at 01:26pm, Mike Rapoport wrote:

On Mon, Mar 30, 2020 at 11:58:43AM +0200, Michal Hocko wrote:

quoted

On Mon 30-03-20 12:21:27, Mike Rapoport wrote:

quoted

On Mon, Mar 30, 2020 at 09:42:46AM +0200, Michal Hocko wrote:

quoted

On Sat 28-03-20 11:31:17, Hoan Tran wrote:

quoted

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. Most of them enables
this config by default with CONFIG_NUMA. This patch, by default, enables
CONFIG_NODES_SPAN_OTHER_NODES or uses early_pfn_in_nid() for NUMA.

I am not opposed to this at all. It reduces the config space and that is
a good thing on its own. The history has shown that meory layout might
be really wild wrt NUMA. The config is only used for early_pfn_in_nid
which is clearly an overkill.

Your description doesn't really explain why this is safe though. The
history of this config is somehow messy, though. Mike has tried
to remove it a94b3ab7eab4 ("[PATCH] mm: remove arch independent
NODES_SPAN_OTHER_NODES") just to be reintroduced by 7516795739bd
("[PATCH] Reintroduce NODES_SPAN_OTHER_NODES for powerpc") without any
reasoning what so ever. This doesn't make it really easy see whether
reasons for reintroduction are still there. Maybe there are some subtle
dependencies. I do not see any TBH but that might be burried deep in an
arch specific code.

Well, back then early_pfn_in_nid() was arch-dependant, today everyone
except ia64 rely on HAVE_MEMBLOCK_NODE_MAP.

What would it take to make ia64 use HAVE_MEMBLOCK_NODE_MAP? I would
really love to see that thing go away. It is causing problems when
people try to use memblock api.

Sorry, my bad, ia64 does not have NODES_SPAN_OTHER_NODES, but it does have
HAVE_MEMBLOCK_NODE_MAP.

I remember I've tried killing HAVE_MEMBLOCK_NODE_MAP, but I've run into
some problems and then I've got distracted. I too would like to have
HAVE_MEMBLOCK_NODE_MAP go away, maybe I'll take another look at it.
 

quoted

quoted

So, if the memblock node map
is correct, that using CONFIG_NUMA instead of CONFIG_NODES_SPAN_OTHER_NODES
would only mean that early_pfn_in_nid() will cost several cycles more on
architectures that didn't select CONFIG_NODES_SPAN_OTHER_NODES (i.e. arm64
and sh).

Do we have any idea on how much of an overhead that is? Because this is
per each pfn so it can accumulate a lot! 

It's O(log(N)) where N is the amount of the memory banks (ie. memblock.memory.cnt)

This is for the Node id searching. But early_pfn_in_nid() is calling for
each pfn, this is the big one, I think. Otherwise, it may be optimized
as no-op.

quoted

quoted

Agian, ia64 is an exception here.

Thanks for the clarification!
-- 
Michal Hocko
SUSE Labs

-- 
Sincerely yours,
Mike.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel