[PATCH v2 0/3] mm: fix memory to node bad links in sysfs

[PATCH v2 0/3] mm: fix memory to node bad links in sysfs

[Laurent Dufour]

Sometimes, firmware may expose interleaved memory layout like this:
 Early memory node ranges
   node   1: [mem 0x0000000000000000-0x000000011fffffff]
   node   2: [mem 0x0000000120000000-0x000000014fffffff]
   node   1: [mem 0x0000000150000000-0x00000001ffffffff]
   node   0: [mem 0x0000000200000000-0x000000048fffffff]
   node   2: [mem 0x0000000490000000-0x00000007ffffffff]

In that case, we can see memory blocks assigned to multiple nodes in sysfs:

$ ls -l /sys/devices/system/memory/memory21
total 0
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node1 -> ../../node/node1
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node2 -> ../../node/node2
-rw-r--r-- 1 root root 65536 Aug 24 05:27 online
-r--r--r-- 1 root root 65536 Aug 24 05:27 phys_device
-r--r--r-- 1 root root 65536 Aug 24 05:27 phys_index
drwxr-xr-x 2 root root     0 Aug 24 05:27 power
-r--r--r-- 1 root root 65536 Aug 24 05:27 removable
-rw-r--r-- 1 root root 65536 Aug 24 05:27 state
lrwxrwxrwx 1 root root     0 Aug 24 05:25 subsystem -> ../../../../bus/memory
-rw-r--r-- 1 root root 65536 Aug 24 05:25 uevent
-r--r--r-- 1 root root 65536 Aug 24 05:27 valid_zones

The same applies in the node's directory with a memory21 link in both the
node1 and node2's directory.

This is wrong but doesn't prevent the system to run. However when later one
of these memory blocks is hot-unplugged and then hot-plugged, the system is
detecting an inconsistency in the sysfs layout and a BUG_ON() is raised:

