Skip to contentSkip to conversation

mm/memory_hotplug: handle memblocks only with CONFIG_ARCH_KEEP_MEMBLOCK

PATCH Applied David Hildenbrand 1 participant 3 messages Apr 22, 2020

Subsystems (3)
Memory management MM core Memory hot(un)plug
lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

cover mm/memory_hotplug: handle memblocks only with CONFIG_ARCH_KEEP_MEMBLOCK

While working on some other cleanups I stumbled over the creation/removal of memblocks in hotplug code and wondered why we still need that. Turns out, we only need that handling with CONFIG_ARCH_KEEP_MEMBLOCK.

Gave it a quick test on x86-64.

RFC -> v1:
- Added ACKs
- Rephrased subject/description of patch #1
- Use IS_ENABLED() instead of ifdefs in patch #2

David Hildenbrand (2):
mm/memory_hotplug: set node_start_pfn of hotadded pgdat to 0 mm/memory_hotplug: handle memblocks only with
CONFIG_ARCH_KEEP_MEMBLOCK

 mm/Kconfig          |  3 +++
 mm/memory_hotplug.c | 35 ++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 19 deletions(-)
Signature · 1 line
2.25.3
author
#1 lore raw

PATCH 1/2 mm/memory_hotplug: set node_start_pfn of hotadded pgdat to 0

A hotadded node/pgdat will span no pages at all, until memory is moved to the zone/node via move_pfn_range_to_zone() -> resize_pgdat_range - e.g., when onlining memory blocks. We don't have to initialize the node_start_pfn to the memory we are adding.

Especially, there is an inconsistency:
- Hotplugging memory to a memory-less node with cpus: node_start_pf == 0
- Offlining and removing last memory from a node: node_start_pfn == 0
- Hotplugging memory to a memory-less node without cpus: node_start_pfn !=0

As soon as memory is onlined, node_start_pfn is overwritten with the actual start. E.g., when adding two DIMMs but only onlining one of both, only that DIMM (with online memory blocks) is spanned by the node.

Currently, the validity of node_start_pfn really is linked to node_spanned_pages != 0. With node_spanned_pages == 0 (e.g., before onlining memory), it has no meaning.

So let's stop setting node_start_pfn, just to be overwritten via move_pfn_range_to_zone(). This avoids confusion when looking at the code, wondering which magic will be performed with the node_start_pfn in this function, when hotadding a pgdat.

Acked-by Pankaj Gupta <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Baoquan He <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Pankaj Gupta <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 mm/memory_hotplug.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
mm/memory_hotplug.c +6 -9
index b65e0d1c9812..4dc263d2525a 100644
@@ -866,10 +866,9 @@ static void reset_node_present_pages(pg_data_t *pgdat)
 }
 
 /* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
-static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)
+static pg_data_t __ref *hotadd_new_pgdat(int nid)
 {
 	struct pglist_data *pgdat;
-	unsigned long start_pfn = PFN_DOWN(start);
 
 	pgdat = NODE_DATA(nid);
 	if (!pgdat) {

@@ -899,9 +898,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)
 	}
 
 	/* we can use NODE_DATA(nid) from here */
-
 	pgdat->node_id = nid;
-	pgdat->node_start_pfn = start_pfn;
+	pgdat->node_start_pfn = 0;
 
 	/* init node's zones as empty zones, we don't have any present pages.*/
 	free_area_init_core_hotplug(nid);

@@ -936,7 +934,6 @@ static void rollback_node_hotadd(int nid)
 /**
  * try_online_node - online a node if offlined
  * @nid: the node ID
- * @start: start addr of the node
  * @set_node_online: Whether we want to online the node
  * called by cpu_up() to online a node without onlined memory.
  *

@@ -945,7 +942,7 @@ static void rollback_node_hotadd(int nid)
  * 0 -> the node is already online
  * -ENOMEM -> the node could not be allocated
  */
