mm/memory_hotplug: handle memblocks only with CONFIG_ARCH_KEEP_MEMBLOCK

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.

Note: we'll also end up with pgdat->node_start_pfn == 0 when offlined the last memory block belonging to a node (via remove_pfn_range_from_zone()-> update_pgdat_span()).

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 +5 -8

index 47cf6036eb31..9b15ce465be2 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;
 }

2.25.1

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.

Note: we'll also end up with pgdat->node_start_pfn == 0 when offlined the last memory block belonging to a node (via remove_pfn_range_from_zone()-> update_pgdat_span()).

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 47cf6036eb31..9b15ce465be2 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;
 }

@@ -1031,7 +1028,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.1

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

Quoted 8 lines · Michal Hocko

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

Quoted 5 lines

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

"information we already know" - no, not before we online the memory.

Before onlining, it's just setting node_start_pfn to *some value* to be overwritten in move_pfn_range_to_zone()->resize_pgdat_range().

Quoted 15 lines · David Hildenbrand

Quoted 13 lines · Michal Hocko

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

Quoted 1 line

"information we already know" - no, not before we online the memory.

Is this really the case? All add_memory_resource users operate on a physical memory range.

Quoted 3 lines

Before onlining, it's just setting node_start_pfn to *some value* to be overwritten in move_pfn_range_to_zone()->resize_pgdat_range().

Yes the value is overwritten but I am not sure this is actually correct thing to do. I cannot remember why I've chosen to do that. It doesn't really seem unlikely to online node in a higher physical address.

Btw. one thing that I have in my notes, I was never able to actually test the no numa node case. Because I have always been testing with node being allocated during the boot. Do you have any way to trigger this path?

Quoted 18 lines · Michal Hocko

Quoted 16 lines · David Hildenbrand

Quoted 8 lines · Michal Hocko

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

Quoted 5 lines

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

"information we already know" - no, not before we online the memory.

Quoted 2 lines

Is this really the case? All add_memory_resource users operate on a physical memory range.

Having the first add_memory() to magically set node_start_pfn of a hotplugged node isn't dangerous, I think we agree on that. It's just completely unnecessary here and at least left me confused why this is needed at all- because the node start/end pfn is only really touched when onlining/offlining memory (when resizing the zone and the pgdat).

Quoted 4 lines

Btw. one thing that I have in my notes, I was never able to actually test the no numa node case. Because I have always been testing with node being allocated during the boot. Do you have any way to trigger this path?

Sure, here is my test case

#! /bin/bash

sudo qemu-system-x86_64 \
    --enable-kvm \
    -m 4G,maxmem=20G,slots=2 \
    -smp sockets=2,cores=2 \
    -numa node,nodeid=0,cpus=0-1,mem=4G -numa node,nodeid=1,mem=0G \

Quoted 19 lines · David Hildenbrand

Quoted 18 lines · Michal Hocko

Quoted 16 lines · David Hildenbrand

Quoted 8 lines · Michal Hocko

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

Quoted 5 lines

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

"information we already know" - no, not before we online the memory.

Quoted 8 lines

Quoted 2 lines

Is this really the case? All add_memory_resource users operate on a physical memory range.

Having the first add_memory() to magically set node_start_pfn of a hotplugged node isn't dangerous, I think we agree on that. It's just completely unnecessary here and at least left me confused why this is needed at all- because the node start/end pfn is only really touched when onlining/offlining memory (when resizing the zone and the pgdat).

I do not see any specific problem. It just feels odd to
ignore the start pfn when we have that information. I am little bit
worried that this might kick back. E.g. say we start using the memmaps
from the hotplugged memory then the initial part of the node will never> get online and we would have memmaps outside of the node span. I do not

see an immediate problem except for the feeling this is odd.

That being said I will shut up now and leave it alone.

Quoted 29 lines · Michal Hocko

Quoted 19 lines · David Hildenbrand

Quoted 18 lines · Michal Hocko

Quoted 16 lines · David Hildenbrand

Quoted 8 lines · Michal Hocko

Sorry for the late reply

Quoted 4 lines · David Hildenbrand

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.

Quoted 5 lines

You are right that the node is empty at this phase but that is already reflected by zero present pages (hmm, I do not see spanned pages to be set 0 though). What I am missing here is why this is an improvement. The new node is already visible here and I do not see why we hide the information we already know.

"information we already know" - no, not before we online the memory.

Quoted 8 lines

Quoted 2 lines

Is this really the case? All add_memory_resource users operate on a physical memory range.

Having the first add_memory() to magically set node_start_pfn of a hotplugged node isn't dangerous, I think we agree on that. It's just completely unnecessary here and at least left me confused why this is needed at all- because the node start/end pfn is only really touched when onlining/offlining memory (when resizing the zone and the pgdat).

Quoted 4 lines