------------[ cut here ]------------
kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
NIP:  c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
REGS: c0000004876e3660 TRAP: 0700   Not tainted  (5.9.0-rc1+)
MSR:  800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 24000448  XER: 20040000
CFAR: c000000000846d20 IRQMASK: 0
GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
NIP [c000000000403f34] add_memory_resource+0x244/0x340
LR [c000000000403f2c] add_memory_resource+0x23c/0x340
Call Trace:
[c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
[c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
[c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
[c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
[c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
[c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
[c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
[c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
[c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
[c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
[c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
[c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
[c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
Instruction dump:
48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
---[ end trace 562fd6c109cd0fb2 ]---

This has been seen on PowerPC LPAR.

The root cause of this issue is that when node's memory is registered, the
range used can overlap another node's range, thus the memory block is
registered to multiple nodes in sysfs.

There are 2 issues here:

a. The sysfs memory and node's layout are broken due to these multiple
   links

b. The link errors in link_mem_sections() should not lead to a system
   panic.

To address a. register_mem_sect_under_node should not rely on the system
state to detect whether the link operation is triggered by a hot plug
operation or not. This is addressed by the patches 1 and 2 of this series.

The patch 3 is addressing the point b.

Thanks,
Laurent

Since v1:
- change context enum's name from Michal's comment
- use 2 callbacks in link_mem_sections from David's comment
- use dev_err_ratelimited from Greg's comment

Laurent Dufour (3):
  mm: replace memmap_context by memplug_context
  mm: don't rely on system state to detect hot-plug operations
  mm: don't panic when links can't be created in sysfs

 arch/ia64/mm/init.c    |  6 +--
 drivers/base/node.c    | 99 ++++++++++++++++++++++++++++--------------
 include/linux/mm.h     |  2 +-
 include/linux/mmzone.h | 11 +++--
 include/linux/node.h   | 13 +++---
 mm/memory_hotplug.c    |  6 +--
 mm/page_alloc.c        | 10 ++---
 7 files changed, 94 insertions(+), 53 deletions(-)

-- 
2.28.0

[PATCH v2 0/3] mm: fix memory to node bad links in sysfs

[Laurent Dufour]

Sometimes, firmware may expose interleaved memory layout like this:
 Early memory node ranges
   node   1: [mem 0x0000000000000000-0x000000011fffffff]
   node   2: [mem 0x0000000120000000-0x000000014fffffff]
   node   1: [mem 0x0000000150000000-0x00000001ffffffff]
   node   0: [mem 0x0000000200000000-0x000000048fffffff]
   node   2: [mem 0x0000000490000000-0x00000007ffffffff]

In that case, we can see memory blocks assigned to multiple nodes in sysfs:

$ ls -l /sys/devices/system/memory/memory21
total 0
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node1 -> ../../node/node1
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node2 -> ../../node/node2
-rw-r--r-- 1 root root 65536 Aug 24 05:27 online
-r--r--r-- 1 root root 65536 Aug 24 05:27 phys_device
-r--r--r-- 1 root root 65536 Aug 24 05:27 phys_index
drwxr-xr-x 2 root root     0 Aug 24 05:27 power
-r--r--r-- 1 root root 65536 Aug 24 05:27 removable
-rw-r--r-- 1 root root 65536 Aug 24 05:27 state
lrwxrwxrwx 1 root root     0 Aug 24 05:25 subsystem -> ../../../../bus/memory
-rw-r--r-- 1 root root 65536 Aug 24 05:25 uevent
-r--r--r-- 1 root root 65536 Aug 24 05:27 valid_zones

The same applies in the node's directory with a memory21 link in both the
node1 and node2's directory.

This is wrong but doesn't prevent the system to run. However when later one
of these memory blocks is hot-unplugged and then hot-plugged, the system is
detecting an inconsistency in the sysfs layout and a BUG_ON() is raised:

------------[ cut here ]------------
kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZEdK MMU=Hash SMP NR_CPUS 48 NUMA pSeries
Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
NIP:  c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
REGS: c0000004876e3660 TRAP: 0700   Not tainted  (5.9.0-rc1+)
MSR:  800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 24000448  XER: 20040000
CFAR: c000000000846d20 IRQMASK: 0
GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
NIP [c000000000403f34] add_memory_resource+0x244/0x340
LR [c000000000403f2c] add_memory_resource+0x23c/0x340
Call Trace:
[c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
[c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
[c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
[c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
[c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
[c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
[c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
[c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
[c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
[c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
[c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
[c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
[c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
Instruction dump:
48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
---[ end trace 562fd6c109cd0fb2 ]---

This has been seen on PowerPC LPAR.

The root cause of this issue is that when node's memory is registered, the
range used can overlap another node's range, thus the memory block is
registered to multiple nodes in sysfs.

There are 2 issues here:

a. The sysfs memory and node's layout are broken due to these multiple
   links

b. The link errors in link_mem_sections() should not lead to a system
   panic.

To address a. register_mem_sect_under_node should not rely on the system
state to detect whether the link operation is triggered by a hot plug
operation or not. This is addressed by the patches 1 and 2 of this series.

The patch 3 is addressing the point b.

Thanks,
Laurent

Since v1:
- change context enum's name from Michal's comment
- use 2 callbacks in link_mem_sections from David's comment
- use dev_err_ratelimited from Greg's comment

Laurent Dufour (3):
  mm: replace memmap_context by memplug_context
  mm: don't rely on system state to detect hot-plug operations
  mm: don't panic when links can't be created in sysfs

 arch/ia64/mm/init.c    |  6 +--
 drivers/base/node.c    | 99 ++++++++++++++++++++++++++++--------------
 include/linux/mm.h     |  2 +-
 include/linux/mmzone.h | 11 +++--
 include/linux/node.h   | 13 +++---
 mm/memory_hotplug.c    |  6 +--
 mm/page_alloc.c        | 10 ++---
 7 files changed, 94 insertions(+), 53 deletions(-)

-- 
2.28.0

[PATCH v2 1/3] mm: replace memmap_context by memplug_context

[Laurent Dufour]

The memmap_context is used to detect whether a memory operation is due to a
hot-add operation or happening at boot time.

Make it general to the hotplug operation and rename it at memplug_context.

There is no functional change introduced by this patch

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
 arch/ia64/mm/init.c    |  6 +++---
 include/linux/mm.h     |  2 +-
 include/linux/mmzone.h | 11 ++++++++---
 mm/memory_hotplug.c    |  2 +-
 mm/page_alloc.c        | 10 +++++-----
 5 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index 0b3fb4c7af29..b5054b5e77c8 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
 	if (map_start < map_end)
 		memmap_init_zone((unsigned long)(map_end - map_start),
 				 args->nid, args->zone, page_to_pfn(map_start),
-				 MEMMAP_EARLY, NULL);
+				 MEMPLUG_EARLY, NULL);
 	return 0;
 }
 
@@ -547,8 +547,8 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
 	     unsigned long start_pfn)
 {
 	if (!vmem_map) {
-		memmap_init_zone(size, nid, zone, start_pfn, MEMMAP_EARLY,
-				NULL);
+		memmap_init_zone(size, nid, zone, start_pfn,
+				 MEMPLUG_EARLY, NULL);
 	} else {
 		struct page *start;
 		struct memmap_init_callback_data args;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1983e08f5906..e942f91ed155 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2409,7 +2409,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn,
 
 extern void set_dma_reserve(unsigned long new_dma_reserve);
 extern void memmap_init_zone(unsigned long, int, unsigned long, unsigned long,
-		enum memmap_context, struct vmem_altmap *);
+		enum meminit_context, struct vmem_altmap *);
 extern void setup_per_zone_wmarks(void);
 extern int __meminit init_per_zone_wmark_min(void);
 extern void mem_init(void);
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 8379432f4f2f..0f7a4ff4b059 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -824,10 +824,15 @@ bool zone_watermark_ok(struct zone *z, unsigned int order,
 		unsigned int alloc_flags);
 bool zone_watermark_ok_safe(struct zone *z, unsigned int order,
 		unsigned long mark, int highest_zoneidx);
-enum memmap_context {
-	MEMMAP_EARLY,
-	MEMMAP_HOTPLUG,
+/*
+ * Memory initialization context, use to differentiate memory added by
+ * the platform statically or via memory hotplug interface.
+ */
+enum meminit_context {
+	MEMINIT_EARLY,
+	MEMINIT_HOTPLUG,
 };
+
 extern void init_currently_empty_zone(struct zone *zone, unsigned long start_pfn,
 				     unsigned long size);
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e9d5ab5d3ca0..fc21625e42de 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -729,7 +729,7 @@ void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 	 * are reserved so nobody should be touching them so we should be safe
 	 */
 	memmap_init_zone(nr_pages, nid, zone_idx(zone), start_pfn,
-			MEMMAP_HOTPLUG, altmap);
+			 MEMPLUG_HOTPLUG, altmap);
 
 	set_zone_contiguous(zone);
 }
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fab5e97dc9ca..5661fa164f13 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5975,7 +5975,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * done. Non-atomic initialization, single-pass.
  */
 void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
-		unsigned long start_pfn, enum memmap_context context,
+		unsigned long start_pfn, enum meminit_context context,
 		struct vmem_altmap *altmap)
 {
 	unsigned long pfn, end_pfn = start_pfn + size;
@@ -6007,7 +6007,7 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 		 * There can be holes in boot-time mem_map[]s handed to this
 		 * function.  They do not exist on hotplugged memory.
 		 */
-		if (context == MEMMAP_EARLY) {
+		if (context == MEMINIT_EARLY) {
 			if (overlap_memmap_init(zone, &pfn))
 				continue;
 			if (defer_init(nid, pfn, end_pfn))
@@ -6016,7 +6016,7 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 
 		page = pfn_to_page(pfn);
 		__init_single_page(page, pfn, zone, nid);
-		if (context == MEMMAP_HOTPLUG)
+		if (context == MEMINIT_HOTPLUG)
 			__SetPageReserved(page);
 
 		/*
@@ -6099,7 +6099,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
 		 * check here not to call set_pageblock_migratetype() against
 		 * pfn out of zone.
 		 *
-		 * Please note that MEMMAP_HOTPLUG path doesn't clear memmap
+		 * Please note that MEMINIT_HOTPLUG path doesn't clear memmap
 		 * because this is done early in section_activate()
 		 */
 		if (!(pfn & (pageblock_nr_pages - 1))) {
@@ -6137,7 +6137,7 @@ void __meminit __weak memmap_init(unsigned long size, int nid,
 		if (end_pfn > start_pfn) {
 			size = end_pfn - start_pfn;
 			memmap_init_zone(size, nid, zone, start_pfn,
-					 MEMMAP_EARLY, NULL);
+					 MEMINIT_EARLY, NULL);
 		}
 	}
 }
-- 
2.28.0

[PATCH v2 1/3] mm: replace memmap_context by memplug_context

[Laurent Dufour]

The memmap_context is used to detect whether a memory operation is due to a
hot-add operation or happening at boot time.

Make it general to the hotplug operation and rename it at memplug_context.

There is no functional change introduced by this patch

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
 arch/ia64/mm/init.c    |  6 +++---
 include/linux/mm.h     |  2 +-
 include/linux/mmzone.h | 11 ++++++++---
 mm/memory_hotplug.c    |  2 +-
 mm/page_alloc.c        | 10 +++++-----
 5 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index 0b3fb4c7af29..b5054b5e77c8 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
 	if (map_start < map_end)
 		memmap_init_zone((unsigned long)(map_end - map_start),
 				 args->nid, args->zone, page_to_pfn(map_start),
-				 MEMMAP_EARLY, NULL);
+				 MEMPLUG_EARLY, NULL);
 	return 0;
 }
 
@@ -547,8 +547,8 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
 	     unsigned long start_pfn)
 {
 	if (!vmem_map) {
-		memmap_init_zone(size, nid, zone, start_pfn, MEMMAP_EARLY,
-				NULL);
+		memmap_init_zone(size, nid, zone, start_pfn,
+				 MEMPLUG_EARLY, NULL);
 	} else {
 		struct page *start;
 		struct memmap_init_callback_data args;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1983e08f5906..e942f91ed155 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2409,7 +2409,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn,
 
 extern void set_dma_reserve(unsigned long new_dma_reserve);
 extern void memmap_init_zone(unsigned long, int, unsigned long, unsigned long,
-		enum memmap_context, struct vmem_altmap *);
+		enum meminit_context, struct vmem_altmap *);
 extern void setup_per_zone_wmarks(void);
 extern int __meminit init_per_zone_wmark_min(void);
 extern void mem_init(void);
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 8379432f4f2f..0f7a4ff4b059 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -824,10 +824,15 @@ bool zone_watermark_ok(struct zone *z, unsigned int order,
 		unsigned int alloc_flags);
 bool zone_watermark_ok_safe(struct zone *z, unsigned int order,
 		unsigned long mark, int highest_zoneidx);
-enum memmap_context {
-	MEMMAP_EARLY,
-	MEMMAP_HOTPLUG,
+/*
+ * Memory initialization context, use to differentiate memory added by
+ * the platform statically or via memory hotplug interface.
+ */
+enum meminit_context {
+	MEMINIT_EARLY,
+	MEMINIT_HOTPLUG,
 };
+
 extern void init_currently_empty_zone(struct zone *zone, unsigned long start_pfn,
 				     unsigned long size);
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e9d5ab5d3ca0..fc21625e42de 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -729,7 +729,7 @@ void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 	 * are reserved so nobody should be touching them so we should be safe
 	 */
 	memmap_init_zone(nr_pages, nid, zone_idx(zone), start_pfn,
-			MEMMAP_HOTPLUG, altmap);
+			 MEMPLUG_HOTPLUG, altmap);
 
 	set_zone_contiguous(zone);
 }
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fab5e97dc9ca..5661fa164f13 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5975,7 +5975,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * done. Non-atomic initialization, single-pass.
  */
 void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
-		unsigned long start_pfn, enum memmap_context context,
+		unsigned long start_pfn, enum meminit_context context,
 		struct vmem_altmap *altmap)
 {
 	unsigned long pfn, end_pfn = start_pfn + size;
@@ -6007,7 +6007,7 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 		 * There can be holes in boot-time mem_map[]s handed to this
 		 * function.  They do not exist on hotplugged memory.
 		 */
-		if (context = MEMMAP_EARLY) {
+		if (context = MEMINIT_EARLY) {
 			if (overlap_memmap_init(zone, &pfn))
 				continue;
 			if (defer_init(nid, pfn, end_pfn))
@@ -6016,7 +6016,7 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 
 		page = pfn_to_page(pfn);
 		__init_single_page(page, pfn, zone, nid);
-		if (context = MEMMAP_HOTPLUG)
+		if (context = MEMINIT_HOTPLUG)
 			__SetPageReserved(page);
 
 		/*
@@ -6099,7 +6099,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
 		 * check here not to call set_pageblock_migratetype() against
 		 * pfn out of zone.
 		 *
-		 * Please note that MEMMAP_HOTPLUG path doesn't clear memmap
+		 * Please note that MEMINIT_HOTPLUG path doesn't clear memmap
 		 * because this is done early in section_activate()
 		 */
 		if (!(pfn & (pageblock_nr_pages - 1))) {
@@ -6137,7 +6137,7 @@ void __meminit __weak memmap_init(unsigned long size, int nid,
 		if (end_pfn > start_pfn) {
 			size = end_pfn - start_pfn;
 			memmap_init_zone(size, nid, zone, start_pfn,
-					 MEMMAP_EARLY, NULL);
+					 MEMINIT_EARLY, NULL);
 		}
 	}
 }
-- 
2.28.0

[PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[Laurent Dufour]

In register_mem_sect_under_node() the system_state’s value is checked to
detect whether the operation the call is made during boot time or during an
hot-plug operation. Unfortunately, that check against SYSTEM_BOOTING is
wrong because regular memory is registered at SYSTEM_SCHEDULING state. In
addition memory hot-plug operation can be triggered at this system state
too by the ACPI [1]. So checking against the system state is not enough.

The consequence is that on system with interleaved node's ranges like this:
 Early memory node ranges
   node   1: [mem 0x0000000000000000-0x000000011fffffff]
   node   2: [mem 0x0000000120000000-0x000000014fffffff]
   node   1: [mem 0x0000000150000000-0x00000001ffffffff]
   node   0: [mem 0x0000000200000000-0x000000048fffffff]
   node   2: [mem 0x0000000490000000-0x00000007ffffffff]

This can be seen on PowerPC LPAR after multiple memory hot-plug and
hot-unplug operations are done. At the next reboot the node's memory ranges
can be interleaved and since the call to link_mem_sections() is made in
topology_init() while the system is in the SYSTEM_SCHEDULING state, the
node's id is not checked, and the sections registered to multiple nodes:

$ ls -l /sys/devices/system/memory/memory21/node*
total 0
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node1 -> ../../node/node1
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node2 -> ../../node/node2

In that case, the system is able to boot but if later one of theses memory
block is hot-unplugged and then hot-plugged, the sysfs inconsistency is
detected and triggered a BUG_ON():

------------[ cut here ]------------
kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
NIP:  c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
REGS: c0000004876e3660 TRAP: 0700   Not tainted  (5.9.0-rc1+)
MSR:  800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 24000448  XER: 20040000
CFAR: c000000000846d20 IRQMASK: 0
GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
NIP [c000000000403f34] add_memory_resource+0x244/0x340
LR [c000000000403f2c] add_memory_resource+0x23c/0x340
Call Trace:
[c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
[c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
[c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
[c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
[c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
[c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
[c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
[c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
[c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
[c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
[c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
[c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
[c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
Instruction dump:
48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
---[ end trace 562fd6c109cd0fb2 ]---

This patch addresses the root cause by not relying on the system_state
value to detect whether the call is due to a hot-plug operation. An extra
parameter is added to link_mem_sections() detailing whether the operation
is due to a hot-plug operation.

[1] According to Oscar Salvador, this qemu command line, ACPI memory
hotplug operations are raised at SYSTEM_SCHEDULING state:

$QEMU -enable-kvm -machine pc -smp 4,sockets=4,cores=1,threads=1 -cpu host -monitor pty \
        -m size=$MEM,slots=255,maxmem=4294967296k  \
        -numa node,nodeid=0,cpus=0-3,mem=512 -numa node,nodeid=1,mem=512 \
        -object memory-backend-ram,id=memdimm0,size=134217728 -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \
        -object memory-backend-ram,id=memdimm1,size=134217728 -device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=1 \
        -object memory-backend-ram,id=memdimm2,size=134217728 -device pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2 \
        -object memory-backend-ram,id=memdimm3,size=134217728 -device pc-dimm,node=0,memdev=memdimm3,id=dimm3,slot=3 \
        -object memory-backend-ram,id=memdimm4,size=134217728 -device pc-dimm,node=1,memdev=memdimm4,id=dimm4,slot=4 \
        -object memory-backend-ram,id=memdimm5,size=134217728 -device pc-dimm,node=1,memdev=memdimm5,id=dimm5,slot=5 \
        -object memory-backend-ram,id=memdimm6,size=134217728 -device pc-dimm,node=1,memdev=memdimm6,id=dimm6,slot=6 \

Fixes: 4fbce633910e ("mm/memory_hotplug.c: make register_mem_sect_under_node() a callback of walk_memory_range()")
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Oscar Salvador <osalvador@suse.de>
---
 arch/ia64/mm/init.c  |  4 +--
 drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
 include/linux/node.h | 11 +++---
 mm/memory_hotplug.c  |  5 +--
 4 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index b5054b5e77c8..8e7b8c6c576e 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
 	if (map_start < map_end)
 		memmap_init_zone((unsigned long)(map_end - map_start),
 				 args->nid, args->zone, page_to_pfn(map_start),
-				 MEMPLUG_EARLY, NULL);
+				 MEMINIT_EARLY, NULL);
 	return 0;
 }
 
@@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
 {
 	if (!vmem_map) {
 		memmap_init_zone(size, nid, zone, start_pfn,
-				 MEMPLUG_EARLY, NULL);
+				 MEMINIT_EARLY, NULL);
 	} else {
 		struct page *start;
 		struct memmap_init_callback_data args;
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 508b80f6329b..01ee73c9d675 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	return pfn_to_nid(pfn);
 }
 
+static int do_register_memory_block_under_node(int nid,
+					       struct memory_block *mem_blk)
+{
+	int ret;
+
+	/*
+	 * If this memory block spans multiple nodes, we only indicate
+	 * the last processed node.
+	 */
+	mem_blk->nid = nid;
+
+	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
+				       &mem_blk->dev.kobj,
+				       kobject_name(&mem_blk->dev.kobj));
+	if (ret)
+		return ret;
+
+	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
+				&node_devices[nid]->dev.kobj,
+				kobject_name(&node_devices[nid]->dev.kobj));
+}
+
 /* register memory section under specified node if it spans that node */
-static int register_mem_sect_under_node(struct memory_block *mem_blk,
-					 void *arg)
+static int register_mem_block_under_node_early(struct memory_block *mem_blk,
+					       void *arg)
 {
 	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
 	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
 	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
-	int ret, nid = *(int *)arg;
+	int nid = *(int *)arg;
 	unsigned long pfn;
 
 	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
@@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
 		}
 
 		/*
-		 * We need to check if page belongs to nid only for the boot
-		 * case, during hotplug we know that all pages in the memory
-		 * block belong to the same node.
-		 */
-		if (system_state == SYSTEM_BOOTING) {
-			page_nid = get_nid_for_pfn(pfn);
-			if (page_nid < 0)
-				continue;
-			if (page_nid != nid)
-				continue;
-		}
-
-		/*
-		 * If this memory block spans multiple nodes, we only indicate
-		 * the last processed node.
+		 * We need to check if page belongs to nid only at the boot
+		 * case because node's ranges can be interleaved.
 		 */
-		mem_blk->nid = nid;
-
-		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
-					&mem_blk->dev.kobj,
-					kobject_name(&mem_blk->dev.kobj));
-		if (ret)
-			return ret;
+		page_nid = get_nid_for_pfn(pfn);
+		if (page_nid < 0)
+			continue;
+		if (page_nid != nid)
+			continue;
 
-		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
-				&node_devices[nid]->dev.kobj,
-				kobject_name(&node_devices[nid]->dev.kobj));
+		/* The memory block is registered to the first matching node */
+		return do_register_memory_block_under_node(nid, mem_blk);
 	}
 	/* mem section does not span the specified node */
 	return 0;
 }
 
+/*
+ * During hotplug we know that all pages in the memory block belong to the same
+ * node.
+ */
+static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
+						 void *arg)
+{
+	int nid = *(int *)arg;
+
+	return do_register_memory_block_under_node(nid, mem_blk);
+}
+
 /*
  * Unregister a memory block device under the node it spans. Memory blocks
  * with multiple nodes cannot be offlined and therefore also never be removed.
@@ -832,11 +850,19 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
 			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
 }
 
-int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
+int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
+		      enum meminit_context context)
 {
+	walk_memory_blocks_func_t func;
+
+	if (context == MEMINIT_HOTPLUG)
+		func = register_mem_block_under_node_hotplug;
+	else
+		func = register_mem_block_under_node_early;
+
 	return walk_memory_blocks(PFN_PHYS(start_pfn),
 				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
-				  register_mem_sect_under_node);
+				  func);
 }
 
 #ifdef CONFIG_HUGETLBFS
diff --git a/include/linux/node.h b/include/linux/node.h
index 4866f32a02d8..014ba3ab2efd 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -99,11 +99,13 @@ extern struct node *node_devices[];
 typedef  void (*node_registration_func_t)(struct node *);
 
 #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
-extern int link_mem_sections(int nid, unsigned long start_pfn,
-			     unsigned long end_pfn);
+int link_mem_sections(int nid, unsigned long start_pfn,
+		      unsigned long end_pfn,
+		      enum meminit_context context);
 #else
 static inline int link_mem_sections(int nid, unsigned long start_pfn,
-				    unsigned long end_pfn)
+				    unsigned long end_pfn,
+				    enum meminit_context context)
 {
 	return 0;
 }
@@ -128,7 +130,8 @@ static inline int register_one_node(int nid)
 		if (error)
 			return error;
 		/* link memory sections under this node */
-		error = link_mem_sections(nid, start_pfn, end_pfn);
+		error = link_mem_sections(nid, start_pfn, end_pfn,
+					  MEMINIT_EARLY);
 	}
 
 	return error;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index fc21625e42de..03df20078827 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -729,7 +729,7 @@ void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 	 * are reserved so nobody should be touching them so we should be safe
 	 */
 	memmap_init_zone(nr_pages, nid, zone_idx(zone), start_pfn,
-			 MEMPLUG_HOTPLUG, altmap);
+			 MEMINIT_HOTPLUG, altmap);
 
 	set_zone_contiguous(zone);
 }
@@ -1080,7 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	}
 
 	/* link memory sections under this node.*/
-	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1));
+	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
+				MEMINIT_HOTPLUG);
 	BUG_ON(ret);
 
 	/* create new memmap entry */
-- 
2.28.0

[PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[Laurent Dufour]

In register_mem_sect_under_node() the system_state’s value is checked to
detect whether the operation the call is made during boot time or during an
hot-plug operation. Unfortunately, that check against SYSTEM_BOOTING is
wrong because regular memory is registered at SYSTEM_SCHEDULING state. In
addition memory hot-plug operation can be triggered at this system state
too by the ACPI [1]. So checking against the system state is not enough.

The consequence is that on system with interleaved node's ranges like this:
 Early memory node ranges
   node   1: [mem 0x0000000000000000-0x000000011fffffff]
   node   2: [mem 0x0000000120000000-0x000000014fffffff]
   node   1: [mem 0x0000000150000000-0x00000001ffffffff]
   node   0: [mem 0x0000000200000000-0x000000048fffffff]
   node   2: [mem 0x0000000490000000-0x00000007ffffffff]

This can be seen on PowerPC LPAR after multiple memory hot-plug and
hot-unplug operations are done. At the next reboot the node's memory ranges
can be interleaved and since the call to link_mem_sections() is made in
topology_init() while the system is in the SYSTEM_SCHEDULING state, the
node's id is not checked, and the sections registered to multiple nodes:

$ ls -l /sys/devices/system/memory/memory21/node*
total 0
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node1 -> ../../node/node1
lrwxrwxrwx 1 root root     0 Aug 24 05:27 node2 -> ../../node/node2

In that case, the system is able to boot but if later one of theses memory
block is hot-unplugged and then hot-plugged, the sysfs inconsistency is
detected and triggered a BUG_ON():

------------[ cut here ]------------
kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZEdK MMU=Hash SMP NR_CPUS 48 NUMA pSeries
Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
NIP:  c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
REGS: c0000004876e3660 TRAP: 0700   Not tainted  (5.9.0-rc1+)
MSR:  800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 24000448  XER: 20040000
CFAR: c000000000846d20 IRQMASK: 0
GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
NIP [c000000000403f34] add_memory_resource+0x244/0x340
LR [c000000000403f2c] add_memory_resource+0x23c/0x340
Call Trace:
[c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
[c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
[c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
[c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
[c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
[c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
[c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
[c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
[c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
[c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
[c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
[c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
[c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
Instruction dump:
48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
---[ end trace 562fd6c109cd0fb2 ]---

This patch addresses the root cause by not relying on the system_state
value to detect whether the call is due to a hot-plug operation. An extra
parameter is added to link_mem_sections() detailing whether the operation
is due to a hot-plug operation.

[1] According to Oscar Salvador, this qemu command line, ACPI memory
hotplug operations are raised at SYSTEM_SCHEDULING state:

$QEMU -enable-kvm -machine pc -smp 4,sockets=4,cores=1,threads=1 -cpu host -monitor pty \
        -m size=$MEM,slots%5,maxmemB94967296k  \
        -numa node,nodeid=0,cpus=0-3,memQ2 -numa node,nodeid=1,memQ2 \
        -object memory-backend-ram,id=memdimm0,size\x134217728 -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \
        -object memory-backend-ram,id=memdimm1,size\x134217728 -device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=1 \
        -object memory-backend-ram,id=memdimm2,size\x134217728 -device pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2 \
        -object memory-backend-ram,id=memdimm3,size\x134217728 -device pc-dimm,node=0,memdev=memdimm3,id=dimm3,slot=3 \
        -object memory-backend-ram,id=memdimm4,size\x134217728 -device pc-dimm,node=1,memdev=memdimm4,id=dimm4,slot=4 \
        -object memory-backend-ram,id=memdimm5,size\x134217728 -device pc-dimm,node=1,memdev=memdimm5,id=dimm5,slot=5 \
        -object memory-backend-ram,id=memdimm6,size\x134217728 -device pc-dimm,node=1,memdev=memdimm6,id=dimm6,slot=6 \

Fixes: 4fbce633910e ("mm/memory_hotplug.c: make register_mem_sect_under_node() a callback of walk_memory_range()")
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Oscar Salvador <osalvador@suse.de>
---
 arch/ia64/mm/init.c  |  4 +--
 drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
 include/linux/node.h | 11 +++---
 mm/memory_hotplug.c  |  5 +--
 4 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index b5054b5e77c8..8e7b8c6c576e 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
 	if (map_start < map_end)
 		memmap_init_zone((unsigned long)(map_end - map_start),
 				 args->nid, args->zone, page_to_pfn(map_start),
-				 MEMPLUG_EARLY, NULL);
+				 MEMINIT_EARLY, NULL);
 	return 0;
 }
 
@@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
 {
 	if (!vmem_map) {
 		memmap_init_zone(size, nid, zone, start_pfn,
-				 MEMPLUG_EARLY, NULL);
+				 MEMINIT_EARLY, NULL);
 	} else {
 		struct page *start;
 		struct memmap_init_callback_data args;
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 508b80f6329b..01ee73c9d675 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	return pfn_to_nid(pfn);
 }
 
+static int do_register_memory_block_under_node(int nid,
+					       struct memory_block *mem_blk)
+{
+	int ret;
+
+	/*
+	 * If this memory block spans multiple nodes, we only indicate
+	 * the last processed node.
+	 */
+	mem_blk->nid = nid;
+
+	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
+				       &mem_blk->dev.kobj,
+				       kobject_name(&mem_blk->dev.kobj));
+	if (ret)
+		return ret;
+
+	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
+				&node_devices[nid]->dev.kobj,
+				kobject_name(&node_devices[nid]->dev.kobj));
+}
+
 /* register memory section under specified node if it spans that node */
-static int register_mem_sect_under_node(struct memory_block *mem_blk,
-					 void *arg)
+static int register_mem_block_under_node_early(struct memory_block *mem_blk,
+					       void *arg)
 {
 	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
 	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
 	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
-	int ret, nid = *(int *)arg;
+	int nid = *(int *)arg;
 	unsigned long pfn;
 
 	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
@@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
 		}
 
 		/*
-		 * We need to check if page belongs to nid only for the boot
-		 * case, during hotplug we know that all pages in the memory
-		 * block belong to the same node.
-		 */
-		if (system_state = SYSTEM_BOOTING) {
-			page_nid = get_nid_for_pfn(pfn);
-			if (page_nid < 0)
-				continue;
-			if (page_nid != nid)
-				continue;
-		}
-
-		/*
-		 * If this memory block spans multiple nodes, we only indicate
-		 * the last processed node.
+		 * We need to check if page belongs to nid only at the boot
+		 * case because node's ranges can be interleaved.
 		 */
-		mem_blk->nid = nid;
-
-		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
-					&mem_blk->dev.kobj,
-					kobject_name(&mem_blk->dev.kobj));
-		if (ret)
-			return ret;
+		page_nid = get_nid_for_pfn(pfn);
+		if (page_nid < 0)
+			continue;
+		if (page_nid != nid)
+			continue;
 
-		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
-				&node_devices[nid]->dev.kobj,
-				kobject_name(&node_devices[nid]->dev.kobj));
+		/* The memory block is registered to the first matching node */
+		return do_register_memory_block_under_node(nid, mem_blk);
 	}
 	/* mem section does not span the specified node */
 	return 0;
 }
 
+/*
+ * During hotplug we know that all pages in the memory block belong to the same
+ * node.
+ */
+static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
+						 void *arg)
+{
+	int nid = *(int *)arg;
+
+	return do_register_memory_block_under_node(nid, mem_blk);
+}
+
 /*
  * Unregister a memory block device under the node it spans. Memory blocks
  * with multiple nodes cannot be offlined and therefore also never be removed.
@@ -832,11 +850,19 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
 			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
 }
 
-int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
+int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
+		      enum meminit_context context)
 {
+	walk_memory_blocks_func_t func;
+
+	if (context = MEMINIT_HOTPLUG)
+		func = register_mem_block_under_node_hotplug;
+	else
+		func = register_mem_block_under_node_early;
+
 	return walk_memory_blocks(PFN_PHYS(start_pfn),
 				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
-				  register_mem_sect_under_node);
+				  func);
 }
 
 #ifdef CONFIG_HUGETLBFS
diff --git a/include/linux/node.h b/include/linux/node.h
index 4866f32a02d8..014ba3ab2efd 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -99,11 +99,13 @@ extern struct node *node_devices[];
 typedef  void (*node_registration_func_t)(struct node *);
 
 #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
-extern int link_mem_sections(int nid, unsigned long start_pfn,
-			     unsigned long end_pfn);
+int link_mem_sections(int nid, unsigned long start_pfn,
+		      unsigned long end_pfn,
+		      enum meminit_context context);
 #else
 static inline int link_mem_sections(int nid, unsigned long start_pfn,
-				    unsigned long end_pfn)
+				    unsigned long end_pfn,
+				    enum meminit_context context)
 {
 	return 0;
 }
@@ -128,7 +130,8 @@ static inline int register_one_node(int nid)
 		if (error)
 			return error;
 		/* link memory sections under this node */
-		error = link_mem_sections(nid, start_pfn, end_pfn);
+		error = link_mem_sections(nid, start_pfn, end_pfn,
+					  MEMINIT_EARLY);
 	}
 
 	return error;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index fc21625e42de..03df20078827 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -729,7 +729,7 @@ void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 	 * are reserved so nobody should be touching them so we should be safe
 	 */
 	memmap_init_zone(nr_pages, nid, zone_idx(zone), start_pfn,
-			 MEMPLUG_HOTPLUG, altmap);
+			 MEMINIT_HOTPLUG, altmap);
 
 	set_zone_contiguous(zone);
 }
@@ -1080,7 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	}
 
 	/* link memory sections under this node.*/
-	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1));
+	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
+				MEMINIT_HOTPLUG);
 	BUG_ON(ret);
 
 	/* create new memmap entry */
-- 
2.28.0

[PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[Laurent Dufour]

At boot time, or when doing memory hot-add operations, if the links in
sysfs can't be created, the system is still able to run, so just report the
error in the kernel log rather than BUG_ON and potentially make system
unusable because the callpath can be called with locks held.

Since the number of memory blocks managed could be high, the messages are
rate limited.

As a consequence, link_mem_sections() has no status to report anymore.

Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/node.c  | 33 +++++++++++++++++++++------------
 include/linux/node.h | 16 +++++++---------
 mm/memory_hotplug.c  |  5 ++---
 3 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 01ee73c9d675..249b2ba6dc81 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	return pfn_to_nid(pfn);
 }
 
-static int do_register_memory_block_under_node(int nid,
-					       struct memory_block *mem_blk)
+static void do_register_memory_block_under_node(int nid,
+						struct memory_block *mem_blk)
 {
 	int ret;
 
@@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
 	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
 				       &mem_blk->dev.kobj,
 				       kobject_name(&mem_blk->dev.kobj));
-	if (ret)
-		return ret;
+	if (ret && ret != -EEXIST)
+		dev_err_ratelimited(&node_devices[nid]->dev,
+				    "can't create link to %s in sysfs (%d)\n",
+				    kobject_name(&mem_blk->dev.kobj), ret);
 
-	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
+	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
 				&node_devices[nid]->dev.kobj,
 				kobject_name(&node_devices[nid]->dev.kobj));
+	if (ret && ret != -EEXIST)
+		dev_err_ratelimited(&mem_blk->dev,
+				    "can't create link to %s in sysfs (%d)\n",
+				    kobject_name(&node_devices[nid]->dev.kobj),
+				    ret);
 }
 
 /* register memory section under specified node if it spans that node */
@@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
 			continue;
 
 		/* The memory block is registered to the first matching node */
-		return do_register_memory_block_under_node(nid, mem_blk);
+		do_register_memory_block_under_node(nid, mem_blk);
+		return 0;
 	}
 	/* mem section does not span the specified node */
 	return 0;
@@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
 {
 	int nid = *(int *)arg;
 
-	return do_register_memory_block_under_node(nid, mem_blk);
+	do_register_memory_block_under_node(nid, mem_blk);
+	return 0;
 }
 
 /*
@@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
 			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
 }
 
-int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
-		      enum meminit_context context)
+void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
+		       enum meminit_context context)
 {
 	walk_memory_blocks_func_t func;
 
@@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
 	else
 		func = register_mem_block_under_node_early;
 
-	return walk_memory_blocks(PFN_PHYS(start_pfn),
-				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
-				  func);
+	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
+			   (void *)&nid, func);
+	return;
 }
 
 #ifdef CONFIG_HUGETLBFS
diff --git a/include/linux/node.h b/include/linux/node.h
index 014ba3ab2efd..8e5a29897936 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -99,15 +99,14 @@ extern struct node *node_devices[];
 typedef  void (*node_registration_func_t)(struct node *);
 
 #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
-int link_mem_sections(int nid, unsigned long start_pfn,
-		      unsigned long end_pfn,
-		      enum meminit_context context);
+void link_mem_sections(int nid, unsigned long start_pfn,
+		       unsigned long end_pfn,
+		       enum meminit_context context);
 #else
-static inline int link_mem_sections(int nid, unsigned long start_pfn,
-				    unsigned long end_pfn,
-				    enum meminit_context context)
+static inline void link_mem_sections(int nid, unsigned long start_pfn,
+				     unsigned long end_pfn,
+				     enum meminit_context context)
 {
-	return 0;
 }
 #endif
 
@@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
 		if (error)
 			return error;
 		/* link memory sections under this node */
-		error = link_mem_sections(nid, start_pfn, end_pfn,
-					  MEMINIT_EARLY);
+		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
 	}
 
 	return error;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 03df20078827..01e01a530d38 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	}
 
 	/* link memory sections under this node.*/
-	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
-				MEMINIT_HOTPLUG);
-	BUG_ON(ret);
+	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
+			  MEMINIT_HOTPLUG);
 
 	/* create new memmap entry */
 	if (!strcmp(res->name, "System RAM"))
-- 
2.28.0

[PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[Laurent Dufour]

At boot time, or when doing memory hot-add operations, if the links in
sysfs can't be created, the system is still able to run, so just report the
error in the kernel log rather than BUG_ON and potentially make system
unusable because the callpath can be called with locks held.

Since the number of memory blocks managed could be high, the messages are
rate limited.

As a consequence, link_mem_sections() has no status to report anymore.

Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/node.c  | 33 +++++++++++++++++++++------------
 include/linux/node.h | 16 +++++++---------
 mm/memory_hotplug.c  |  5 ++---
 3 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 01ee73c9d675..249b2ba6dc81 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	return pfn_to_nid(pfn);
 }
 
-static int do_register_memory_block_under_node(int nid,
-					       struct memory_block *mem_blk)
+static void do_register_memory_block_under_node(int nid,
+						struct memory_block *mem_blk)
 {
 	int ret;
 
@@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
 	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
 				       &mem_blk->dev.kobj,
 				       kobject_name(&mem_blk->dev.kobj));
-	if (ret)
-		return ret;
+	if (ret && ret != -EEXIST)
+		dev_err_ratelimited(&node_devices[nid]->dev,
+				    "can't create link to %s in sysfs (%d)\n",
+				    kobject_name(&mem_blk->dev.kobj), ret);
 
-	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
+	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
 				&node_devices[nid]->dev.kobj,
 				kobject_name(&node_devices[nid]->dev.kobj));
+	if (ret && ret != -EEXIST)
+		dev_err_ratelimited(&mem_blk->dev,
+				    "can't create link to %s in sysfs (%d)\n",
+				    kobject_name(&node_devices[nid]->dev.kobj),
+				    ret);
 }
 
 /* register memory section under specified node if it spans that node */
@@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
 			continue;
 
 		/* The memory block is registered to the first matching node */
-		return do_register_memory_block_under_node(nid, mem_blk);
+		do_register_memory_block_under_node(nid, mem_blk);
+		return 0;
 	}
 	/* mem section does not span the specified node */
 	return 0;
@@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
 {
 	int nid = *(int *)arg;
 
-	return do_register_memory_block_under_node(nid, mem_blk);
+	do_register_memory_block_under_node(nid, mem_blk);
+	return 0;
 }
 
 /*
@@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
 			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
 }
 
-int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
-		      enum meminit_context context)
+void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
+		       enum meminit_context context)
 {
 	walk_memory_blocks_func_t func;
 
@@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
 	else
 		func = register_mem_block_under_node_early;
 
-	return walk_memory_blocks(PFN_PHYS(start_pfn),
-				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
-				  func);
+	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
+			   (void *)&nid, func);
+	return;
 }
 
 #ifdef CONFIG_HUGETLBFS
diff --git a/include/linux/node.h b/include/linux/node.h
index 014ba3ab2efd..8e5a29897936 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -99,15 +99,14 @@ extern struct node *node_devices[];
 typedef  void (*node_registration_func_t)(struct node *);
 
 #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
-int link_mem_sections(int nid, unsigned long start_pfn,
-		      unsigned long end_pfn,
-		      enum meminit_context context);
+void link_mem_sections(int nid, unsigned long start_pfn,
+		       unsigned long end_pfn,
+		       enum meminit_context context);
 #else
-static inline int link_mem_sections(int nid, unsigned long start_pfn,
-				    unsigned long end_pfn,
-				    enum meminit_context context)
+static inline void link_mem_sections(int nid, unsigned long start_pfn,
+				     unsigned long end_pfn,
+				     enum meminit_context context)
 {
-	return 0;
 }
 #endif
 
@@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
 		if (error)
 			return error;
 		/* link memory sections under this node */
-		error = link_mem_sections(nid, start_pfn, end_pfn,
-					  MEMINIT_EARLY);
+		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
 	}
 
 	return error;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 03df20078827..01e01a530d38 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	}
 
 	/* link memory sections under this node.*/
-	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
-				MEMINIT_HOTPLUG);
-	BUG_ON(ret);
+	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
+			  MEMINIT_HOTPLUG);
 
 	/* create new memmap entry */
 	if (!strcmp(res->name, "System RAM"))
-- 
2.28.0

Re: [PATCH v2 1/3] mm: replace memmap_context by memplug_context

[David Hildenbrand]

On 14.09.20 18:50, Laurent Dufour wrote:
> The memmap_context is used to detect whether a memory operation is due to a
> hot-add operation or happening at boot time.
> 
> Make it general to the hotplug operation and rename it at memplug_context.
> 
> There is no functional change introduced by this patch
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/ia64/mm/init.c    |  6 +++---
>  include/linux/mm.h     |  2 +-
>  include/linux/mmzone.h | 11 ++++++++---
>  mm/memory_hotplug.c    |  2 +-
>  mm/page_alloc.c        | 10 +++++-----
>  5 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index 0b3fb4c7af29..b5054b5e77c8 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>  	if (map_start < map_end)
>  		memmap_init_zone((unsigned long)(map_end - map_start),
>  				 args->nid, args->zone, page_to_pfn(map_start),
> -				 MEMMAP_EARLY, NULL);
> +				 MEMPLUG_EARLY, NULL);

I am pretty sure that won't compile (MEMINIT_EARLY).

(same at other places)


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: replace memmap_context by memplug_context

[David Hildenbrand]

On 14.09.20 18:50, Laurent Dufour wrote:
> The memmap_context is used to detect whether a memory operation is due to a
> hot-add operation or happening at boot time.
> 
> Make it general to the hotplug operation and rename it at memplug_context.
> 
> There is no functional change introduced by this patch
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/ia64/mm/init.c    |  6 +++---
>  include/linux/mm.h     |  2 +-
>  include/linux/mmzone.h | 11 ++++++++---
>  mm/memory_hotplug.c    |  2 +-
>  mm/page_alloc.c        | 10 +++++-----
>  5 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index 0b3fb4c7af29..b5054b5e77c8 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>  	if (map_start < map_end)
>  		memmap_init_zone((unsigned long)(map_end - map_start),
>  				 args->nid, args->zone, page_to_pfn(map_start),
> -				 MEMMAP_EARLY, NULL);
> +				 MEMPLUG_EARLY, NULL);

I am pretty sure that won't compile (MEMINIT_EARLY).

(same at other places)


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[David Hildenbrand]

>  arch/ia64/mm/init.c  |  4 +--
>  drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
>  include/linux/node.h | 11 +++---
>  mm/memory_hotplug.c  |  5 +--
>  4 files changed, 68 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index b5054b5e77c8..8e7b8c6c576e 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>  	if (map_start < map_end)
>  		memmap_init_zone((unsigned long)(map_end - map_start),
>  				 args->nid, args->zone, page_to_pfn(map_start),
> -				 MEMPLUG_EARLY, NULL);
> +				 MEMINIT_EARLY, NULL);

Patch #1.

>  	return 0;
>  }
>  
> @@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
>  {
>  	if (!vmem_map) {
>  		memmap_init_zone(size, nid, zone, start_pfn,
> -				 MEMPLUG_EARLY, NULL);
> +				 MEMINIT_EARLY, NULL);
>  	} else {
>  		struct page *start;
>  		struct memmap_init_callback_data args;
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 508b80f6329b..01ee73c9d675 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>  	return pfn_to_nid(pfn);
>  }
>  
> +static int do_register_memory_block_under_node(int nid,
> +					       struct memory_block *mem_blk)
> +{
> +	int ret;
> +
> +	/*
> +	 * If this memory block spans multiple nodes, we only indicate
> +	 * the last processed node.
> +	 */
> +	mem_blk->nid = nid;
> +
> +	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
> +				       &mem_blk->dev.kobj,
> +				       kobject_name(&mem_blk->dev.kobj));
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> +				&node_devices[nid]->dev.kobj,
> +				kobject_name(&node_devices[nid]->dev.kobj));
> +}
> +
>  /* register memory section under specified node if it spans that node */
> -static int register_mem_sect_under_node(struct memory_block *mem_blk,
> -					 void *arg)
> +static int register_mem_block_under_node_early(struct memory_block *mem_blk,
> +					       void *arg)
>  {
>  	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
>  	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
>  	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
> -	int ret, nid = *(int *)arg;
> +	int nid = *(int *)arg;
>  	unsigned long pfn;
>  
>  	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
> @@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
>  		}
>  
>  		/*
> -		 * We need to check if page belongs to nid only for the boot
> -		 * case, during hotplug we know that all pages in the memory
> -		 * block belong to the same node.
> -		 */
> -		if (system_state == SYSTEM_BOOTING) {
> -			page_nid = get_nid_for_pfn(pfn);
> -			if (page_nid < 0)
> -				continue;
> -			if (page_nid != nid)
> -				continue;
> -		}
> -
> -		/*
> -		 * If this memory block spans multiple nodes, we only indicate
> -		 * the last processed node.
> +		 * We need to check if page belongs to nid only at the boot
> +		 * case because node's ranges can be interleaved.
>  		 */
> -		mem_blk->nid = nid;
> -
> -		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
> -					&mem_blk->dev.kobj,
> -					kobject_name(&mem_blk->dev.kobj));
> -		if (ret)
> -			return ret;
> +		page_nid = get_nid_for_pfn(pfn);
> +		if (page_nid < 0)
> +			continue;
> +		if (page_nid != nid)
> +			continue;
>  
> -		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> -				&node_devices[nid]->dev.kobj,
> -				kobject_name(&node_devices[nid]->dev.kobj));
> +		/* The memory block is registered to the first matching node */

