mm: make movable onlining suck less

Quoted 4 lines · Michal Hocko

mm/page_alloc.c +0 -0

@@ -5535,9 +5535,6 @@ int __meminit init_currently_empty_zone(struct zone *zone,
 			zone_start_pfn, (zone_start_pfn + size));
 

mm/page_alloc.c int __meminit init_currently_empty_zone(struct zone *zone, · line 5538

Quoted 5 lines

mm/page_alloc.c +0 -3

 	zone_init_free_lists(zone);
-	zone->initialized = 1;
-
-	return 0;
 }

Nit: Add changes more than correct.

Quoted 6 lines · Hillf Danton

Quoted 4 lines · Michal Hocko

mm/page_alloc.c +0 -0

@@ -5535,9 +5535,6 @@ int __meminit init_currently_empty_zone(struct zone *zone,
 			zone_start_pfn, (zone_start_pfn + size));
 

Quoted 6 lines

mm/page_alloc.c int __meminit init_currently_empty_zone(struct zone *zone, · line 5538

Quoted 5 lines

mm/page_alloc.c +0 -3

 	zone_init_free_lists(zone);
-	zone->initialized = 1;
-
-	return 0;
 }

Nit: Add changes more than correct.

I am sorry, I do not follow?

Fixed screw ups during the initial patch split up as per Hillf
---
From 8be6c5e47de66210e47710c80e72e8abd899017b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Wed, 29 Mar 2017 15:11:30 +0200
Subject: [PATCH] mm: get rid of zone_is_initialized

There shouldn't be any reason to add initialized when we can tell the same thing from checking whether there are any pages spanned to the zone. Remove zone_is_initialized() and replace it by zone_is_empty which can be used for the same set of tests.

This shouldn't have any visible effect

Signed-off-by Michal Hocko <[email protected]>

---
 include/linux/mmzone.h | 7 -------
 mm/memory_hotplug.c    | 6 +++---
 mm/page_alloc.c        | 3 +--
 3 files changed, 4 insertions(+), 12 deletions(-)

include/linux/mmzone.h +0 -7

index 618499159a7c..3bac3ed71c7a 100644
@@ -442,8 +442,6 @@ struct zone {
 	seqlock_t		span_seqlock;
 #endif
 
-	int initialized;
-
 	/* Write-intensive fields used from the page allocator */
 	ZONE_PADDING(_pad1_)
 

@@ -520,11 +518,6 @@ static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn)
 	return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone);
 }
 