-static int __try_online_node(int nid, u64 start, bool set_node_online)
+static int __try_online_node(int nid, bool set_node_online)
 {
 	pg_data_t *pgdat;
 	int ret = 1;

@@ -953,7 +950,7 @@ static int __try_online_node(int nid, u64 start, bool set_node_online)
 	if (node_online(nid))
 		return 0;
 
-	pgdat = hotadd_new_pgdat(nid, start);
+	pgdat = hotadd_new_pgdat(nid);
 	if (!pgdat) {
 		pr_err("Cannot online node %d due to NULL pgdat\n", nid);
 		ret = -ENOMEM;

@@ -977,7 +974,7 @@ int try_online_node(int nid)
 	int ret;
 
 	mem_hotplug_begin();
-	ret =  __try_online_node(nid, 0, true);
+	ret =  __try_online_node(nid, true);
 	mem_hotplug_done();
 	return ret;
 }

@@ -1036,7 +1033,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	 */
 	memblock_add_node(start, size, nid);
 
-	ret = __try_online_node(nid, start, false);
+	ret = __try_online_node(nid, false);
 	if (ret < 0)
 		goto error;
 	new_node = ret;
Signature · 1 line
2.25.3
author
#2 lore raw

PATCH 2/2 mm/memory_hotplug: handle memblocks only with CONFIG_ARCH_KEEP_MEMBLOCK

In reply to David Hildenbrand

The comment in add_memory_resource() is stale: hotadd_new_pgdat() will no longer call get_pfn_range_for_nid(), as a hotadded pgdat will simply span no pages at all, until memory is moved to the zone/node via move_pfn_range_to_zone() - e.g., when onlining memory blocks.

The only archs that care about memblocks for hotplugged memory (either for iterating over all system RAM or testing for memory validity) are arm64, s390x, and powerpc - due to CONFIG_ARCH_KEEP_MEMBLOCK. Without CONFIG_ARCH_KEEP_MEMBLOCK, we can simply stop messing with memblocks.

Acked-by Mike Rapoport <[email protected]>
Acked-by Michal Hocko <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Baoquan He <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Pankaj Gupta <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Anshuman Khandual <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 mm/Kconfig          |  3 +++
 mm/memory_hotplug.c | 20 ++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)
mm/Kconfig +3 -0
index 3af64646f343..db626b8d4fdb 100644
@@ -133,6 +133,9 @@ config HAVE_FAST_GUP
 	depends on MMU
 	bool
 
+# Don't discard allocated memory used to track "memory" and "reserved" memblocks
+# after early boot, so it can still be used to test for validity of memory.
+# Also, memblocks are updated with memory hot(un)plug.
 config ARCH_KEEP_MEMBLOCK
 	bool
mm/memory_hotplug.c +10 -10
index 4dc263d2525a..555137bd0882 100644
@@ -1025,13 +1025,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 
 	mem_hotplug_begin();
 
-	/*
-	 * Add new range to memblock so that when hotadd_new_pgdat() is called
-	 * to allocate new pgdat, get_pfn_range_for_nid() will be able to find
-	 * this new range and calculate total pages correctly.  The range will
-	 * be removed at hot-remove time.
-	 */
-	memblock_add_node(start, size, nid);
+	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
+		memblock_add_node(start, size, nid);
 
 	ret = __try_online_node(nid, false);
 	if (ret < 0)

@@ -1080,7 +1075,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	/* rollback pgdat allocation and others */
 	if (new_node)
 		rollback_node_hotadd(nid);
-	memblock_remove(start, size);
+	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
+		memblock_remove(start, size);
 	mem_hotplug_done();
 	return ret;
 }

@@ -1677,8 +1673,12 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
 	mem_hotplug_begin();
 
 	arch_remove_memory(nid, start, size, NULL);
-	memblock_free(start, size);
-	memblock_remove(start, size);
+
+	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
+		memblock_free(start, size);
+		memblock_remove(start, size);
+	}
+
 	__release_memory_resource(start, size);
 
 	try_offline_node(nid);
Signature · 1 line
2.25.3