That comment is misleading in that context.

A memory block is registered if there is at least a page that belongs to
the nid. It's perfectly fine to have a single memory block belong to
multiple NUMA nodes (when the split is within a memory block). I'd just
drop it.

[...]

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[David Hildenbrand]

>  arch/ia64/mm/init.c  |  4 +--
>  drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
>  include/linux/node.h | 11 +++---
>  mm/memory_hotplug.c  |  5 +--
>  4 files changed, 68 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index b5054b5e77c8..8e7b8c6c576e 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>  	if (map_start < map_end)
>  		memmap_init_zone((unsigned long)(map_end - map_start),
>  				 args->nid, args->zone, page_to_pfn(map_start),
> -				 MEMPLUG_EARLY, NULL);
> +				 MEMINIT_EARLY, NULL);

Patch #1.

>  	return 0;
>  }
>  
> @@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
>  {
>  	if (!vmem_map) {
>  		memmap_init_zone(size, nid, zone, start_pfn,
> -				 MEMPLUG_EARLY, NULL);
> +				 MEMINIT_EARLY, NULL);
>  	} else {
>  		struct page *start;
>  		struct memmap_init_callback_data args;
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 508b80f6329b..01ee73c9d675 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>  	return pfn_to_nid(pfn);
>  }
>  
> +static int do_register_memory_block_under_node(int nid,
> +					       struct memory_block *mem_blk)
> +{
> +	int ret;
> +
> +	/*
> +	 * If this memory block spans multiple nodes, we only indicate
> +	 * the last processed node.
> +	 */
> +	mem_blk->nid = nid;
> +
> +	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
> +				       &mem_blk->dev.kobj,
> +				       kobject_name(&mem_blk->dev.kobj));
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> +				&node_devices[nid]->dev.kobj,
> +				kobject_name(&node_devices[nid]->dev.kobj));
> +}
> +
>  /* register memory section under specified node if it spans that node */
> -static int register_mem_sect_under_node(struct memory_block *mem_blk,
> -					 void *arg)
> +static int register_mem_block_under_node_early(struct memory_block *mem_blk,
> +					       void *arg)
>  {
>  	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
>  	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
>  	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
> -	int ret, nid = *(int *)arg;
> +	int nid = *(int *)arg;
>  	unsigned long pfn;
>  
>  	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
> @@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
>  		}
>  
>  		/*
> -		 * We need to check if page belongs to nid only for the boot
> -		 * case, during hotplug we know that all pages in the memory
> -		 * block belong to the same node.
> -		 */
> -		if (system_state = SYSTEM_BOOTING) {
> -			page_nid = get_nid_for_pfn(pfn);
> -			if (page_nid < 0)
> -				continue;
> -			if (page_nid != nid)
> -				continue;
> -		}
> -
> -		/*
> -		 * If this memory block spans multiple nodes, we only indicate
> -		 * the last processed node.
> +		 * We need to check if page belongs to nid only at the boot
> +		 * case because node's ranges can be interleaved.
>  		 */
> -		mem_blk->nid = nid;
> -
> -		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
> -					&mem_blk->dev.kobj,
> -					kobject_name(&mem_blk->dev.kobj));
> -		if (ret)
> -			return ret;
> +		page_nid = get_nid_for_pfn(pfn);
> +		if (page_nid < 0)
> +			continue;
> +		if (page_nid != nid)
> +			continue;
>  
> -		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> -				&node_devices[nid]->dev.kobj,
> -				kobject_name(&node_devices[nid]->dev.kobj));
> +		/* The memory block is registered to the first matching node */