-static inline bool zone_is_initialized(struct zone *zone)
-{
-	return zone->initialized;
-}
-
 static inline bool zone_is_empty(struct zone *zone)
 {
 	return zone->spanned_pages == 0;

mm/memory_hotplug.c +3 -3

index 6fb6bd2df787..699f5a2a8efd 100644
@@ -348,7 +348,7 @@ static void fix_zone_id(struct zone *zone, unsigned long start_pfn,
 static int __ref ensure_zone_is_initialized(struct zone *zone,
 			unsigned long start_pfn, unsigned long num_pages)
 {
-	if (!zone_is_initialized(zone))
+	if (zone_is_empty(zone))
 		return init_currently_empty_zone(zone, start_pfn, num_pages);
 
 	return 0;

@@ -1051,7 +1051,7 @@ bool zone_can_shift(unsigned long pfn, unsigned long nr_pages,
 
 		/* no zones in use between current zone and target */
 		for (i = idx + 1; i < target; i++)
-			if (zone_is_initialized(zone - idx + i))
+			if (!zone_is_empty(zone - idx + i))
 				return false;
 	}
 

@@ -1062,7 +1062,7 @@ bool zone_can_shift(unsigned long pfn, unsigned long nr_pages,
 
 		/* no zones in use between current zone and target */
 		for (i = target + 1; i < idx; i++)
-			if (zone_is_initialized(zone - idx + i))
+			if (!zone_is_empty(zone - idx + i))
 				return false;
 	}
 

mm/page_alloc.c +1 -2

index 5ee8a26fa383..756353d1e293 100644
@@ -795,7 +795,7 @@ static inline void __free_one_page(struct page *page,
 
 	max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
 
-	VM_BUG_ON(!zone_is_initialized(zone));
+	VM_BUG_ON(zone_is_empty(zone));
 	VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
 
 	VM_BUG_ON(migratetype == -1);

@@ -5535,7 +5535,6 @@ int __meminit init_currently_empty_zone(struct zone *zone,
 			zone_start_pfn, (zone_start_pfn + size));
 
 	zone_init_free_lists(zone);
-	zone->initialized = 1;
 
 	return 0;
 }

Signature · 5 lines

2.11.0

-- 
Michal Hocko
SUSE Labs

Quoted 12 lines · Michal Hocko

Fixed screw ups during the initial patch split up as per Hillf
---
From 8be6c5e47de66210e47710c80e72e8abd899017b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Wed, 29 Mar 2017 15:11:30 +0200
Subject: [PATCH] mm: get rid of zone_is_initialized

There shouldn't be any reason to add initialized when we can tell the same thing from checking whether there are any pages spanned to the zone. Remove zone_is_initialized() and replace it by zone_is_empty which can be used for the same set of tests.

Quoted 1 line

This shouldn't have any visible effect

I've decided to drop this patch. My main motivation was to simplify the hotplug workflow/ The situation is more hairy than I expected, though. On one hand all zones should be initialized early during the hotplug in add_memory_resource but direct users of arch_add_memory will need this to be called I suspect. Let's just keep the current status quo and clean up it later. It is not really needed for this series.

Quoted 15 lines · On Wed, 5 Apr 2017 10:14:00 +0200 Michal Hocko <[email protected]> wrote:

Quoted 13 lines · Michal Hocko

Fixed screw ups during the initial patch split up as per Hillf
---
From 8be6c5e47de66210e47710c80e72e8abd899017b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Wed, 29 Mar 2017 15:11:30 +0200
Subject: [PATCH] mm: get rid of zone_is_initialized

There shouldn't be any reason to add initialized when we can tell the same thing from checking whether there are any pages spanned to the zone. Remove zone_is_initialized() and replace it by zone_is_empty which can be used for the same set of tests.

This shouldn't have any visible effect

Quoted 6 lines

I've decided to drop this patch. My main motivation was to simplify the hotplug workflow/ The situation is more hairy than I expected, though. On one hand all zones should be initialized early during the hotplug in add_memory_resource but direct users of arch_add_memory will need this to be called I suspect. Let's just keep the current status quo and clean up it later. It is not really needed for this series.

Would you post v2 with all fixups you've done so far?

From: Michal Hocko<[email protected]>

these functions are unreachable because tile doesn't support memory hotplug becasuse it doesn't select ARCH_ENABLE_MEMORY_HOTPLUG nor it supports SPARSEMEM.

This code hasn't been compiled for a while obviously because nobody has noticed that __add_pages has a different signature since 2009.

Cc Chris Metcalf<[email protected]>

Signed-off-by Michal Hocko<[email protected]>

---
  arch/tile/mm/init.c | 30 ------------------------------
  1 file changed, 30 deletions(-)

From: Michal Hocko <[email protected]>

init_currently_empty_zone doesn't have any error to return yet it is still an int and callers try to be defensive and try to handle potential error. Remove this nonsense and simplify all callers.

Quoted 6 lines

mm/memory_hotplug.c +1 -3

-/* Can fail with -ENOMEM from allocating a wait table with vmalloc() or
- * alloc_bootmem_node_nopanic()/memblock_virt_alloc_node_nopanic() */
-static int __ref ensure_zone_is_initialized(struct zone *zone,
+static void __ref ensure_zone_is_initialized(struct zone *zone,
 			unsigned long start_pfn, unsigned long num_pages)
 {

mm/memory_hotplug.c static void fix_zone_id(struct zone *zone, unsigned long start_pfn, · line 351

Quoted 7 lines

mm/memory_hotplug.c +2 -4

-	if (zone_is_empty(zone))
-		return init_currently_empty_zone(zone, start_pfn, num_pages);
-
-	return 0;
+	if (!zone_is_empty(zone))
+		init_currently_empty_zone(zone, start_pfn, num_pages);
 }

Semantic change added?

On Fri 31-03-17 11:49:49, Hillf Danton wrote:
[...]

Quoted 6 lines

Quoted 6 lines

mm/memory_hotplug.c +1 -3

-/* Can fail with -ENOMEM from allocating a wait table with vmalloc() or
- * alloc_bootmem_node_nopanic()/memblock_virt_alloc_node_nopanic() */
-static int __ref ensure_zone_is_initialized(struct zone *zone,
+static void __ref ensure_zone_is_initialized(struct zone *zone,
 			unsigned long start_pfn, unsigned long num_pages)
 {

Quoted 8 lines

mm/memory_hotplug.c static void fix_zone_id(struct zone *zone, unsigned long start_pfn, · line 351

Quoted 7 lines

mm/memory_hotplug.c +2 -4

-	if (zone_is_empty(zone))
-		return init_currently_empty_zone(zone, start_pfn, num_pages);
-
-	return 0;
+	if (!zone_is_empty(zone))
+		init_currently_empty_zone(zone, start_pfn, num_pages);
 }

Semantic change added?

could you be more specific?

Quoted 17 lines · Michal Hocko

On Fri 31-03-17 11:49:49, Hillf Danton wrote:
[...]

Quoted 14 lines

mm/memory_hotplug.c static void fix_zone_id(struct zone *zone, unsigned long start_pfn, · line 351

Quoted 13 lines

mm/memory_hotplug.c +3 -7

-/* Can fail with -ENOMEM from allocating a wait table with vmalloc() or
- * alloc_bootmem_node_nopanic()/memblock_virt_alloc_node_nopanic() */
-static int __ref ensure_zone_is_initialized(struct zone *zone,
+static void __ref ensure_zone_is_initialized(struct zone *zone,
 			unsigned long start_pfn, unsigned long num_pages)
 {
-	if (zone_is_empty(zone))
-		return init_currently_empty_zone(zone, start_pfn, num_pages);
-
-	return 0;
+	if (!zone_is_empty(zone))
+		init_currently_empty_zone(zone, start_pfn, num_pages);
 }

Semantic change added?

Quoted 1 line

could you be more specific?

Well, I'm wondering why you are trying to initiate a nonempty zone.

init_currently_empty_zone doesn't have any error to return yet it is still an int and callers try to be defensive and try to handle potential error. Remove this nonsense and simplify all callers.

Quoted 3 lines · Michal Hocko

init_currently_empty_zone doesn't have any error to return yet it is still an int and callers try to be defensive and try to handle potential error. Remove this nonsense and simplify all callers.

Semi-related; arch_remove_memory() returns int, but callers ignore it.

Is that worth cleaning up? If so, should the implementations be simplified, or should we maybe do a pr_error() or something with it?

mm/memory_hotplug.c static void node_states_set_node(int node, struct memory_notify *arg) · line 1043

Quoted 11 lines · Michal Hocko

mm/memory_hotplug.c +10 -0

+static void __meminit resize_zone_range(struct zone *zone, unsigned long start_pfn,
+		unsigned long nr_pages)
+{
+	unsigned long old_end_pfn = zone_end_pfn(zone);
+
+	if (start_pfn < zone->zone_start_pfn)
+		zone->zone_start_pfn = start_pfn;
+
+	zone->spanned_pages = max(start_pfn + nr_pages, old_end_pfn) - zone->zone_start_pfn;
+}

The implementation above implies zone can only go bigger.

Can we resize zone with the given data?

From: Michal Hocko <[email protected]>

The current memory hotplug implementation relies on having all the struct pages associate with a zone during the physical hotplug phase (arch_add_memory->__add_pages->__add_section->__add_zone). In the vast majority of cases this means that they are added to ZONE_NORMAL. This has been so since 9d99aaa31f59 ("[PATCH] x86_64: Support memory hotadd without sparsemem") and it wasn't a big deal back then.

Much later memory hotplug wanted to (ab)use ZONE_MOVABLE for movable onlining 511c2aba8f07 ("mm, memory-hotplug: dynamic configure movable memory and portion memory") and then things got more complicated. Rather than reconsidering the zone association which was no longer needed (because the memory hotplug already depended on SPARSEMEM) a convoluted semantic of zone shifting has been developed. Only the currently last memblock or the one adjacent to the zone_movable can be onlined movable. This essentially means that the online time changes as the new memblocks are added.

Let's simulate memory hot online manually
Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal /sys/devices/system/memory/memory33/valid_zones:Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal /sys/devices/system/memory/memory33/valid_zones:Normal /sys/devices/system/memory/memory34/valid_zones:Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal /sys/devices/system/memory/memory33/valid_zones:Normal Movable /sys/devices/system/memory/memory34/valid_zones:Movable Normal

This is an awkward semantic because an udev event is sent as soon as the block is onlined and an udev handler might want to online it based on some policy (e.g. association with a node) but it will inherently race with new blocks showing up.

This patch changes the physical online phase to not associate pages with any zone at all. All the pages are just marked reserved and wait for the onlining phase to be associated with the zone as per the online request. There are only two requirements
- existing ZONE_NORMAL and ZONE_MOVABLE cannot overlap
- ZONE_NORMAL precedes ZONE_MOVABLE in physical addresses
the later on is not inherent and can be changed in the future. It preserves the current behavior and made the code slightly simpler. This is subject to change in future.

This means that the same physical online steps as above will lead to the following state:
Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal Movable /sys/devices/system/memory/memory33/valid_zones:Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal Movable /sys/devices/system/memory/memory33/valid_zones:Normal Movable /sys/devices/system/memory/memory34/valid_zones:Normal Movable

/sys/devices/system/memory/memory32/valid_zones:Normal Movable /sys/devices/system/memory/memory33/valid_zones:Normal Movable /sys/devices/system/memory/memory34/valid_zones:Movable

Implementation:
The current move_pfn_range is reimplemented to check the above requirements (allow_online_pfn_range) and then updates the respective zone (move_pfn_range_to_zone), the pgdat and links all the pages in the pfn range with the zone/node. __add_pages is updated to not require the zone and only initializes sections in the range. This allowed to simplify the arch_add_memory code (s390 could get rid of quite some of code).

devm_memremap_pages is the only user of arch_add_memory which relies on the zone association because it only hooks into the memory hotplug only half way. It uses it to associate the new memory with ZONE_DEVICE but doesn't allow it to be {on,off}lined via sysfs. This means that this particular code path has to call move_pfn_range_to_zone explicitly.

The original zone shifting code is kept in place and will be removed in the follow up patch for an easier review.

Cc Lai Jiangshan <[email protected]>

Cc Dan Williams <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Cc Heiko Carstens <[email protected]>

Cc [email protected]

Signed-off-by Michal Hocko <[email protected]>

---
 arch/ia64/mm/init.c            |   8 +-
 arch/powerpc/mm/mem.c          |  10 +--
 arch/s390/mm/init.c            |  30 +------
 arch/sh/mm/init.c              |   7 +-
 arch/x86/mm/init_32.c          |   5 +-
 arch/x86/mm/init_64.c          |   9 +-
 drivers/base/memory.c          |  52 ++++++-----
 include/linux/memory_hotplug.h |  13 +--
 kernel/memremap.c              |   3 +
 mm/memory_hotplug.c            | 195 +++++++++++++++++++++++++----------------
 mm/sparse.c                    |   3 +-
 11 files changed, 165 insertions(+), 170 deletions(-

Hi Michal,

building an x86 allmodconfig with next-20170404 results in the following section mismatch warnings probably caused by this patch:

WARNING: mm/built-in.o(.text+0x5a1c2): Section mismatch in reference from the function move_pfn_range_to_zone() to the function .meminit.text:memmap_init_zone() The function move_pfn_range_to_zone() references
the function __meminit memmap_init_zone().
This is often because move_pfn_range_to_zone lacks a __meminit
annotation or the annotation of memmap_init_zone is wrong.

WARNING: mm/built-in.o(.text+0x5a25b): Section mismatch in reference from the function move_pfn_range_to_zone() to the function .meminit.text:init_currently_empty_zone() The function move_pfn_range_to_zone() references
the function __meminit init_currently_empty_zone().
This is often because move_pfn_range_to_zone lacks a __meminit
annotation or the annotation of init_currently_empty_zone is wrong.

(quoted diff) +8 -5

-static int __meminit __add_section(int nid, struct zone *zone,
-					unsigned long phys_start_pfn)
+static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 {
 	int ret;
+	int i;
 
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
 
-	ret = sparse_add_one_section(zone, phys_start_pfn);
-
+	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn);
 	if (ret < 0)
 		return ret;
 
-	ret = __add_zone(zone, phys_start_pfn);
+	/*
+	 * Make all the pages reserved so that nobody will stumble over half
+	 * initialized state.
+	 */
+	for (i = 0; i < PAGES_PER_SECTION; i++) {

(quoted diff) +13 -0

+static struct zone * __meminit move_pfn_range(int online_type, int nid,
+		unsigned long start_pfn, unsigned long nr_pages)
+{
+	struct pglist_data *pgdat = NODE_DATA(nid);
+	struct zone *zone = &pgdat->node_zones[ZONE_NORMAL];
+
+	if (online_type == MMOP_ONLINE_KEEP) {
+		/*
+		 * MMOP_ONLINE_KEEP inherits the current zone which is
+		 * ZONE_NORMAL by default but we might be within ZONE_MOVABLE
+		 * already.
+		 */
+		if (allow_online_pfn_range(nid, start_pfn, nr_pages, MMOP_ONLINE_MOVABLE))

Patch 5 is the core of the change. In order to make it easier to review I have tried it to be as minimalistic as possible and the large code removal is moved to patch 6.

I would appreciate if s390 folks could take a look at patch 4 and the arch_add_memory because I am not sure I've grokked what they wanted to achieve there completely.

Quoted 4 lines · Michal Hocko

Patch 5 is the core of the change. In order to make it easier to review I have tried it to be as minimalistic as possible and the large code removal is moved to patch 6.

Quoted 3 lines

I would appreciate if s390 folks could take a look at patch 4 and the arch_add_memory because I am not sure I've grokked what they wanted to achieve there completely.

[adding Gerald Schaefer]

This seems to work fine on s390. So for the s390 bits:

Acked-by Heiko Carstens <[email protected]>

Any thoughts, complains, suggestions?

On Thu 30-03-17 13:54:48, Michal Hocko wrote:
[...]

Quoted 1 line

Any thoughts, complains, suggestions?

Anyting? I would really appreciate a feedback from IBM and Futjitsu guys who have shaped this code last few years. Also Igor and Vitaly seem to be using memory hotplug in virtualized environments. I do not expect they would see a huge advantage of the rework but I would appreciate to give it some testing to catch any potential regressions.

I plan to repost the series and would like to prevent from pointless submission if there are any obvious issues.

Thanks!

Anyting? I would really appreciate a feedback from IBM and Futjitsu guys who have shaped this code last few years. Also Igor and Vitaly seem to be using memory hotplug in virtualized environments. I do not expect they would see a huge advantage of the rework but I would appreciate to give it some testing to catch any potential regressions.

Quoted 5 lines · Michal Hocko

Anyting? I would really appreciate a feedback from IBM and Futjitsu guys who have shaped this code last few years. Also Igor and Vitaly seem to be using memory hotplug in virtualized environments. I do not expect they would see a huge advantage of the rework but I would appreciate to give it some testing to catch any potential regressions.

Sorry for the delayed reply.

With this set, I'm able to "online_movable" blocks in ascending order.

However, I am seeing a regression. When adding memory to a memoryless node, it shows up in node 0 instead. I'm digging to see if I can help narrow down where things go wrong.

Quoted 1 line · Reza Arbab

However, I am seeing a regression. When adding memory to a memoryless node, it shows up in node 0 instead. I'm digging to see if I can help narrow down where things go wrong.

OK, I guess I know what is going on here. online_pages relies on pfn_to_nid(pfn) to return a proper node. But we are doing page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being properly initialized. Damn, I should have noticed that. There are two ways around that. Either the __add_section stores the nid into the struct page and make page_to_nid reliable or store it somewhere else (ideally into the memblock). The first is easier (let's do it for now) but longterm we do not want to rely on the struct page at all I believe.

Does the following help?
---

mm/memory_hotplug.c +4 -1

index b9dc1c4e26c3..0e21b9f67c9d 100644
@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 
 	/*
 	 * Make all the pages reserved so that nobody will stumble over half
-	 * initialized state.
+	 * initialized state.
+	 * FIXME: We also have to associate it with a node because pfn_to_node
+	 * relies on having page with the proper node.
 	 */
 	for (i = 0; i < PAGES_PER_SECTION; i++) {
 		unsigned long pfn = phys_start_pfn + i;
+		struct page *page;

Quoted 32 lines · Michal Hocko

Quoted 3 lines · Reza Arbab

However, I am seeing a regression. When adding memory to a memoryless node, it shows up in node 0 instead. I'm digging to see if I can help narrow down where things go wrong.

OK, I guess I know what is going on here. online_pages relies on pfn_to_nid(pfn) to return a proper node. But we are doing page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being properly initialized. Damn, I should have noticed that. There are two ways around that. Either the __add_section stores the nid into the struct page and make page_to_nid reliable or store it somewhere else (ideally into the memblock). The first is easier (let's do it for now) but longterm we do not want to rely on the struct page at all I believe.

Does the following help?
---

mm/memory_hotplug.c +4 -1

index b9dc1c4e26c3..0e21b9f67c9d 100644
@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 
 	/*
 	 * Make all the pages reserved so that nobody will stumble over half
-	 * initialized state.
+	 * initialized state.
+	 * FIXME: We also have to associate it with a node because pfn_to_node
+	 * relies on having page with the proper node.
 	 */
 	for (i = 0; i < PAGES_PER_SECTION; i++) {
 		unsigned long pfn = phys_start_pfn + i;
+		struct page *page;

mm/memory_hotplug.c static int __meminit __add_section(int nid, unsigned long phys_start_pfn) · line 327

Quoted 10 lines

mm/memory_hotplug.c +3 -1

 		if (!pfn_valid(pfn))
 			continue;
 
-		SetPageReserved(pfn_to_page(phys_start_pfn + i));
+		page = pfn_to_page(pfn);
+		set_page_node(page, nid);
+		SetPageReserved(page);
 	}
 
 	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));

Almost there. I'm seeing the memory in the correct node now, but the /sys/devices/system/node/nodeX/memoryY links are not being created.

I think it's tripping up here, in register_mem_sect_under_node():

		page_nid = get_nid_for_pfn(pfn);
		if (page_nid < 0)
			continue;

[Let's add Gary who as introduced this code c04fc586c1a48]

Quoted 8 lines

Almost there. I'm seeing the memory in the correct node now, but the /sys/devices/system/node/nodeX/memoryY links are not being created.

I think it's tripping up here, in register_mem_sect_under_node():

		page_nid = get_nid_for_pfn(pfn);
		if (page_nid < 0)
			continue;

Huh, this code is confusing. How can we have a memblock spanning more nodes? If not then the loop over all sections in the memblock seem pointless as well. Also why do we require page_initialized() in get_nid_for_pfn? The changelog doesn't explain that and there are no comments that would help either.

Gary, could you clarify this please?

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Michal Hocko

[Let's add Gary who as introduced this code c04fc586c1a48]

OK, so Gary's email doesn't exist anymore. Does anybody can comment on this? I suspect this code is just-in-case... Mel?

Quoted 1 line · Reza Arbab

[...]

Quoted 14 lines

Quoted 8 lines

Almost there. I'm seeing the memory in the correct node now, but the /sys/devices/system/node/nodeX/memoryY links are not being created.

I think it's tripping up here, in register_mem_sect_under_node():

		page_nid = get_nid_for_pfn(pfn);
		if (page_nid < 0)
			continue;

Huh, this code is confusing. How can we have a memblock spanning more nodes? If not then the loop over all sections in the memblock seem pointless as well. Also why do we require page_initialized() in get_nid_for_pfn? The changelog doesn't explain that and there are no comments that would help either.

OK, so I've been thinkin about that and I believe that page_initialized check in get_nid_for_pfn is just bogus. There is nothing to rely on the page::lru to be already initialized. So I will go with the following as a separate preparatory patch.

I believe the whole code should be revisited and I have put that on my ever growing todo list because I suspect that it is more complex than necessary. I suspect that memblock do not span more nodes and all this is just-in-case code (e.g. the onlining code assumes a single zone aka node. But let's do that later.

---
From fd2e3b6eca1cf7766527203d23db6aca5957a3f1 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 4 Apr 2017 10:05:06 +0200
Subject: [PATCH] mm: drop page_initialized check from get_nid_for_pfn

c04fc586c1a4 ("mm: show node to memory section relationship with symlinks in sysfs") has added means to export memblock<->node association into the sysfs. It has also introduced get_nid_for_pfn which is a rather confusing counterpart of pfn_to_nid which checks also whether the pfn page is already initialized (page_initialized). This is done by checking page::lru != NULL which doesn't make any sense at all. Nothing in this path really relies on the lru list being used or initialized. Just remove it

Signed-off-by Michal Hocko <[email protected]>

---
drivers/base/node.c | 5 -----

1 file changed, 5 deletions(-)

drivers/base/node.c +0 -2

index 5548f9686016..ee080a35e869 100644
@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-#define page_initialized(page)  (page->lru.next)
-
 static int __ref get_nid_for_pfn(unsigned long pfn)
 {
 	struct page *page;

drivers/base/node.c +0 -2

index 5548f9686016..ee080a35e869 100644
@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-#define page_initialized(page)  (page->lru.next)
-
 static int __ref get_nid_for_pfn(unsigned long pfn)
 {
 	struct page *page;

Quoted 14 lines · Michal Hocko

drivers/base/node.c +0 -2

index 5548f9686016..ee080a35e869 100644
@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-#define page_initialized(page)  (page->lru.next)
-
 static int __ref get_nid_for_pfn(unsigned long pfn)
 {
 	struct page *page;

drivers/base/node.c static int __ref get_nid_for_pfn(unsigned long pfn) · line 383

Quoted 9 lines

drivers/base/node.c +0 -3

@@ -380,9 +378,6 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	if (system_state == SYSTEM_BOOTING)
 		return early_pfn_to_nid(pfn);
 #endif
-	page = pfn_to_page(pfn);
-	if (!page_initialized(page))
-		return -1;
 	return pfn_to_nid(pfn);
 }
 

You can get rid of 'page' altogether.

drivers/base/node.c: In function a??get_nid_for_pfna??:
drivers/base/node.c:373:15: warning: unused variable a??pagea?? [-Wunused-variable]

Thanks for your testing! This is highly appreciated. Can I assume your Tested-by?

Quoted 2 lines · Michal Hocko

Thanks for your testing! This is highly appreciated. Can I assume your Tested-by?

Of course! Not quite done, though.

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

I stuck some debugging prints in, for context:

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
shrink_pgdat_span: start_pfn=0x10100, end_pfn=0x10200, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000 ...%<...
shrink_pgdat_span: start_pfn=0x1fe00, end_pfn=0x1ff00, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
shrink_pgdat_span: start_pfn=0x1ff00, end_pfn=0x20000, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
find_biggest_section_pfn: start_pfn=0x0, end_pfn=0x1ff00
find_biggest_section_pfn loop: pfn=0x1feff, sec_nr = 0x1fe find_biggest_section_pfn loop: pfn=0x1fdff, sec_nr = 0x1fd
...%<...
find_biggest_section_pfn loop: pfn=0x1ff, sec_nr = 0x1
find_biggest_section_pfn loop: pfn=0xff, sec_nr = 0x0
find_biggest_section_pfn loop: pfn=0xffffffffffffffff, sec_nr = 0xffffffffffffff

Unable to handle kernel paging request for data at address 0xc000800000f19e78

Quoted 12 lines · Reza Arbab

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

Is this something new or an old issue? I believe the state after the online should be the same as before. So if you onlined the full node then there shouldn't be any difference. Let me have a look...

From 1b08ecef3e8ebcef585fe8f2b23155be54cce335 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 4 Apr 2017 21:09:00 +0200
Subject: [PATCH] mm, hotplug: get rid of zone/node shrinking

---
mm/memory_hotplug.c | 207 ----------------------------------------------------

1 file changed, 207 deletions(-)

Quoted 14 lines · Michal Hocko

Quoted 12 lines · Reza Arbab

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

Quoted 3 lines

Is this something new or an old issue? I believe the state after the online should be the same as before. So if you onlined the full node then there shouldn't be any difference. Let me have a look...

It's new. Without this patchset, I can repeatedly add_memory()->online_movable->offline->remove_memory() all of a node's memory.

Quoted 5 lines

From 1b08ecef3e8ebcef585fe8f2b23155be54cce335 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 4 Apr 2017 21:09:00 +0200
Subject: [PATCH] mm, hotplug: get rid of zone/node shrinking

...%<...

Quoted 3 lines

---
mm/memory_hotplug.c | 207 ----------------------------------------------------

1 file changed, 207 deletions(-)

Okay, getting further. With this I can again repeatedly add and remove, but now I'm seeing a weird variation of that earlier issue:

1. add_memory(), online_movable /sys/devices/system/node/nodeX/memoryY symlinks are created.

2. offline, remove_memory()
The node is offlined, since all memory has been removed, so all of /sys/devices/system/node/nodeX is gone. This is normal.

3. add_memory(), online_movable
The node is onlined, so /sys/devices/system/node/nodeX is recreated, and the memory is added, but just like earlier in this email thread, the memoryY links are not there.

Quoted 19 lines · Reza Arbab

Quoted 17 lines · Michal Hocko

Quoted 12 lines · Reza Arbab

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

Is this something new or an old issue? I believe the state after the online should be the same as before. So if you onlined the full node then there shouldn't be any difference. Let me have a look...

Quoted 3 lines

It's new. Without this patchset, I can repeatedly add_memory()->online_movable->offline->remove_memory() all of a node's memory.

This is quite unexpected because the code obviously cannot handle the first memory section. Could you paste /proc/zoneinfo and grep . -r /sys/devices/system/memory/auto_online_blocks/memory*, after onlining for both patched and unpatched kernels?

Quoted 5 lines · Michal Hocko

Quoted 3 lines · Reza Arbab

It's new. Without this patchset, I can repeatedly add_memory()->online_movable->offline->remove_memory() all of a node's memory.

Quoted 4 lines

This is quite unexpected because the code obviously cannot handle the first memory section. Could you paste /proc/zoneinfo and grep . -r /sys/devices/system/memory/auto_online_blocks/memory*, after onlining for both patched and unpatched kernels?

Btw. how do you test this? I am really surprised you managed to hotremove such a low pfn range.

I hope this made sense. :/

But one thing that is really bugging me is how could you see low pfns in the previous oops. Please drop the last patch and sprinkle printks down the remove_memory path to see where this all go south. I believe that there is something in the initialization code lurking in my code. Please also scratch the pfn_valid check in online_pages diff. It will not help here.

Quoted 6 lines · Michal Hocko

But one thing that is really bugging me is how could you see low pfns in the previous oops. Please drop the last patch and sprinkle printks down the remove_memory path to see where this all go south. I believe that there is something in the initialization code lurking in my code. Please also scratch the pfn_valid check in online_pages diff. It will not help here.

Got it.

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000

The problem is that pgdat_start_pfn here should be 0x10000. As you suspected, it never got set. This fixes things for me.

mm/memory_hotplug.c +0 -0

index 623507f..37c1b63 100644

Quoted 23 lines · Reza Arbab

Quoted 6 lines · Michal Hocko

But one thing that is really bugging me is how could you see low pfns in the previous oops. Please drop the last patch and sprinkle printks down the remove_memory path to see where this all go south. I believe that there is something in the initialization code lurking in my code. Please also scratch the pfn_valid check in online_pages diff. It will not help here.

Got it.

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000

The problem is that pgdat_start_pfn here should be 0x10000. As you suspected, it never got set. This fixes things for me.

mm/memory_hotplug.c +0 -0

index 623507f..37c1b63 100644
@@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
 {
 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
 

mm/memory_hotplug.c static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon · line 888

Quoted 3 lines

mm/memory_hotplug.c +1 -1

-	if (start_pfn < pgdat->node_start_pfn)
+	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
 		pgdat->node_start_pfn = start_pfn;

Dang! You are absolutely right. This explains the issue during the remove_memory. I still fail to see how this makes any difference for the sysfs file registration though. If anything the pgdat will be larger and so try_offline_node would check also unrelated node0 but the code will handle that and eventually offline the node1 anyway. /me still confused.

Quoted 23 lines · Reza Arbab

Quoted 6 lines · Michal Hocko

But one thing that is really bugging me is how could you see low pfns in the previous oops. Please drop the last patch and sprinkle printks down the remove_memory path to see where this all go south. I believe that there is something in the initialization code lurking in my code. Please also scratch the pfn_valid check in online_pages diff. It will not help here.

Got it.

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000

The problem is that pgdat_start_pfn here should be 0x10000. As you suspected, it never got set. This fixes things for me.

mm/memory_hotplug.c +0 -0

index 623507f..37c1b63 100644
@@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
 {
 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
 

mm/memory_hotplug.c static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon · line 888

Quoted 3 lines

mm/memory_hotplug.c +1 -1

-	if (start_pfn < pgdat->node_start_pfn)
+	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
 		pgdat->node_start_pfn = start_pfn;

Dang! You are absolutely right. This explains the issue during the remove_memory. I still fail to see how this makes any difference for the sysfs file registration though. If anything the pgdat will be larger and so try_offline_node would check also unrelated node0 but the code will handle that and eventually offline the node1 anyway. /me still confused.

OK, I was staring into the code and I guess I finally understand what is going on here. Looking at arch_add_memory->...->register_mem_sect_under_node was just misleading. I am still not 100% sure why but we try to do the same thing later from register_one_node->link_mem_sections for nodes which were offline. I should have noticed this path before. And here is the difference from the previous code. We are past arch_add_memory and that path used to do __add_zone which among other things will also resize node boundaries. I am not doing that anymore because I postpone that to the onlining phase. Jeez this code is so convoluted my head spins.

I am not really sure how to fix this. I suspect register_mem_sect_under_node should just ignore the online state of the node. But I wouldn't be all that surprised if this had some subtle reason as well. An alternative would be to actually move register_mem_sect_under_node out of register_new_memory and move it up the call stack, most probably to add_memory_resource. We have the range and can map it to the memblock and so will not rely on the node range. I will sleep over it and hopefully come up with something tomorrow.

Quoted 9 lines · Reza Arbab

Okay, getting further. With this I can again repeatedly add and remove, but now I'm seeing a weird variation of that earlier issue:

1. add_memory(), online_movable /sys/devices/system/node/nodeX/memoryY symlinks are created.

2. offline, remove_memory()
The node is offlined, since all memory has been removed, so all of /sys/devices/system/node/nodeX is gone. This is normal.

Quoted 4 lines

3. add_memory(), online_movable
The node is onlined, so /sys/devices/system/node/nodeX is recreated, and the memory is added, but just like earlier in this email thread, the memoryY links are not there.

Could you add some printks to see why the sysfs creation failed please?

Quoted 11 lines · Michal Hocko

Quoted 10 lines · Reza Arbab

Okay, getting further. With this I can again repeatedly add and remove, but now I'm seeing a weird variation of that earlier issue:

1. add_memory(), online_movable /sys/devices/system/node/nodeX/memoryY symlinks are created.

2. offline, remove_memory()
The node is offlined, since all memory has been removed, so all of /sys/devices/system/node/nodeX is gone. This is normal.

Quoted 6 lines

Quoted 4 lines

3. add_memory(), online_movable
The node is onlined, so /sys/devices/system/node/nodeX is recreated, and the memory is added, but just like earlier in this email thread, the memoryY links are not there.

Could you add some printks to see why the sysfs creation failed please?

Ah, simple enough. It's this, right at the top of
register_mem_sect_under_node():

	if (!node_online(nid))
		return 0;

That being the case, I really don't understand why your patches make any difference. Is node_set_online() being called later than before somehow?

This is really interesting. Because add_memory_resource does the following

	/* call arch's memory hotadd */
	ret = arch_add_memory(nid, start, size);

	if (ret < 0)
		goto error;

	/* we online node here. we can't roll back from here. */
	node_set_online(nid);

so we are setting the node online _after_ arch_add_memory but the code which adds those sysfs file is called from

arch_add_memory
__add_pages
__add_section
register_new_memory
register_mem_sect_under_node
node_online check

Quoted 19 lines · Reza Arbab

Quoted 11 lines · Michal Hocko

Quoted 10 lines · Reza Arbab

Okay, getting further. With this I can again repeatedly add and remove, but now I'm seeing a weird variation of that earlier issue:

1. add_memory(), online_movable /sys/devices/system/node/nodeX/memoryY symlinks are created.

2. offline, remove_memory()
The node is offlined, since all memory has been removed, so all of /sys/devices/system/node/nodeX is gone. This is normal.

Quoted 6 lines

Quoted 4 lines

3. add_memory(), online_movable
The node is onlined, so /sys/devices/system/node/nodeX is recreated, and the memory is added, but just like earlier in this email thread, the memoryY links are not there.

Could you add some printks to see why the sysfs creation failed please?

Quoted 8 lines

Ah, simple enough. It's this, right at the top of
register_mem_sect_under_node():

	if (!node_online(nid))
		return 0;

That being the case, I really don't understand why your patches make any difference. Is node_set_online() being called later than before somehow?

This is really interesting. Because add_memory_resource does the following

	/* call arch's memory hotadd */
	ret = arch_add_memory(nid, start, size);

	if (ret < 0)
		goto error;

	/* we online node here. we can't roll back from here. */
	node_set_online(nid);

so we are setting the node online _after_ arch_add_memory but the code which adds those sysfs file is called from

arch_add_memory
__add_pages
__add_section
register_new_memory
register_mem_sect_under_node
node_online check

I haven't touched this part. What is the point of this check anyway? We have already associated all the pages with a node (and with a zone prior to my patches) so we _know_ how to create those links. The check goes back to the initial submissions. Gary is not available anymore so we cannot ask. But I completely fail to see how my changes could have made any difference.

Quoted 14 lines · Michal Hocko

Quoted 12 lines · Reza Arbab

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

Quoted 3 lines

Is this something new or an old issue? I believe the state after the online should be the same as before. So if you onlined the full node then there shouldn't be any difference. Let me have a look...

It's new. Without this patchset, I can repeatedly add_memory()->online_movable->offline->remove_memory() all of a node's memory.

My code doesn't do that though. So I guess I have to sanitize. Does this help? Please drop the "mm, memory_hotplug: get rid of zone/node shrinking" patch.
---

mm/memory_hotplug.c +2 -0

index acf2b5eb5ecb..2c5613d19eb6 100644
@@ -750,6 +750,15 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	int ret;
 	struct memory_notify arg;
 
+	do {
+		if (pfn_valid(pfn))

Quoted 5 lines · Reza Arbab

Quoted 2 lines · Michal Hocko

Thanks for your testing! This is highly appreciated. Can I assume your Tested-by?

Of course! Not quite done, though.

Ohh, I didn't mean to rush you to that!

Quoted 12 lines

I think I found another edge case. You
get an oops when removing all of a node's memory:

__nr_to_section
__pfn_to_section
find_biggest_section_pfn
shrink_pgdat_span
__remove_zone
__remove_section
__remove_pages
arch_remove_memory
remove_memory

Is this something new or an old issue? I believe the state after the online should be the same as before. So if you onlined the full node then there shouldn't be any difference. Let me have a look...

Quoted 14 lines

I stuck some debugging prints in, for context:

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
shrink_pgdat_span: start_pfn=0x10100, end_pfn=0x10200, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000 ...%<...
shrink_pgdat_span: start_pfn=0x1fe00, end_pfn=0x1ff00, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
shrink_pgdat_span: start_pfn=0x1ff00, end_pfn=0x20000, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
find_biggest_section_pfn: start_pfn=0x0, end_pfn=0x1ff00
find_biggest_section_pfn loop: pfn=0x1feff, sec_nr = 0x1fe find_biggest_section_pfn loop: pfn=0x1fdff, sec_nr = 0x1fd
...%<...
find_biggest_section_pfn loop: pfn=0x1ff, sec_nr = 0x1
find_biggest_section_pfn loop: pfn=0xff, sec_nr = 0x0
find_biggest_section_pfn loop: pfn=0xffffffffffffffff, sec_nr = 0xffffffffffffff

Quoted 1 line

Unable to handle kernel paging request for data at address 0xc000800000f19e78

...this looks like a straight underflow and it is clear that the code is just broken. Have a look at the loop

	pfn = end_pfn - 1;
	for (; pfn >= start_pfn; pfn -= PAGES_PER_SECTION) {

assume that end_pfn is properly PAGES_PER_SECTION aligned (start_pfn would be 0 obviously). This is unsigned arithmetic and so it cannot work for the first section. So the code is broken and has been broken since it has been introduced. Nobody has noticed because the low pfns are usually reserved and out of the hotplug reach. We could tweak it but I am not even sure we really want/need this behavior. It complicates the code and am not really sure we need to support
online_movable(range)
offline_movable(range)
online_kernel(range)

OK, so after recent change mostly driven by testing from Reza Arbab (thanks again) I believe I am getting to a working state finally. All I currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more testing of course and if there are no new issues found I will repost the series for the review.

Quoted 7 lines · Michal Hocko

OK, so after recent change mostly driven by testing from Reza Arbab (thanks again) I believe I am getting to a working state finally. All I currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more testing of course and if there are no new issues found I will repost the series for the review.

Looking good! I can do my add/remove/repeat test and things seem fine.

One thing--starting on the second iteration, I am seeing the WARN in free_area_init_node();

add_memory
add_memory_resource
hotadd_new_pgdat

     free_area_init_node
	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);

Quoted 11 lines · Reza Arbab

Quoted 7 lines · Michal Hocko

OK, so after recent change mostly driven by testing from Reza Arbab (thanks again) I believe I am getting to a working state finally. All I currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more testing of course and if there are no new issues found I will repost the series for the review.

Looking good! I can do my add/remove/repeat test and things seem fine.

Quoted 8 lines

One thing--starting on the second iteration, I am seeing the WARN in free_area_init_node();

add_memory
add_memory_resource
hotadd_new_pgdat

     free_area_init_node
	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);

Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned elsewhere? Because I suspect that "mm: get rid of zone_is_initialized" might cause this.

Quoted 21 lines · Michal Hocko

Quoted 19 lines · Reza Arbab

Quoted 7 lines · Michal Hocko

OK, so after recent change mostly driven by testing from Reza Arbab (thanks again) I believe I am getting to a working state finally. All I currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more testing of course and if there are no new issues found I will repost the series for the review.

Looking good! I can do my add/remove/repeat test and things seem fine.

One thing--starting on the second iteration, I am seeing the WARN in free_area_init_node();

add_memory
add_memory_resource
hotadd_new_pgdat

    free_area_init_node
	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);

Quoted 3 lines

Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned elsewhere? Because I suspect that "mm: get rid of zone_is_initialized" might cause this.

This was my first time using your git branch instead of applying the patches from this thread to v4.11-rc5 myself.

Quoted 22 lines · Reza Arbab

Quoted 21 lines · Michal Hocko

Quoted 19 lines · Reza Arbab

Quoted 7 lines · Michal Hocko

OK, so after recent change mostly driven by testing from Reza Arbab (thanks again) I believe I am getting to a working state finally. All I currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more testing of course and if there are no new issues found I will repost the series for the review.

Looking good! I can do my add/remove/repeat test and things seem fine.

One thing--starting on the second iteration, I am seeing the WARN in free_area_init_node();

add_memory
add_memory_resource
hotadd_new_pgdat

    free_area_init_node
	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);

Quoted 6 lines

Quoted 3 lines

Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned elsewhere? Because I suspect that "mm: get rid of zone_is_initialized" might cause this.

This was my first time using your git branch instead of applying the patches from this thread to v4.11-rc5 myself.

OK, so this looks like another thing to resolve. I have seen this warning as well but I didn't consider it relevant because I had to tweak the code make the node go offline (removed check_and_unmap_cpu_on_node from try_offline_node) so I thought it was a fallout from there.

But let's have a look. hotadd_new_pgdat does for an existing pgdat

		/* Reset the nr_zones, order and classzone_idx before reuse */
		pgdat->nr_zones = 0;
		pgdat->kswapd_order = 0;
		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;

so free_area_init_node absolutely has to hit this warning. This is not in the Linus tree because it is still in Andrew's mmotm coming from
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

So yay, finally that doesn't come from me. Mel, I guess that either hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning should be updated.

Quoted 2 lines

This was my first time using your git branch instead of applying the patches from this thread to v4.11-rc5 myself.

OK, so this looks like another thing to resolve. I have seen this warning as well but I didn't consider it relevant because I had to tweak the code make the node go offline (removed check_and_unmap_cpu_on_node from try_offline_node) so I thought it was a fallout from there.

But let's have a look. hotadd_new_pgdat does for an existing pgdat

		/* Reset the nr_zones, order and classzone_idx before reuse */
		pgdat->nr_zones = 0;
		pgdat->kswapd_order = 0;
		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;

so free_area_init_node absolutely has to hit this warning. This is not in the Linus tree because it is still in Andrew's mmotm coming from
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

So yay, finally that doesn't come from me. Mel, I guess that either hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning should be updated.

Quoted 14 lines · Michal Hocko

Quoted 2 lines

This was my first time using your git branch instead of applying the patches from this thread to v4.11-rc5 myself.

OK, so this looks like another thing to resolve. I have seen this warning as well but I didn't consider it relevant because I had to tweak the code make the node go offline (removed check_and_unmap_cpu_on_node from try_offline_node) so I thought it was a fallout from there.

But let's have a look. hotadd_new_pgdat does for an existing pgdat

		/* Reset the nr_zones, order and classzone_idx before reuse */
		pgdat->nr_zones = 0;
		pgdat->kswapd_order = 0;
		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;

Quoted 8 lines

so free_area_init_node absolutely has to hit this warning. This is not in the Linus tree because it is still in Andrew's mmotm coming from
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

So yay, finally that doesn't come from me. Mel, I guess that either hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning should be updated.

Actually, it's obvious very quickly when I started the fix that updating the warning would then trigger on normal boot. It's more appropriate to let a hotadd of a new pgdat defer the initialisation of that field to kswapd starting for the new node.

Can you try this? It's build/boot tested only, no hotplug testing.

---8<---
mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx -fix

The patch "mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx" has different initial starting conditions when kswapd is asleep. kswapd initialises it properly when it starts but the patch initialises kswapd_classzone_idx early and trips on a warning in free_area_init_node. This patch leaves the kswapd_classzone_idx as zero and defers to kswapd to initialise it properly when it starts.

This is a fix to the mmotm patch mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

Signed-off-by Mel Gorman <[email protected]>

mm/memory_hotplug.c +6 -3

index 2309a7fbec93..76d4745513ee 100644
@@ -1214,10 +1214,14 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)
 
 		arch_refresh_nodedata(nid, pgdat);
 	} else {
-		/* Reset the nr_zones, order and classzone_idx before reuse */
+		/*
+		 * Reset the nr_zones, order and classzone_idx before reuse.
+		 * Note that kswapd will init kswapd_classzone_idx properly
+		 * when it starts in the near future.
+		 */
 		pgdat->nr_zones = 0;
 		pgdat->kswapd_order = 0;
-		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
+		pgdat->kswapd_classzone_idx = 0;
 	}
 
 	/* we can use NODE_DATA(nid) from here */
 
--

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 9 lines

---8<---
mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx -fix

The patch "mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx" has different initial starting conditions when kswapd is asleep. kswapd initialises it properly when it starts but the patch initialises kswapd_classzone_idx early and trips on a warning in free_area_init_node. This patch leaves the kswapd_classzone_idx as zero and defers to kswapd to initialise it properly when it starts.

It will start during the online phase which is later than this physical memory hotadd.

Quoted 4 lines

This is a fix to the mmotm patch mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

Signed-off-by Mel Gorman <[email protected]>

Yes, that is what I would expect. Feel free to add

Acked-by Michal Hocko <[email protected]>

if this is routed as a separate patch. Although I expect Andrew will fold it into the original patch.