I do not see any specific problem. It just feels odd to
ignore the start pfn when we have that information. I am little bit
worried that this might kick back. E.g. say we start using the memmaps
from the hotplugged memory then the initial part of the node will never> get online and we would have memmaps outside of the node span. I do not

That's a general issue, which I pointed out as response to Oscars last series. This needs more thought and reworks, especially how node_start_pfn/node_spanned_pages are glued to memory onlining/offlining today.

Quoted 1 line

see an immediate problem except for the feeling this is odd.

I think it's inconsistent. E.g., start with memory-less/cpu-less node and don't online memory from the kernel immediately.

Hotplug CPU. PGDAT initialized with node_start_pfn=0. Hotplug memory. -> node_start_pfn=0 until memory is actually onlined.

Hotplug memory. PGDAT initialized with node_start_pfn=$VALUE. Hotplug CPU. -> node_start_pfn=$VALUE

Hotplug memory. PGDAT initialized with node_start_pfn=$VALUE. Hotplug CPU. Hotunplug memory.
-> node_start_pfn=$VALUE, although there is no memory anymore.

Hotplug memory 1. PGDAT initialized with node_start_pfn=$VALUE. Hotplug memory 2. Hotunplug memory 2.
-> node_start_pfn=$VALUE1 instead of $VALUE2.

Again, because node_start_pfn has absolutely no meaning until memory is actually onlined - today.

Quoted 2 lines

That being said I will shut up now and leave it alone.

Is that a nack?

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.

For s390x, it seems to be fairly easy to avoid CONFIG_ARCH_KEEP_MEMBLOCK. arm64 could rework most code (esp., pfn_valid(), valid_phys_addr_range() and kexec_file_load()) to not require memblocks for hotplugged memory. E.g., as hotplugged memory has no holes and can be identified using !early_section(), arm64's variant of pfn_valid() could be reworked fairly easily to not require memblocks for hotadded memory. powerpc might be more involed.

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 | 13 +++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

mm/Kconfig +3 -0

index c1acc34c1c35..a063fd9cdff4 100644
@@ -136,6 +136,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 +7 -6

index 9b15ce465be2..104285ee9ae8 100644
@@ -1020,13 +1020,9 @@ 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.
-	 */
+#ifdef CONFIG_ARCH_KEEP_MEMBLOCK
 	memblock_add_node(start, size, nid);
+#endif
 
 	ret = __try_online_node(nid, false);
 	if (ret < 0)

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

@@ -1751,8 +1749,11 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
 	mem_hotplug_begin();
 
 	arch_remove_memory(nid, start, size, NULL);
+
+#ifdef CONFIG_ARCH_KEEP_MEMBLOCK
 	memblock_free(start, size);
 	memblock_remove(start, size);
+#endif
 	__release_memory_resource(start, size);
 
 	try_offline_node(nid);

Signature · 1 line

2.25.1

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.

For s390x, it seems to be fairly easy to avoid CONFIG_ARCH_KEEP_MEMBLOCK. arm64 could rework most code (esp., pfn_valid(), valid_phys_addr_range() and kexec_file_load()) to not require memblocks for hotplugged memory. E.g., as hotplugged memory has no holes and can be identified using !early_section(), arm64's variant of pfn_valid() could be reworked fairly easily to not require memblocks for hotadded memory. powerpc might be more involed.

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/memory_hotplug.c +6 -0

 	memblock_add_node(start, size, nid);
+#endif
 
 	ret = __try_online_node(nid, false);
 	if (ret < 0)

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

@@ -1751,8 +1749,11 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
 	mem_hotplug_begin();
 
 	arch_remove_memory(nid, start, size, NULL);
+
+#ifdef CONFIG_ARCH_KEEP_MEMBLOCK
 	memblock_free(start, size);
 	memblock_remove(start, size);
+#endif
 	__release_memory_resource(start, size);
 
 	try_offline_node(nid);

Signature · 1 line

2.25.1

Quoted 7 lines

For s390x, it seems to be fairly easy to avoid CONFIG_ARCH_KEEP_MEMBLOCK. arm64 could rework most code (esp., pfn_valid(), valid_phys_addr_range() and kexec_file_load()) to not require memblocks for hotplugged memory. E.g., as hotplugged memory has no holes and can be identified using !early_section(), arm64's variant of pfn_valid() could be reworked fairly easily to not require memblocks for hotadded memory. powerpc might be more involed.

I haven't checked these architectures but is the information really useful for this patch?

Quoted 9 lines

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]>

Acked-by Michal Hocko <[email protected]>

with a minor nit

mm/memory_hotplug.c int __ref add_memory_resource(int nid, struct resource *res) · line 1023

Quoted 7 lines

mm/memory_hotplug.c +1 -6

-	/*
-	 * 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.
-	 */
+#ifdef CONFIG_ARCH_KEEP_MEMBLOCK

if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)

would be slightly nicer. This should work for all the ifedefs in this patch.