That comment is misleading in that context.

A memory block is registered if there is at least a page that belongs to
the nid. It's perfectly fine to have a single memory block belong to
multiple NUMA nodes (when the split is within a memory block). I'd just
drop it.

[...]

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: replace memmap_context by memplug_context

[Laurent Dufour]

Le 14/09/2020 à 19:10, David Hildenbrand a écrit :
> On 14.09.20 18:50, Laurent Dufour wrote:
>> The memmap_context is used to detect whether a memory operation is due to a
>> hot-add operation or happening at boot time.
>>
>> Make it general to the hotplug operation and rename it at memplug_context.
>>
>> There is no functional change introduced by this patch
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/ia64/mm/init.c    |  6 +++---
>>   include/linux/mm.h     |  2 +-
>>   include/linux/mmzone.h | 11 ++++++++---
>>   mm/memory_hotplug.c    |  2 +-
>>   mm/page_alloc.c        | 10 +++++-----
>>   5 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index 0b3fb4c7af29..b5054b5e77c8 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>>   	if (map_start < map_end)
>>   		memmap_init_zone((unsigned long)(map_end - map_start),
>>   				 args->nid, args->zone, page_to_pfn(map_start),
>> -				 MEMMAP_EARLY, NULL);
>> +				 MEMPLUG_EARLY, NULL);
> 
> I am pretty sure that won't compile (MEMINIT_EARLY).
> 
> (same at other places)

Ouch I can't understand what I did at sending time, I did check compile on ia64 
w and w/o memory hotplug.

Sorry for the noise.

Re: [PATCH v2 1/3] mm: replace memmap_context by memplug_context

[Laurent Dufour]

Le 14/09/2020 à 19:10, David Hildenbrand a écrit :
> On 14.09.20 18:50, Laurent Dufour wrote:
>> The memmap_context is used to detect whether a memory operation is due to a
>> hot-add operation or happening at boot time.
>>
>> Make it general to the hotplug operation and rename it at memplug_context.
>>
>> There is no functional change introduced by this patch
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>   arch/ia64/mm/init.c    |  6 +++---
>>   include/linux/mm.h     |  2 +-
>>   include/linux/mmzone.h | 11 ++++++++---
>>   mm/memory_hotplug.c    |  2 +-
>>   mm/page_alloc.c        | 10 +++++-----
>>   5 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index 0b3fb4c7af29..b5054b5e77c8 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>>   	if (map_start < map_end)
>>   		memmap_init_zone((unsigned long)(map_end - map_start),
>>   				 args->nid, args->zone, page_to_pfn(map_start),
>> -				 MEMMAP_EARLY, NULL);
>> +				 MEMPLUG_EARLY, NULL);
> 
> I am pretty sure that won't compile (MEMINIT_EARLY).
> 
> (same at other places)

Ouch I can't understand what I did at sending time, I did check compile on ia64 
w and w/o memory hotplug.

Sorry for the noise.

Re: [PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[Laurent Dufour]

Le 14/09/2020 à 19:15, David Hildenbrand a écrit :
>>   arch/ia64/mm/init.c  |  4 +--
>>   drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
>>   include/linux/node.h | 11 +++---
>>   mm/memory_hotplug.c  |  5 +--
>>   4 files changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index b5054b5e77c8..8e7b8c6c576e 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>>   	if (map_start < map_end)
>>   		memmap_init_zone((unsigned long)(map_end - map_start),
>>   				 args->nid, args->zone, page_to_pfn(map_start),
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);
> 
> Patch #1.

Sure, this explains why I was able to build on ia64 but that's not the right place.

>>   	return 0;
>>   }
>>   
>> @@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
>>   {
>>   	if (!vmem_map) {
>>   		memmap_init_zone(size, nid, zone, start_pfn,
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);

I'll fix that too.

>>   	} else {
>>   		struct page *start;
>>   		struct memmap_init_callback_data args;
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 508b80f6329b..01ee73c9d675 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>>   	return pfn_to_nid(pfn);
>>   }
>>   
>> +static int do_register_memory_block_under_node(int nid,
>> +					       struct memory_block *mem_blk)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * If this memory block spans multiple nodes, we only indicate
>> +	 * the last processed node.
>> +	 */
>> +	mem_blk->nid = nid;
>> +
>> +	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> +				       &mem_blk->dev.kobj,
>> +				       kobject_name(&mem_blk->dev.kobj));
>> +	if (ret)
>> +		return ret;
>> +
>> +	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> +				&node_devices[nid]->dev.kobj,
>> +				kobject_name(&node_devices[nid]->dev.kobj));
>> +}
>> +
>>   /* register memory section under specified node if it spans that node */
>> -static int register_mem_sect_under_node(struct memory_block *mem_blk,
>> -					 void *arg)
>> +static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>> +					       void *arg)
>>   {
>>   	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
>>   	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
>>   	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
>> -	int ret, nid = *(int *)arg;
>> +	int nid = *(int *)arg;
>>   	unsigned long pfn;
>>   
>>   	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
>> @@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
>>   		}
>>   
>>   		/*
>> -		 * We need to check if page belongs to nid only for the boot
>> -		 * case, during hotplug we know that all pages in the memory
>> -		 * block belong to the same node.
>> -		 */
>> -		if (system_state == SYSTEM_BOOTING) {
>> -			page_nid = get_nid_for_pfn(pfn);
>> -			if (page_nid < 0)
>> -				continue;
>> -			if (page_nid != nid)
>> -				continue;
>> -		}
>> -
>> -		/*
>> -		 * If this memory block spans multiple nodes, we only indicate
>> -		 * the last processed node.
>> +		 * We need to check if page belongs to nid only at the boot
>> +		 * case because node's ranges can be interleaved.
>>   		 */
>> -		mem_blk->nid = nid;
>> -
>> -		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> -					&mem_blk->dev.kobj,
>> -					kobject_name(&mem_blk->dev.kobj));
>> -		if (ret)
>> -			return ret;
>> +		page_nid = get_nid_for_pfn(pfn);
>> +		if (page_nid < 0)
>> +			continue;
>> +		if (page_nid != nid)
>> +			continue;
>>   
>> -		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> -				&node_devices[nid]->dev.kobj,
>> -				kobject_name(&node_devices[nid]->dev.kobj));
>> +		/* The memory block is registered to the first matching node */
> 
> That comment is misleading in that context.
> 
> A memory block is registered if there is at least a page that belongs to
> the nid. It's perfectly fine to have a single memory block belong to
> multiple NUMA nodes (when the split is within a memory block). I'd just
> drop it.

I agree the comment is not accurate, I'll drop it.

Re: [PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[Laurent Dufour]

Le 14/09/2020 à 19:15, David Hildenbrand a écrit :
>>   arch/ia64/mm/init.c  |  4 +--
>>   drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
>>   include/linux/node.h | 11 +++---
>>   mm/memory_hotplug.c  |  5 +--
>>   4 files changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index b5054b5e77c8..8e7b8c6c576e 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>>   	if (map_start < map_end)
>>   		memmap_init_zone((unsigned long)(map_end - map_start),
>>   				 args->nid, args->zone, page_to_pfn(map_start),
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);
> 
> Patch #1.

Sure, this explains why I was able to build on ia64 but that's not the right place.

>>   	return 0;
>>   }
>>   
>> @@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
>>   {
>>   	if (!vmem_map) {
>>   		memmap_init_zone(size, nid, zone, start_pfn,
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);

I'll fix that too.

>>   	} else {
>>   		struct page *start;
>>   		struct memmap_init_callback_data args;
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 508b80f6329b..01ee73c9d675 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>>   	return pfn_to_nid(pfn);
>>   }
>>   
>> +static int do_register_memory_block_under_node(int nid,
>> +					       struct memory_block *mem_blk)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * If this memory block spans multiple nodes, we only indicate
>> +	 * the last processed node.
>> +	 */
>> +	mem_blk->nid = nid;
>> +
>> +	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> +				       &mem_blk->dev.kobj,
>> +				       kobject_name(&mem_blk->dev.kobj));
>> +	if (ret)
>> +		return ret;
>> +
>> +	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> +				&node_devices[nid]->dev.kobj,
>> +				kobject_name(&node_devices[nid]->dev.kobj));
>> +}
>> +
>>   /* register memory section under specified node if it spans that node */
>> -static int register_mem_sect_under_node(struct memory_block *mem_blk,
>> -					 void *arg)
>> +static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>> +					       void *arg)
>>   {
>>   	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
>>   	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
>>   	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
>> -	int ret, nid = *(int *)arg;
>> +	int nid = *(int *)arg;
>>   	unsigned long pfn;
>>   
>>   	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
>> @@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
>>   		}
>>   
>>   		/*
>> -		 * We need to check if page belongs to nid only for the boot
>> -		 * case, during hotplug we know that all pages in the memory
>> -		 * block belong to the same node.
>> -		 */
>> -		if (system_state = SYSTEM_BOOTING) {
>> -			page_nid = get_nid_for_pfn(pfn);
>> -			if (page_nid < 0)
>> -				continue;
>> -			if (page_nid != nid)
>> -				continue;
>> -		}
>> -
>> -		/*
>> -		 * If this memory block spans multiple nodes, we only indicate
>> -		 * the last processed node.
>> +		 * We need to check if page belongs to nid only at the boot
>> +		 * case because node's ranges can be interleaved.
>>   		 */
>> -		mem_blk->nid = nid;
>> -
>> -		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> -					&mem_blk->dev.kobj,
>> -					kobject_name(&mem_blk->dev.kobj));
>> -		if (ret)
>> -			return ret;
>> +		page_nid = get_nid_for_pfn(pfn);
>> +		if (page_nid < 0)
>> +			continue;
>> +		if (page_nid != nid)
>> +			continue;
>>   
>> -		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> -				&node_devices[nid]->dev.kobj,
>> -				kobject_name(&node_devices[nid]->dev.kobj));
>> +		/* The memory block is registered to the first matching node */
> 
> That comment is misleading in that context.
> 
> A memory block is registered if there is at least a page that belongs to
> the nid. It's perfectly fine to have a single memory block belong to
> multiple NUMA nodes (when the split is within a memory block). I'd just
> drop it.

I agree the comment is not accurate, I'll drop it.

Re: [PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[David Hildenbrand]

On 14.09.20 18:50, Laurent Dufour wrote:
> At boot time, or when doing memory hot-add operations, if the links in
> sysfs can't be created, the system is still able to run, so just report the
> error in the kernel log rather than BUG_ON and potentially make system
> unusable because the callpath can be called with locks held.
> 
> Since the number of memory blocks managed could be high, the messages are
> rate limited.
> 
> As a consequence, link_mem_sections() has no status to report anymore.
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/node.c  | 33 +++++++++++++++++++++------------
>  include/linux/node.h | 16 +++++++---------
>  mm/memory_hotplug.c  |  5 ++---
>  3 files changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 01ee73c9d675..249b2ba6dc81 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>  	return pfn_to_nid(pfn);
>  }
>  
> -static int do_register_memory_block_under_node(int nid,
> -					       struct memory_block *mem_blk)
> +static void do_register_memory_block_under_node(int nid,
> +						struct memory_block *mem_blk)
>  {
>  	int ret;
>  
> @@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
>  	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>  				       &mem_blk->dev.kobj,
>  				       kobject_name(&mem_blk->dev.kobj));
> -	if (ret)
> -		return ret;
> +	if (ret && ret != -EEXIST)
> +		dev_err_ratelimited(&node_devices[nid]->dev,
> +				    "can't create link to %s in sysfs (%d)\n",
> +				    kobject_name(&mem_blk->dev.kobj), ret);
>  
> -	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> +	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>  				&node_devices[nid]->dev.kobj,
>  				kobject_name(&node_devices[nid]->dev.kobj));
> +	if (ret && ret != -EEXIST)
> +		dev_err_ratelimited(&mem_blk->dev,
> +				    "can't create link to %s in sysfs (%d)\n",
> +				    kobject_name(&node_devices[nid]->dev.kobj),
> +				    ret);
>  }
>  
>  /* register memory section under specified node if it spans that node */
> @@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>  			continue;
>  
>  		/* The memory block is registered to the first matching node */
> -		return do_register_memory_block_under_node(nid, mem_blk);
> +		do_register_memory_block_under_node(nid, mem_blk);
> +		return 0;
>  	}
>  	/* mem section does not span the specified node */
>  	return 0;
> @@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
>  {
>  	int nid = *(int *)arg;
>  
> -	return do_register_memory_block_under_node(nid, mem_blk);
> +	do_register_memory_block_under_node(nid, mem_blk);
> +	return 0;
>  }
>  
>  /*
> @@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
>  			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
>  }
>  
> -int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
> -		      enum meminit_context context)
> +void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
> +		       enum meminit_context context)
>  {
>  	walk_memory_blocks_func_t func;
>  
> @@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>  	else
>  		func = register_mem_block_under_node_early;
>  
> -	return walk_memory_blocks(PFN_PHYS(start_pfn),
> -				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
> -				  func);
> +	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
> +			   (void *)&nid, func);
> +	return;
>  }
>  
>  #ifdef CONFIG_HUGETLBFS
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 014ba3ab2efd..8e5a29897936 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -99,15 +99,14 @@ extern struct node *node_devices[];
>  typedef  void (*node_registration_func_t)(struct node *);
>  
>  #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
> -int link_mem_sections(int nid, unsigned long start_pfn,
> -		      unsigned long end_pfn,
> -		      enum meminit_context context);
> +void link_mem_sections(int nid, unsigned long start_pfn,
> +		       unsigned long end_pfn,
> +		       enum meminit_context context);
>  #else
> -static inline int link_mem_sections(int nid, unsigned long start_pfn,
> -				    unsigned long end_pfn,
> -				    enum meminit_context context)
> +static inline void link_mem_sections(int nid, unsigned long start_pfn,
> +				     unsigned long end_pfn,
> +				     enum meminit_context context)
>  {
> -	return 0;
>  }
>  #endif
>  
> @@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
>  		if (error)
>  			return error;
>  		/* link memory sections under this node */
> -		error = link_mem_sections(nid, start_pfn, end_pfn,
> -					  MEMINIT_EARLY);
> +		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
>  	}
>  
>  	return error;
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 03df20078827..01e01a530d38 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
>  	}
>  
>  	/* link memory sections under this node.*/
> -	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
> -				MEMINIT_HOTPLUG);
> -	BUG_ON(ret);
> +	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
> +			  MEMINIT_HOTPLUG);
>  
>  	/* create new memmap entry */
>  	if (!strcmp(res->name, "System RAM"))
> 

I just remember that I still have some cleanup patches lying around that
rework the whole node onlining on the add_memory() path, being able to
fail in a nice way rather than ignoring errors. Anyhow, this is good
enough for now

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[David Hildenbrand]

On 14.09.20 18:50, Laurent Dufour wrote:
> At boot time, or when doing memory hot-add operations, if the links in
> sysfs can't be created, the system is still able to run, so just report the
> error in the kernel log rather than BUG_ON and potentially make system
> unusable because the callpath can be called with locks held.
> 
> Since the number of memory blocks managed could be high, the messages are
> rate limited.
> 
> As a consequence, link_mem_sections() has no status to report anymore.
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/node.c  | 33 +++++++++++++++++++++------------
>  include/linux/node.h | 16 +++++++---------
>  mm/memory_hotplug.c  |  5 ++---
>  3 files changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 01ee73c9d675..249b2ba6dc81 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>  	return pfn_to_nid(pfn);
>  }
>  
> -static int do_register_memory_block_under_node(int nid,
> -					       struct memory_block *mem_blk)
> +static void do_register_memory_block_under_node(int nid,
> +						struct memory_block *mem_blk)
>  {
>  	int ret;
>  
> @@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
>  	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>  				       &mem_blk->dev.kobj,
>  				       kobject_name(&mem_blk->dev.kobj));
> -	if (ret)
> -		return ret;
> +	if (ret && ret != -EEXIST)
> +		dev_err_ratelimited(&node_devices[nid]->dev,
> +				    "can't create link to %s in sysfs (%d)\n",
> +				    kobject_name(&mem_blk->dev.kobj), ret);
>  
> -	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
> +	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>  				&node_devices[nid]->dev.kobj,
>  				kobject_name(&node_devices[nid]->dev.kobj));
> +	if (ret && ret != -EEXIST)
> +		dev_err_ratelimited(&mem_blk->dev,
> +				    "can't create link to %s in sysfs (%d)\n",
> +				    kobject_name(&node_devices[nid]->dev.kobj),
> +				    ret);
>  }
>  
>  /* register memory section under specified node if it spans that node */
> @@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>  			continue;
>  
>  		/* The memory block is registered to the first matching node */
> -		return do_register_memory_block_under_node(nid, mem_blk);
> +		do_register_memory_block_under_node(nid, mem_blk);
> +		return 0;
>  	}
>  	/* mem section does not span the specified node */
>  	return 0;
> @@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
>  {
>  	int nid = *(int *)arg;
>  
> -	return do_register_memory_block_under_node(nid, mem_blk);
> +	do_register_memory_block_under_node(nid, mem_blk);
> +	return 0;
>  }
>  
>  /*
> @@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
>  			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
>  }
>  
> -int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
> -		      enum meminit_context context)
> +void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
> +		       enum meminit_context context)
>  {
>  	walk_memory_blocks_func_t func;
>  
> @@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>  	else
>  		func = register_mem_block_under_node_early;
>  
> -	return walk_memory_blocks(PFN_PHYS(start_pfn),
> -				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
> -				  func);
> +	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
> +			   (void *)&nid, func);
> +	return;
>  }
>  
>  #ifdef CONFIG_HUGETLBFS
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 014ba3ab2efd..8e5a29897936 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -99,15 +99,14 @@ extern struct node *node_devices[];
>  typedef  void (*node_registration_func_t)(struct node *);
>  
>  #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
> -int link_mem_sections(int nid, unsigned long start_pfn,
> -		      unsigned long end_pfn,
> -		      enum meminit_context context);
> +void link_mem_sections(int nid, unsigned long start_pfn,
> +		       unsigned long end_pfn,
> +		       enum meminit_context context);
>  #else
> -static inline int link_mem_sections(int nid, unsigned long start_pfn,
> -				    unsigned long end_pfn,
> -				    enum meminit_context context)
> +static inline void link_mem_sections(int nid, unsigned long start_pfn,
> +				     unsigned long end_pfn,
> +				     enum meminit_context context)
>  {
> -	return 0;
>  }
>  #endif
>  
> @@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
>  		if (error)
>  			return error;
>  		/* link memory sections under this node */
> -		error = link_mem_sections(nid, start_pfn, end_pfn,
> -					  MEMINIT_EARLY);
> +		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
>  	}
>  
>  	return error;
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 03df20078827..01e01a530d38 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
>  	}
>  
>  	/* link memory sections under this node.*/
> -	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
> -				MEMINIT_HOTPLUG);
> -	BUG_ON(ret);
> +	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
> +			  MEMINIT_HOTPLUG);
>  
>  	/* create new memmap entry */
>  	if (!strcmp(res->name, "System RAM"))
> 

I just remember that I still have some cleanup patches lying around that
rework the whole node onlining on the add_memory() path, being able to
fail in a nice way rather than ignoring errors. Anyhow, this is good
enough for now

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[Laurent Dufour]

Le 15/09/2020 à 09:23, David Hildenbrand a écrit :
> On 14.09.20 18:50, Laurent Dufour wrote:
>> At boot time, or when doing memory hot-add operations, if the links in
>> sysfs can't be created, the system is still able to run, so just report the
>> error in the kernel log rather than BUG_ON and potentially make system
>> unusable because the callpath can be called with locks held.
>>
>> Since the number of memory blocks managed could be high, the messages are
>> rate limited.
>>
>> As a consequence, link_mem_sections() has no status to report anymore.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/base/node.c  | 33 +++++++++++++++++++++------------
>>   include/linux/node.h | 16 +++++++---------
>>   mm/memory_hotplug.c  |  5 ++---
>>   3 files changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 01ee73c9d675..249b2ba6dc81 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>>   	return pfn_to_nid(pfn);
>>   }
>>   
>> -static int do_register_memory_block_under_node(int nid,
>> -					       struct memory_block *mem_blk)
>> +static void do_register_memory_block_under_node(int nid,
>> +						struct memory_block *mem_blk)
>>   {
>>   	int ret;
>>   
>> @@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
>>   	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>>   				       &mem_blk->dev.kobj,
>>   				       kobject_name(&mem_blk->dev.kobj));
>> -	if (ret)
>> -		return ret;
>> +	if (ret && ret != -EEXIST)
>> +		dev_err_ratelimited(&node_devices[nid]->dev,
>> +				    "can't create link to %s in sysfs (%d)\n",
>> +				    kobject_name(&mem_blk->dev.kobj), ret);
>>   
>> -	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> +	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>>   				&node_devices[nid]->dev.kobj,
>>   				kobject_name(&node_devices[nid]->dev.kobj));
>> +	if (ret && ret != -EEXIST)
>> +		dev_err_ratelimited(&mem_blk->dev,
>> +				    "can't create link to %s in sysfs (%d)\n",
>> +				    kobject_name(&node_devices[nid]->dev.kobj),
>> +				    ret);
>>   }
>>   
>>   /* register memory section under specified node if it spans that node */
>> @@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>>   			continue;
>>   
>>   		/* The memory block is registered to the first matching node */
>> -		return do_register_memory_block_under_node(nid, mem_blk);
>> +		do_register_memory_block_under_node(nid, mem_blk);
>> +		return 0;
>>   	}
>>   	/* mem section does not span the specified node */
>>   	return 0;
>> @@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
>>   {
>>   	int nid = *(int *)arg;
>>   
>> -	return do_register_memory_block_under_node(nid, mem_blk);
>> +	do_register_memory_block_under_node(nid, mem_blk);
>> +	return 0;
>>   }
>>   
>>   /*
>> @@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
>>   			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
>>   }
>>   
>> -int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>> -		      enum meminit_context context)
>> +void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>> +		       enum meminit_context context)
>>   {
>>   	walk_memory_blocks_func_t func;
>>   
>> @@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>>   	else
>>   		func = register_mem_block_under_node_early;
>>   
>> -	return walk_memory_blocks(PFN_PHYS(start_pfn),
>> -				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
>> -				  func);
>> +	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
>> +			   (void *)&nid, func);
>> +	return;
>>   }
>>   
>>   #ifdef CONFIG_HUGETLBFS
>> diff --git a/include/linux/node.h b/include/linux/node.h
>> index 014ba3ab2efd..8e5a29897936 100644
>> --- a/include/linux/node.h
>> +++ b/include/linux/node.h
>> @@ -99,15 +99,14 @@ extern struct node *node_devices[];
>>   typedef  void (*node_registration_func_t)(struct node *);
>>   
>>   #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
>> -int link_mem_sections(int nid, unsigned long start_pfn,
>> -		      unsigned long end_pfn,
>> -		      enum meminit_context context);
>> +void link_mem_sections(int nid, unsigned long start_pfn,
>> +		       unsigned long end_pfn,
>> +		       enum meminit_context context);
>>   #else
>> -static inline int link_mem_sections(int nid, unsigned long start_pfn,
>> -				    unsigned long end_pfn,
>> -				    enum meminit_context context)
>> +static inline void link_mem_sections(int nid, unsigned long start_pfn,
>> +				     unsigned long end_pfn,
>> +				     enum meminit_context context)
>>   {
>> -	return 0;
>>   }
>>   #endif
>>   
>> @@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
>>   		if (error)
>>   			return error;
>>   		/* link memory sections under this node */
>> -		error = link_mem_sections(nid, start_pfn, end_pfn,
>> -					  MEMINIT_EARLY);
>> +		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
>>   	}
>>   
>>   	return error;
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 03df20078827..01e01a530d38 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
>>   	}
>>   
>>   	/* link memory sections under this node.*/
>> -	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
>> -				MEMINIT_HOTPLUG);
>> -	BUG_ON(ret);
>> +	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
>> +			  MEMINIT_HOTPLUG);
>>   
>>   	/* create new memmap entry */
>>   	if (!strcmp(res->name, "System RAM"))
>>
> 
> I just remember that I still have some cleanup patches lying around that
> rework the whole node onlining on the add_memory() path, being able to
> fail in a nice way rather than ignoring errors. Anyhow, this is good
> enough for now
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks David.

Re: [PATCH v2 3/3] mm: don't panic when links can't be created in sysfs

[Laurent Dufour]

Le 15/09/2020 à 09:23, David Hildenbrand a écrit :
> On 14.09.20 18:50, Laurent Dufour wrote:
>> At boot time, or when doing memory hot-add operations, if the links in
>> sysfs can't be created, the system is still able to run, so just report the
>> error in the kernel log rather than BUG_ON and potentially make system
>> unusable because the callpath can be called with locks held.
>>
>> Since the number of memory blocks managed could be high, the messages are
>> rate limited.
>>
>> As a consequence, link_mem_sections() has no status to report anymore.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/base/node.c  | 33 +++++++++++++++++++++------------
>>   include/linux/node.h | 16 +++++++---------
>>   mm/memory_hotplug.c  |  5 ++---
>>   3 files changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 01ee73c9d675..249b2ba6dc81 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -761,8 +761,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>>   	return pfn_to_nid(pfn);
>>   }
>>   
>> -static int do_register_memory_block_under_node(int nid,
>> -					       struct memory_block *mem_blk)
>> +static void do_register_memory_block_under_node(int nid,
>> +						struct memory_block *mem_blk)
>>   {
>>   	int ret;
>>   
>> @@ -775,12 +775,19 @@ static int do_register_memory_block_under_node(int nid,
>>   	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>>   				       &mem_blk->dev.kobj,
>>   				       kobject_name(&mem_blk->dev.kobj));
>> -	if (ret)
>> -		return ret;
>> +	if (ret && ret != -EEXIST)
>> +		dev_err_ratelimited(&node_devices[nid]->dev,
>> +				    "can't create link to %s in sysfs (%d)\n",
>> +				    kobject_name(&mem_blk->dev.kobj), ret);
>>   
>> -	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> +	ret = sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>>   				&node_devices[nid]->dev.kobj,
>>   				kobject_name(&node_devices[nid]->dev.kobj));
>> +	if (ret && ret != -EEXIST)
>> +		dev_err_ratelimited(&mem_blk->dev,
>> +				    "can't create link to %s in sysfs (%d)\n",
>> +				    kobject_name(&node_devices[nid]->dev.kobj),
>> +				    ret);
>>   }
>>   
>>   /* register memory section under specified node if it spans that node */
>> @@ -817,7 +824,8 @@ static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>>   			continue;
>>   
>>   		/* The memory block is registered to the first matching node */
>> -		return do_register_memory_block_under_node(nid, mem_blk);
>> +		do_register_memory_block_under_node(nid, mem_blk);
>> +		return 0;
>>   	}
>>   	/* mem section does not span the specified node */
>>   	return 0;
>> @@ -832,7 +840,8 @@ static int register_mem_block_under_node_hotplug(struct memory_block *mem_blk,
>>   {
>>   	int nid = *(int *)arg;
>>   
>> -	return do_register_memory_block_under_node(nid, mem_blk);
>> +	do_register_memory_block_under_node(nid, mem_blk);
>> +	return 0;
>>   }
>>   
>>   /*
>> @@ -850,8 +859,8 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
>>   			  kobject_name(&node_devices[mem_blk->nid]->dev.kobj));
>>   }
>>   
>> -int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>> -		      enum meminit_context context)
>> +void link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>> +		       enum meminit_context context)
>>   {
>>   	walk_memory_blocks_func_t func;
>>   
>> @@ -860,9 +869,9 @@ int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn,
>>   	else
>>   		func = register_mem_block_under_node_early;
>>   
>> -	return walk_memory_blocks(PFN_PHYS(start_pfn),
>> -				  PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
>> -				  func);
>> +	walk_memory_blocks(PFN_PHYS(start_pfn), PFN_PHYS(end_pfn - start_pfn),
>> +			   (void *)&nid, func);
>> +	return;
>>   }
>>   
>>   #ifdef CONFIG_HUGETLBFS
>> diff --git a/include/linux/node.h b/include/linux/node.h
>> index 014ba3ab2efd..8e5a29897936 100644
>> --- a/include/linux/node.h
>> +++ b/include/linux/node.h
>> @@ -99,15 +99,14 @@ extern struct node *node_devices[];
>>   typedef  void (*node_registration_func_t)(struct node *);
>>   
>>   #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_NUMA)
>> -int link_mem_sections(int nid, unsigned long start_pfn,
>> -		      unsigned long end_pfn,
>> -		      enum meminit_context context);
>> +void link_mem_sections(int nid, unsigned long start_pfn,
>> +		       unsigned long end_pfn,
>> +		       enum meminit_context context);
>>   #else
>> -static inline int link_mem_sections(int nid, unsigned long start_pfn,
>> -				    unsigned long end_pfn,
>> -				    enum meminit_context context)
>> +static inline void link_mem_sections(int nid, unsigned long start_pfn,
>> +				     unsigned long end_pfn,
>> +				     enum meminit_context context)
>>   {
>> -	return 0;
>>   }
>>   #endif
>>   
>> @@ -130,8 +129,7 @@ static inline int register_one_node(int nid)
>>   		if (error)
>>   			return error;
>>   		/* link memory sections under this node */
>> -		error = link_mem_sections(nid, start_pfn, end_pfn,
>> -					  MEMINIT_EARLY);
>> +		link_mem_sections(nid, start_pfn, end_pfn, MEMINIT_EARLY);
>>   	}
>>   
>>   	return error;
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 03df20078827..01e01a530d38 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1080,9 +1080,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
>>   	}
>>   
>>   	/* link memory sections under this node.*/
>> -	ret = link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
>> -				MEMINIT_HOTPLUG);
>> -	BUG_ON(ret);
>> +	link_mem_sections(nid, PFN_DOWN(start), PFN_UP(start + size - 1),
>> +			  MEMINIT_HOTPLUG);
>>   
>>   	/* create new memmap entry */
>>   	if (!strcmp(res->name, "System RAM"))
>>
> 
> I just remember that I still have some cleanup patches lying around that
> rework the whole node onlining on the add_memory() path, being able to
> fail in a nice way rather than ignoring errors. Anyhow, this is good
> enough for now
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks David.