mm/hotplug: Only use subsection map in VMEMMAP case

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
__remove_pages+0x99/0xc0
arch_remove_memory+0x23/0x4d
try_remove_memory+0xc8/0x130
? walk_memory_blocks+0x72/0xa0
__remove_memory+0xa/0x11
acpi_memory_device_remove+0x72/0x100
acpi_bus_trim+0x55/0x90
acpi_device_hotplug+0x2eb/0x3d0
acpi_hotplug_work_fn+0x1a/0x30
process_one_work+0x1a7/0x370
worker_thread+0x30/0x380
? flush_rcu_work+0x30/0x30
kthread+0x112/0x130
? kthread_create_on_node+0x60/0x60
ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

---
mm/sparse.c | 4 +++-

1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +3 -1

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = (unsigned long)NULL;
 	}
 
 	if (section_is_early && memmap)
 		free_map_bootmem(memmap);
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
+
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		ms->section_mem_map = (unsigned long)NULL;
 }
 
 static struct page * __meminit section_activate(int nid, unsigned long pfn,

Signature · 1 line

2.17.2

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +0 -1

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = (unsigned long)NULL;
 	}
 

Quoted 27 lines · Baoquan He

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Quoted 3 lines

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Fixes Tag? Stable: Tag?

Quoted 18 lines

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +0 -1

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = (unsigned long)NULL;
 	}
 

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 793

Quoted 10 lines

mm/sparse.c +3 -0

 	if (section_is_early && memmap)
 		free_map_bootmem(memmap);
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
+
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		ms->section_mem_map = (unsigned long)NULL;
 }
 
 static struct page * __meminit section_activate(int nid, unsigned long pfn,

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +0 -0

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
                        ms->usage = NULL;
                }
                memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +0 -1

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = (unsigned long)NULL;
 	}
 

Quoted 55 lines · Baoquan He

In section_deactivate(), pfn_to_page() doesn't work any more after ms->section_mem_map is resetting to NULL in SPARSEMEM|!VMEMMAP case. It caused hot remove failure:

kernel BUG at mm/page_alloc.c:4806!
invalid opcode: 0000 [#1] SMP PTI

CPU: 3 PID: 8 Comm: kworker/u16:0 Tainted: G        W         5.5.0-next-20200205+ #340
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
RIP: 0010:free_pages+0x85/0xa0
Call Trace:
 __remove_pages+0x99/0xc0
 arch_remove_memory+0x23/0x4d
 try_remove_memory+0xc8/0x130
 ? walk_memory_blocks+0x72/0xa0
 __remove_memory+0xa/0x11
 acpi_memory_device_remove+0x72/0x100
 acpi_bus_trim+0x55/0x90
 acpi_device_hotplug+0x2eb/0x3d0
 acpi_hotplug_work_fn+0x1a/0x30
 process_one_work+0x1a7/0x370
 worker_thread+0x30/0x380
 ? flush_rcu_work+0x30/0x30
 kthread+0x112/0x130
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x35/0x40

Let's move the ->section_mem_map resetting after depopulate_section_memmap() to fix it.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/sparse.c +3 -1

index 596b2a45b100..b8e52c8fed7f 100644
@@ -779,13 +779,15 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = (unsigned long)NULL;
 	}
 
 	if (section_is_early && memmap)
 		free_map_bootmem(memmap);
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
+
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		ms->section_mem_map = (unsigned long)NULL;
 }
 

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 793

Quoted 2 lines

mm/sparse.c +0 -0

 static struct page * __meminit section_activate(int nid, unsigned long pfn,

As discussed, I think this is broken here already. As you explained, the subsection_map can get freed via kfree(ms->usage) after the first bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION) check.

Wrap the codes filling subsection map from section_activate() into fill_subsection_map(), this makes section_activate() cleaner and easier to follow.

Signed-off-by Baoquan He <[email protected]>

---
mm/sparse.c | 45 ++++++++++++++++++++++++++++++++++-----------

1 file changed, 34 insertions(+), 11 deletions(-)

mm/sparse.c +34 -11

index b8e52c8fed7f..977b47acd38d 100644
@@ -790,24 +790,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
-static struct page * __meminit section_activate(int nid, unsigned long pfn,
-		unsigned long nr_pages, struct vmem_altmap *altmap)
+/**
+ * fill_subsection_map - fill subsection map of a memory region
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This fills the related subsection map inside one section, and only
+ * intended for hotplug.
+ *
+ * Return:
+ * * 0		- On success.
+ * * -EINVAL	- Invalid memory region.
+ * * -EEXIST	- Subsection map has been set.
+ */
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
-	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	struct mem_section_usage *usage = NULL;
+	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	unsigned long *subsection_map;
-	struct page *memmap;
 	int rc = 0;
 
 	subsection_mask_set(map, pfn, nr_pages);
 
-	if (!ms->usage) {
-		usage = kzalloc(mem_section_usage_size(), GFP_KERNEL);
-		if (!usage)
-			return ERR_PTR(-ENOMEM);
-		ms->usage = usage;
-	}
 	subsection_map = &ms->usage->subsection_map[0];
 
 	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))

@@ -818,6 +822,25 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		bitmap_or(subsection_map, map, subsection_map,
 				SUBSECTIONS_PER_SECTION);
 
+	return rc;
+}
+
+static struct page * __meminit section_activate(int nid, unsigned long pfn,
+		unsigned long nr_pages, struct vmem_altmap *altmap)
+{
+	struct mem_section *ms = __pfn_to_section(pfn);
+	struct mem_section_usage *usage = NULL;
+	struct page *memmap;
+	int rc = 0;
+
+	if (!ms->usage) {
+		usage = kzalloc(mem_section_usage_size(), GFP_KERNEL);
+		if (!usage)
+			return ERR_PTR(-ENOMEM);
+		ms->usage = usage;
+	}
+
+	rc = fill_subsection_map(pfn, nr_pages);
 	if (rc) {
 		if (usage)
 			ms->usage = NULL;

Signature · 1 line

2.17.2

Wrap the codes filling subsection map from section_activate() into

fill_subsection_map(), this makes section_activate() cleaner and easier to follow.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

mm/sparse.c +3 -2

index b8e52c8fed7f..977b47acd38d 100644
@@ -790,24 +790,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
-static struct page * __meminit section_activate(int nid, unsigned long pfn,
-		unsigned long nr_pages, struct vmem_altmap *altmap)
+/**
+ * fill_subsection_map - fill subsection map of a memory region
+ * @pfn - start pfn of the memory range

Quoted 1 line · Baoquan He

Wrap the codes filling subsection map from section_activate() into

"Factor out the code that fills the subsection" ...

Quoted 26 lines

fill_subsection_map(), this makes section_activate() cleaner and easier to follow.

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

mm/sparse.c +7 -2

index b8e52c8fed7f..977b47acd38d 100644
@@ -790,24 +790,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
-static struct page * __meminit section_activate(int nid, unsigned long pfn,
-		unsigned long nr_pages, struct vmem_altmap *altmap)
+/**
+ * fill_subsection_map - fill subsection map of a memory region
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This fills the related subsection map inside one section, and only
+ * intended for hotplug.

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 805

Quoted 6 lines

mm/sparse.c +6 -0

+ *
+ * Return:
+ * * 0		- On success.
+ * * -EINVAL	- Invalid memory region.
+ * * -EEXIST	- Subsection map has been set.
+ */

Without this comment (or a massively reduced one :) )

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------

1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +38 -8

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+
+	return 1;
+}
+
+static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
+		struct vmem_altmap *altmap)
+{
+	struct mem_section *ms = __pfn_to_section(pfn);
+	bool section_is_early = early_section(ms);
+	struct page *memmap = NULL;
+	int rc;
+
+
+	rc = clear_subsection_map(pfn, nr_pages);
+	if (IS_ERR_VALUE((unsigned long)rc))
+		return;
 	/*
 	 * There are 3 cases to handle across two configurations
 	 * (SPARSEMEM_VMEMMAP={y,n}):

@@ -763,8 +794,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) {
+	if (!rc) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);
 
 		/*

@@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+	if (!rc)
 		ms->section_mem_map = (unsigned long)NULL;
 }
 

Signature · 1 line

2.17.2

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c +1 -2

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),

mm/sparse.c +2 -2

+		return;
 	/*
 	 * There are 3 cases to handle across two configurations
 	 * (SPARSEMEM_VMEMMAP={y,n}):

@@ -763,8 +794,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) {
+	if (!rc) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);
 
 		/*

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

I dislike the "rc" handling in the caller.

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 776

Quoted 12 lines

mm/sparse.c +12 -0

+	return 1;
+}
+
+static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
+		struct vmem_altmap *altmap)
+{
+	struct mem_section *ms = __pfn_to_section(pfn);
+	bool section_is_early = early_section(ms);
+	struct page *memmap = NULL;
+	int rc;
+
+

one superfluous empty line

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 778

Quoted 2 lines

mm/sparse.c +2 -0

+	rc = clear_subsection_map(pfn, nr_pages);
+	if (IS_ERR_VALUE((unsigned long)rc))

huh? "if (rc < 0)" ? or am I missing something?

Quoted 16 lines

mm/sparse.c +2 -2

+		return;
 	/*
 	 * There are 3 cases to handle across two configurations
 	 * (SPARSEMEM_VMEMMAP={y,n}):

@@ -763,8 +794,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) {
+	if (!rc) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);
 
 		/*

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 819

Quoted 5 lines

mm/sparse.c +1 -1

@@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+	if (!rc)

I don't really like that handling.

Either

s/rc/section_empty/

or use a separate subsection_map_empty()

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 822

Quoted 4 lines

mm/sparse.c +0 -0

 		ms->section_mem_map = (unsigned long)NULL;
 }
 

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 55 lines · David Hildenbrand

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

Quoted 8 lines

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

I don't follow. Could you be more specific? or pseudo code please?

The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more.

Quoted 67 lines · Baoquan He

Quoted 55 lines · David Hildenbrand

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

Quoted 8 lines

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

I don't follow. Could you be more specific? or pseudo code please?

Quoted 5 lines

The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more.

Something like this on top would be easier to understand IMHO

mm/sparse.c +8 -24

index dc79b00ddaaa..be5c80e9cfee 100644
@@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
 static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };

@@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 		return -EINVAL;
 
 	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+	return 0;
+}
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
+static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages)
+{
+	return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION);
 }
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,

@@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	struct mem_section *ms = __pfn_to_section(pfn);
 	bool section_is_early = early_section(ms);
 	struct page *memmap = NULL;
-	int rc;
-
 
-	rc = clear_subsection_map(pfn, nr_pages);
-	if (IS_ERR_VALUE((unsigned long)rc))
+	if (unlikely(clear_subsection_map(pfn, nr_pages)))
 		return;
 	/*
 	 * There are 3 cases to handle across two configurations

@@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	if (!rc) {
+	if (is_subsection_map_empty(pfn, nr_pages)) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);
 
 		/*

@@ -816,7 +800,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
 
-	if (!rc)
+	if (is_subsection_map_empty(pfn, nr_pages))
 		ms->section_mem_map = (unsigned long)NULL;
 }
 

Signature · 4 lines

-- 
Thanks,

David / dhildenb

Quoted 65 lines · Baoquan He

Quoted 55 lines · David Hildenbrand

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

Quoted 8 lines

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

Quoted 7 lines

I don't follow. Could you be more specific? or pseudo code please?

The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more.

Something like this on top would be easier to understand IMHO

mm/sparse.c +6 -22

index dc79b00ddaaa..be5c80e9cfee 100644
@@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
 static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };

@@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 		return -EINVAL;
 
 	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+	return 0;
+}
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
+static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages)
+{
+	return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION);
 }
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,

@@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	struct mem_section *ms = __pfn_to_section(pfn);
 	bool section_is_early = early_section(ms);
 	struct page *memmap = NULL;
-	int rc;
-
 
-	rc = clear_subsection_map(pfn, nr_pages);
-	if (IS_ERR_VALUE((unsigned long)rc))
+	if (unlikely(clear_subsection_map(pfn, nr_pages)))
 		return;
 	/*
 	 * There are 3 cases to handle across two configurations

mm/sparse.c +1 -1

 		/*

@@ -816,7 +800,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	else
 		depopulate_section_memmap(pfn, nr_pages, altmap);
 
-	if (!rc)
+	if (is_subsection_map_empty(pfn, nr_pages))
 		ms->section_mem_map = (unsigned long)NULL;
 }
 

Signature · 4 lines

-- 
Thanks,

David / dhildenb

Quoted 133 lines · David Hildenbrand

Quoted 65 lines · Baoquan He

Quoted 55 lines · David Hildenbrand

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

Quoted 8 lines

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

Quoted 7 lines

I don't follow. Could you be more specific? or pseudo code please?

The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more.

Something like this on top would be easier to understand IMHO

mm/sparse.c +6 -22

index dc79b00ddaaa..be5c80e9cfee 100644
@@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
 static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };

@@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 		return -EINVAL;
 
 	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+	return 0;
+}
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
+static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages)
+{
+	return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION);
 }
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,

@@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	struct mem_section *ms = __pfn_to_section(pfn);
 	bool section_is_early = early_section(ms);
 	struct page *memmap = NULL;
-	int rc;
-
 
-	rc = clear_subsection_map(pfn, nr_pages);
-	if (IS_ERR_VALUE((unsigned long)rc))
+	if (unlikely(clear_subsection_map(pfn, nr_pages)))
 		return;
 	/*
 	 * There are 3 cases to handle across two configurations

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 782

Quoted 6 lines

mm/sparse.c +1 -1

@@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	if (!rc) {
+	if (is_subsection_map_empty(pfn, nr_pages)) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);

Tried this way, it's not good in this patch. Since ms->usage might be freed in this place.

                if (!PageReserved(virt_to_page(ms->usage))) {
                        kfree(ms->usage);
                        ms->usage = NULL;
                }

Quoted 144 lines · Baoquan He

Quoted 133 lines · David Hildenbrand

Quoted 65 lines · Baoquan He

Quoted 55 lines · David Hildenbrand

Quoted 22 lines · Baoquan He

Wrap the codes which clear subsection map of one memory region from section_deactivate() into clear_subsection_map().

Signed-off-by Baoquan He <[email protected]>

---
 mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

mm/sparse.c +4 -2

index 977b47acd38d..df857ee9330c 100644
@@ -726,14 +726,25 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
-		struct vmem_altmap *altmap)
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 742

Quoted 10 lines

mm/sparse.c +10 -0

+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */

Less verbose please (in my preference: none and simplify return handling)

Quoted 19 lines

mm/sparse.c +4 -3

+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
 	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
 	struct mem_section *ms = __pfn_to_section(pfn);
-	bool section_is_early = early_section(ms);
-	struct page *memmap = NULL;
 	unsigned long *subsection_map = ms->usage
 		? &ms->usage->subsection_map[0] : NULL;
 

@@ -744,8 +755,28 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
 				"section already deactivated (%#lx + %ld)\n",
 				pfn, nr_pages))
-		return;
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
 

Quoted 8 lines

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 764

Quoted 3 lines

mm/sparse.c +3 -0

+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+

Can we please just have a

subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant)

Quoted 7 lines

I don't follow. Could you be more specific? or pseudo code please?

The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more.

Something like this on top would be easier to understand IMHO

mm/sparse.c +6 -22

index dc79b00ddaaa..be5c80e9cfee 100644
@@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
 static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 {
 	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };

@@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 		return -EINVAL;
 
 	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+	return 0;
+}
 
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
+static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages)
+{
+	return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION);
 }
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,

@@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	struct mem_section *ms = __pfn_to_section(pfn);
 	bool section_is_early = early_section(ms);
 	struct page *memmap = NULL;
-	int rc;
-
 
-	rc = clear_subsection_map(pfn, nr_pages);
-	if (IS_ERR_VALUE((unsigned long)rc))
+	if (unlikely(clear_subsection_map(pfn, nr_pages)))
 		return;
 	/*
 	 * There are 3 cases to handle across two configurations

mm/sparse.c static void section_deactivate(unsigned long pfn, unsigned long nr_pages, · line 782

Quoted 6 lines

mm/sparse.c +1 -1

@@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 	 *
 	 * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified
 	 */
-	if (!rc) {
+	if (is_subsection_map_empty(pfn, nr_pages)) {
 		unsigned long section_nr = pfn_to_section_nr(pfn);

Tried this way, it's not good in this patch. Since ms->usage might be freed in this place.

Quoted 4 lines

                if (!PageReserved(virt_to_page(ms->usage))) {
                        kfree(ms->usage);
                        ms->usage = NULL;
                }

So your patch #1 is already broken. Just cache the result in patch #1.

bool empty;

...
empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION);
...

if (empty) {
	...
}

Currently, subsection map is used when SPARSEMEM is enabled, including VMEMMAP case and !VMEMMAP case. However, subsection hotplug is not supported at all in SPARSEMEM|!VMEMMAP case, subsection map is unnecessary and misleading. Let's adjust code to only allow subsection map being used in VMEMMAP case.

Signed-off-by Baoquan He <[email protected]>

---
include/linux/mmzone.h | 2 ++
mm/sparse.c | 20 ++++++++++++++++++++

2 files changed, 22 insertions(+)

include/linux/mmzone.h +2 -0

index 462f6873905a..fc0de3a9a51e 100644
@@ -1185,7 +1185,9 @@ static inline unsigned long section_nr_to_pfn(unsigned long sec)
 #define SUBSECTION_ALIGN_DOWN(pfn) ((pfn) & PAGE_SUBSECTION_MASK)
 
 struct mem_section_usage {
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 	DECLARE_BITMAP(subsection_map, SUBSECTIONS_PER_SECTION);
+#endif
 	/* See declaration of similar field in struct zone */
 	unsigned long pageblock_flags[0];
 };

mm/sparse.c +20 -0

index df857ee9330c..66c497d6a229 100644
@@ -209,6 +209,7 @@ static inline unsigned long first_present_section_nr(void)
 	return next_present_section_nr(-1);
 }
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 static void subsection_mask_set(unsigned long *map, unsigned long pfn,
 		unsigned long nr_pages)
 {

@@ -243,6 +244,11 @@ void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 		nr_pages -= pfns;
 	}
 }
+#else
+void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
+{
+}
+#endif
 
 /* Record a memory area against a node. */
 void __init memory_present(int nid, unsigned long start, unsigned long end)

@@ -726,6 +732,7 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 /**
 * clear_subsection_map - Clear subsection map of one memory region
 *

@@ -764,6 +771,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 
 	return 1;
 }
+#else
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+#endif
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		struct vmem_altmap *altmap)

@@ -820,6 +833,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 /**
 * fill_subsection_map - fill subsection map of a memory region
 * @pfn - start pfn of the memory range

@@ -854,6 +868,12 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 
 	return rc;
 }
+#else
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+#endif
 
 static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap)

Signature · 1 line

2.17.2

Currently, subsection map is used when SPARSEMEM is enabled, including VMEMMAP case and !VMEMMAP case. However, subsection hotplug is not supported at all in SPARSEMEM|!VMEMMAP case, subsection map is unnecessary and misleading. Let's adjust code to only allow subsection map being used in VMEMMAP case.

Signed-off-by Baoquan He <[email protected]>

---
 include/linux/mmzone.h |  2 ++
 mm/sparse.c            | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

include/linux/mmzone.h +2 -0

index 462f6873905a..fc0de3a9a51e 100644
@@ -1185,7 +1185,9 @@ static inline unsigned long section_nr_to_pfn(unsigned long sec)
 #define SUBSECTION_ALIGN_DOWN(pfn) ((pfn) & PAGE_SUBSECTION_MASK)
 
 struct mem_section_usage {
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 	DECLARE_BITMAP(subsection_map, SUBSECTIONS_PER_SECTION);
+#endif
 	/* See declaration of similar field in struct zone */
 	unsigned long pageblock_flags[0];
 };

mm/sparse.c +20 -0

index df857ee9330c..66c497d6a229 100644
@@ -209,6 +209,7 @@ static inline unsigned long first_present_section_nr(void)
 	return next_present_section_nr(-1);
 }
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 static void subsection_mask_set(unsigned long *map, unsigned long pfn,
 		unsigned long nr_pages)
 {

@@ -243,6 +244,11 @@ void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 		nr_pages -= pfns;
 	}
 }
+#else
+void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
+{
+}
+#endif
 
 /* Record a memory area against a node. */
 void __init memory_present(int nid, unsigned long start, unsigned long end)

@@ -726,6 +732,7 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 /**
  * clear_subsection_map - Clear subsection map of one memory region
  *

@@ -764,6 +771,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
 
 	return 1;
 }
+#else
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+#endif
 
 static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		struct vmem_altmap *altmap)

@@ -820,6 +833,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
 /**
  * fill_subsection_map - fill subsection map of a memory region
  * @pfn - start pfn of the memory range

@@ -854,6 +868,12 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 
 	return rc;
 }
+#else
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+#endif
 
 static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap)

Signature · 1 line

2.17.2

Quoted 5 lines · Baoquan He

Currently, subsection map is used when SPARSEMEM is enabled, including VMEMMAP case and !VMEMMAP case. However, subsection hotplug is not supported at all in SPARSEMEM|!VMEMMAP case, subsection map is unnecessary and misleading. Let's adjust code to only allow subsection map being used in VMEMMAP case.

This really needs more explanation I believe. What exactly happens if somebody tries to hotremove a part of the section with !VMEMMAP? I can see that clear_subsection_map returns 0 but that is not an error code. Besides that section_deactivate doesn't propagate the error upwards. /me stares into the code

OK, I can see it now. It is relying on check_pfn_span to use the proper subsection granularity. This really begs for a comment in the code somewhere.

Quoted 13 lines · Michal Hocko

Quoted 5 lines · Baoquan He

Currently, subsection map is used when SPARSEMEM is enabled, including VMEMMAP case and !VMEMMAP case. However, subsection hotplug is not supported at all in SPARSEMEM|!VMEMMAP case, subsection map is unnecessary and misleading. Let's adjust code to only allow subsection map being used in VMEMMAP case.

This really needs more explanation I believe. What exactly happens if somebody tries to hotremove a part of the section with !VMEMMAP? I can see that clear_subsection_map returns 0 but that is not an error code. Besides that section_deactivate doesn't propagate the error upwards. /me stares into the code

Quoted 3 lines

OK, I can see it now. It is relying on check_pfn_span to use the proper subsection granularity. This really begs for a comment in the code somewhere.

Yes, check_pfn_span() guards it. People have no way to hot add/remove on non-section aligned block with !VMEMMAP.

I have added extra comment to above section_activate() to note this, please check patch 5/7. Let me see how to add words to reflect the check_pfn_span() guard thing.

Quoted 21 lines · Baoquan He

Quoted 13 lines · Michal Hocko

Quoted 5 lines · Baoquan He

Currently, subsection map is used when SPARSEMEM is enabled, including VMEMMAP case and !VMEMMAP case. However, subsection hotplug is not supported at all in SPARSEMEM|!VMEMMAP case, subsection map is unnecessary and misleading. Let's adjust code to only allow subsection map being used in VMEMMAP case.

This really needs more explanation I believe. What exactly happens if somebody tries to hotremove a part of the section with !VMEMMAP? I can see that clear_subsection_map returns 0 but that is not an error code. Besides that section_deactivate doesn't propagate the error upwards. /me stares into the code

Quoted 3 lines

OK, I can see it now. It is relying on check_pfn_span to use the proper subsection granularity. This really begs for a comment in the code somewhere.

Yes, check_pfn_span() guards it. People have no way to hot add/remove on non-section aligned block with !VMEMMAP.

Quoted 3 lines

I have added extra comment to above section_activate() to note this, please check patch 5/7. Let me see how to add words to reflect the check_pfn_span() guard thing.

An explicit note about check_pfn_span gating the proper alignement and sizing sounds sufficient to me.

No functional change.

Signed-off-by Baoquan He <[email protected]>

---
mm/sparse.c | 172 +++++++++++++++++++++++++---------------------------

1 file changed, 84 insertions(+), 88 deletions(-)

mm/sparse.c +84 -88

index 14bff0b44e7c..053d6c2e5c1f 100644
@@ -244,10 +244,94 @@ void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 		nr_pages -= pfns;
 	}
 }
+
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
+	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
+	struct mem_section *ms = __pfn_to_section(pfn);
+	unsigned long *subsection_map = ms->usage
+		? &ms->usage->subsection_map[0] : NULL;
+
+	subsection_mask_set(map, pfn, nr_pages);
+	if (subsection_map)
+		bitmap_and(tmp, map, subsection_map, SUBSECTIONS_PER_SECTION);
+
+	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
+				"section already deactivated (%#lx + %ld)\n",
+				pfn, nr_pages))
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+
+	return 1;
+}
+
+/**
+ * fill_subsection_map - fill subsection map of a memory region
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This fills the related subsection map inside one section, and only
+ * intended for hotplug.
+ *
+ * Return:
+ * * 0		- On success.
+ * * -EINVAL	- Invalid memory region.
+ * * -EEXIST	- Subsection map has been set.
+ */
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	struct mem_section *ms = __pfn_to_section(pfn);
+	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
+	unsigned long *subsection_map;
+	int rc = 0;
+
+	subsection_mask_set(map, pfn, nr_pages);
+
+	subsection_map = &ms->usage->subsection_map[0];
+
+	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
+		rc = -EINVAL;
+	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
+		rc = -EEXIST;
+	else
+		bitmap_or(subsection_map, map, subsection_map,
+				SUBSECTIONS_PER_SECTION);
+
+	return rc;
+}
 #else
 void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 {
 }
+
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
 #endif
 
 /* Record a memory area against a node. */

@@ -732,52 +816,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
-static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
-	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
-	struct mem_section *ms = __pfn_to_section(pfn);
-	unsigned long *subsection_map = ms->usage
-		? &ms->usage->subsection_map[0] : NULL;
-
-	subsection_mask_set(map, pfn, nr_pages);
-	if (subsection_map)
-		bitmap_and(tmp, map, subsection_map, SUBSECTIONS_PER_SECTION);
-
-	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
-				"section already deactivated (%#lx + %ld)\n",
-				pfn, nr_pages))
-		return -EINVAL;
-
-	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
-
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
-}
-#else
-static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	return 0;
-}
-#endif
-
 /*
 * To deactivate a memory region, there are 3 cases to handle across
 * two configurations (SPARSEMEM_VMEMMAP={y,n}):

@@ -833,48 +871,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-/**
- * fill_subsection_map - fill subsection map of a memory region
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This fills the related subsection map inside one section, and only
- * intended for hotplug.
- *
- * Return:
- * * 0		- On success.
- * * -EINVAL	- Invalid memory region.
- * * -EEXIST	- Subsection map has been set.
- */
-static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	struct mem_section *ms = __pfn_to_section(pfn);
-	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
-	unsigned long *subsection_map;
-	int rc = 0;
-
-	subsection_mask_set(map, pfn, nr_pages);
-
-	subsection_map = &ms->usage->subsection_map[0];
-
-	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
-		rc = -EINVAL;
-	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
-		rc = -EEXIST;
-	else
-		bitmap_or(subsection_map, map, subsection_map,
-				SUBSECTIONS_PER_SECTION);
-
-	return rc;
-}
-#else
-static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	return 0;
-}
-#endif
-
 static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap)
 {

Signature · 1 line

2.17.2

Quoted 2 lines · Baoquan He

No functional change.

Those functions are introduced in your previous patches.

Is it possible to define them close to each other at the very beginning?

Quoted 209 lines

Signed-off-by Baoquan He <[email protected]>

---
mm/sparse.c | 172 +++++++++++++++++++++++++---------------------------

1 file changed, 84 insertions(+), 88 deletions(-)

mm/sparse.c +84 -88

index 14bff0b44e7c..053d6c2e5c1f 100644
@@ -244,10 +244,94 @@ void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 		nr_pages -= pfns;
 	}
 }
+
+/**
+ * clear_subsection_map - Clear subsection map of one memory region
+ *
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This is only intended for hotplug, and clear the related subsection
+ * map inside one section.
+ *
+ * Return:
+ * * -EINVAL	- Section already deactived.
+ * * 0		- Subsection map is emptied.
+ * * 1		- Subsection map is not empty.
+ */
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
+	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
+	struct mem_section *ms = __pfn_to_section(pfn);
+	unsigned long *subsection_map = ms->usage
+		? &ms->usage->subsection_map[0] : NULL;
+
+	subsection_mask_set(map, pfn, nr_pages);
+	if (subsection_map)
+		bitmap_and(tmp, map, subsection_map, SUBSECTIONS_PER_SECTION);
+
+	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
+				"section already deactivated (%#lx + %ld)\n",
+				pfn, nr_pages))
+		return -EINVAL;
+
+	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
+
+	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
+		return 0;
+
+	return 1;
+}
+
+/**
+ * fill_subsection_map - fill subsection map of a memory region
+ * @pfn - start pfn of the memory range
+ * @nr_pages - number of pfns to add in the region
+ *
+ * This fills the related subsection map inside one section, and only
+ * intended for hotplug.
+ *
+ * Return:
+ * * 0		- On success.
+ * * -EINVAL	- Invalid memory region.
+ * * -EEXIST	- Subsection map has been set.
+ */
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	struct mem_section *ms = __pfn_to_section(pfn);
+	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
+	unsigned long *subsection_map;
+	int rc = 0;
+
+	subsection_mask_set(map, pfn, nr_pages);
+
+	subsection_map = &ms->usage->subsection_map[0];
+
+	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
+		rc = -EINVAL;
+	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
+		rc = -EEXIST;
+	else
+		bitmap_or(subsection_map, map, subsection_map,
+				SUBSECTIONS_PER_SECTION);
+
+	return rc;
+}
 #else
 void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
 {
 }
+
+static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+
+static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
+{
+	return 0;
+}
 #endif
 
 /* Record a memory area against a node. */

@@ -732,52 +816,6 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-/**
- * clear_subsection_map - Clear subsection map of one memory region
- *
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This is only intended for hotplug, and clear the related subsection
- * map inside one section.
- *
- * Return:
- * * -EINVAL	- Section already deactived.
- * * 0		- Subsection map is emptied.
- * * 1		- Subsection map is not empty.
- */
-static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
-	DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 };
-	struct mem_section *ms = __pfn_to_section(pfn);
-	unsigned long *subsection_map = ms->usage
-		? &ms->usage->subsection_map[0] : NULL;
-
-	subsection_mask_set(map, pfn, nr_pages);
-	if (subsection_map)
-		bitmap_and(tmp, map, subsection_map, SUBSECTIONS_PER_SECTION);
-
-	if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION),
-				"section already deactivated (%#lx + %ld)\n",
-				pfn, nr_pages))
-		return -EINVAL;
-
-	bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION);
-
-	if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION))
-		return 0;
-
-	return 1;
-}
-#else
-static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	return 0;
-}
-#endif
-
 /*
 * To deactivate a memory region, there are 3 cases to handle across
 * two configurations (SPARSEMEM_VMEMMAP={y,n}):

@@ -833,48 +871,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 		ms->section_mem_map = (unsigned long)NULL;
 }
 
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-/**
- * fill_subsection_map - fill subsection map of a memory region
- * @pfn - start pfn of the memory range
- * @nr_pages - number of pfns to add in the region
- *
- * This fills the related subsection map inside one section, and only
- * intended for hotplug.
- *
- * Return:
- * * 0		- On success.
- * * -EINVAL	- Invalid memory region.
- * * -EEXIST	- Subsection map has been set.
- */
-static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	struct mem_section *ms = __pfn_to_section(pfn);
-	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
-	unsigned long *subsection_map;
-	int rc = 0;
-
-	subsection_mask_set(map, pfn, nr_pages);
-
-	subsection_map = &ms->usage->subsection_map[0];
-
-	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
-		rc = -EINVAL;
-	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
-		rc = -EEXIST;
-	else
-		bitmap_or(subsection_map, map, subsection_map,
-				SUBSECTIONS_PER_SECTION);
-
-	return rc;
-}
-#else
-static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
-{
-	return 0;
-}
-#endif
-
 static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap)
 {

Signature · 1 line

2.17.2

Signature · 2 lines

Wei Yang
Help you, Help me

Quoted 6 lines · Wei Yang

Quoted 2 lines · Baoquan He

No functional change.

Those functions are introduced in your previous patches.

Quoted 1 line

Is it possible to define them close to each other at the very beginning?

Thanks for reviewing.

Do you mean to discard this patch and keep it as they are in the patch 4/7? If yes, it's fine to me to drop it as you suggested. To me, I prefer to put all subsection map handling codes together.

Quoted 7 lines · Baoquan He

Quoted 6 lines · Wei Yang

Quoted 2 lines · Baoquan He

No functional change.

Those functions are introduced in your previous patches.

Quoted 8 lines

Quoted 1 line

Is it possible to define them close to each other at the very beginning?

Thanks for reviewing.

Do you mean to discard this patch and keep it as they are in the patch 4/7? If yes, it's fine to me to drop it as you suggested. To me, I prefer to put all subsection map handling codes together.

I mean when you introduce clear_subsection_map() in patch 3, is it possible to move close to the definition of fill_subsection_map()?

Since finally you are will to move them together.

Quoted 20 lines · Wei Yang

Quoted 7 lines · Baoquan He

Quoted 6 lines · Wei Yang

Quoted 2 lines · Baoquan He

No functional change.

Those functions are introduced in your previous patches.

Quoted 8 lines

Quoted 1 line

Is it possible to define them close to each other at the very beginning?

Thanks for reviewing.

Do you mean to discard this patch and keep it as they are in the patch 4/7? If yes, it's fine to me to drop it as you suggested. To me, I prefer to put all subsection map handling codes together.

I mean when you introduce clear_subsection_map() in patch 3, is it possible to move close to the definition of fill_subsection_map()?

Quoted 1 line

Since finally you are will to move them together.

Oh, got it. Yeah, I just put them close to their callers separately. I think it's also good to put them together as you suggested, but it doesn't matter much, right? I will consider this and see if I can adjust it if v3 is needed. Thanks.

Memory sub-section hotplug was added to fix the issue that nvdimm could be mapped at non-section aligned starting address. A subsection map is added into struct mem_section_usage to implement it. However, sub-section is only supported in VMEMMAP case.

Hence there's no need to operate
subsection map in SPARSEMEM|!VMEMMAP case. In this patchset, change codes to make sub-section map and the relevant operation only available in VMEMMAP case.

And since sub-section hotplug added, the hot add/remove functionality have been broken in SPARSEMEM|!VMEMMAP case. Wei Yang and I, each of us make one patch to fix one of the failures. In this patchset, the patch 1/7 from me is used to fix the hot remove failure. Wei Yang's patch has been merged by Andrew.

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 4 lines · Baoquan He

Memory sub-section hotplug was added to fix the issue that nvdimm could be mapped at non-section aligned starting address. A subsection map is added into struct mem_section_usage to implement it. However, sub-section is only supported in VMEMMAP case.

Why? Is there any fundamental reason or just a lack of implementation? VMEMMAP should be really only an implementation detail unless I am missing something subtle.

Quoted 6 lines

Hence there's no need to operate
subsection map in SPARSEMEM|!VMEMMAP case. In this patchset, change codes to make sub-section map and the relevant operation only available in VMEMMAP case.

Quoted 5 lines

And since sub-section hotplug added, the hot add/remove functionality have been broken in SPARSEMEM|!VMEMMAP case. Wei Yang and I, each of us make one patch to fix one of the failures. In this patchset, the patch 1/7 from me is used to fix the hot remove failure. Wei Yang's patch has been merged by Andrew.

Not sure I understand. Are there more issues to be fixed?

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Why do we need to add so much code to remove a functionality from one memory model?

Quoted 4 lines

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 2 lines

Why do we need to add so much code to remove a functionality from one memory model?

Hmm, Dan also asked this before.

The adding mainly happens in patch 2, 3, 4, including the two newly added function defitions, the code comments above them, and those added dummy functions for !VMEMMAP.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 8 lines

Quoted 2 lines

Why do we need to add so much code to remove a functionality from one memory model?

Hmm, Dan also asked this before.

The adding mainly happens in patch 2, 3, 4, including the two newly added function defitions, the code comments above them, and those added dummy functions for !VMEMMAP.

AFAIKS, it's mostly a bunch of newly added comments on top of functions. E.g., the comment for fill_subsection_map() alone spans 12 LOC in total. I do wonder if we have to be that verbose. We are barely that verbose on MM code (and usually I don't see much benefit unless it's a function with many users from many different places).

Quoted 13 lines · David Hildenbrand

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 8 lines

Quoted 2 lines

Why do we need to add so much code to remove a functionality from one memory model?

Hmm, Dan also asked this before.

The adding mainly happens in patch 2, 3, 4, including the two newly added function defitions, the code comments above them, and those added dummy functions for !VMEMMAP.

Quoted 5 lines

AFAIKS, it's mostly a bunch of newly added comments on top of functions. E.g., the comment for fill_subsection_map() alone spans 12 LOC in total. I do wonder if we have to be that verbose. We are barely that verbose on MM code (and usually I don't see much benefit unless it's a function with many users from many different places).

I would tend to agree here. Not that I am against kernel doc documentation but these are internal functions and the comment doesn't really give any better insight IMHO. I would be much more inclined if this was the general pattern in the respective file but it just stands out.

Quoted 20 lines · Michal Hocko

Quoted 13 lines · David Hildenbrand

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 8 lines

Quoted 2 lines

Why do we need to add so much code to remove a functionality from one memory model?

Hmm, Dan also asked this before.

The adding mainly happens in patch 2, 3, 4, including the two newly added function defitions, the code comments above them, and those added dummy functions for !VMEMMAP.

Quoted 5 lines

AFAIKS, it's mostly a bunch of newly added comments on top of functions. E.g., the comment for fill_subsection_map() alone spans 12 LOC in total. I do wonder if we have to be that verbose. We are barely that verbose on MM code (and usually I don't see much benefit unless it's a function with many users from many different places).

Quoted 5 lines

I would tend to agree here. Not that I am against kernel doc documentation but these are internal functions and the comment doesn't really give any better insight IMHO. I would be much more inclined if this was the general pattern in the respective file but it just stands out.

I saw there are internal functions which have code comments, e.g shrink_slab() in mm/vmscan.c, not only this one place, there are several places. I personally prefer to see code comment for function if possible, this can save time, e.g people can skip the bitmap operation when read code if not necessary. And here I mainly want to tell there are different returned value to note different behaviour when call them.

Quoted 27 lines · Baoquan He

Quoted 20 lines · Michal Hocko

Quoted 13 lines · David Hildenbrand

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

 include/linux/mmzone.h |   2 +
 mm/sparse.c            | 178 +++++++++++++++++++++++++++++------------
 2 files changed, 127 insertions(+), 53 deletions(-)

Quoted 8 lines

Quoted 2 lines

Why do we need to add so much code to remove a functionality from one memory model?

Hmm, Dan also asked this before.

The adding mainly happens in patch 2, 3, 4, including the two newly added function defitions, the code comments above them, and those added dummy functions for !VMEMMAP.

Quoted 5 lines

AFAIKS, it's mostly a bunch of newly added comments on top of functions. E.g., the comment for fill_subsection_map() alone spans 12 LOC in total. I do wonder if we have to be that verbose. We are barely that verbose on MM code (and usually I don't see much benefit unless it's a function with many users from many different places).

Quoted 5 lines

I would tend to agree here. Not that I am against kernel doc documentation but these are internal functions and the comment doesn't really give any better insight IMHO. I would be much more inclined if this was the general pattern in the respective file but it just stands out.

Quoted 6 lines

I saw there are internal functions which have code comments, e.g shrink_slab() in mm/vmscan.c, not only this one place, there are several places. I personally prefer to see code comment for function if possible, this can save time, e.g people can skip the bitmap operation when read code if not necessary. And here I mainly want to tell there are different returned value to note different behaviour when call them.

... yet nobody really cares about different return code. It is an error that is propagated up the call chain and that's all.

I also like when there is a kernel doc comment that helps to understand the intented usage, context the function can be called from, potential side effects, locking requirements and other details people need to know

when calling functions. But have a look at
/**
* clear_subsection_map - Clear subsection map of one memory region *
* @pfn - start pfn of the memory range
* @nr_pages - number of pfns to add in the region
*
* This is only intended for hotplug, and clear the related subsection
* map inside one section.
*
* Return:
* * -EINVAL - Section already deactived.
* * 0 - Subsection map is emptied.
* * 1 - Subsection map is not empty.
*/

the only useful information in here is that this is a hotplug stuff but I would be completely lost about the intention without already knowing what is this whole subsection about.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 9 lines · Michal Hocko

Quoted 4 lines · Baoquan He

Memory sub-section hotplug was added to fix the issue that nvdimm could be mapped at non-section aligned starting address. A subsection map is added into struct mem_section_usage to implement it. However, sub-section is only supported in VMEMMAP case.

Why? Is there any fundamental reason or just a lack of implementation? VMEMMAP should be really only an implementation detail unless I am missing something subtle.

Thanks for checking.

VMEMMAP is one of two ways to convert a PFN to the corresponding 'struct page' in SPARSE model. I mentioned them as VMEMMAP case, or !VMEMMAP case because we called them like this previously when reviewed patches, hope it won't cause confusion.

Currently, config ZONE_DEVICE depends on SPARSEMEM_VMEMMAP. The subsection_map is added to struct mem_section_usage to track which sub section is present, VMEMMAP fills those bits which corresponding sub-sections are present, while !VMEMMAP, namely classic SPARSE, fills the whole map always.

As we know, VMEMMAP builds page table to map a cluster of 'struct page' into the corresponding area of 'vmemmap'. Subsection hotplug can be supported naturally, w/o any change, just map needed region related to sub-sections on demand. For !VMEMMAP, it allocates memmap with alloc_pages() or vmalloc, thing is a little complicated, e.g the mixed section, boot memory occupies the starting area, later pmem hot added to the rear part.

About !VMEMMAP which doesn't support sub-section hotplog, Dan said it's more because the effort and maintenance burden outweighs the benefit. And the current 64 bit ARCHes all enable SPARSEMEM_VMEMMAP_ENABLE by default.

Quoted 32 lines · Baoquan He

Quoted 9 lines · Michal Hocko

Quoted 4 lines · Baoquan He

Memory sub-section hotplug was added to fix the issue that nvdimm could be mapped at non-section aligned starting address. A subsection map is added into struct mem_section_usage to implement it. However, sub-section is only supported in VMEMMAP case.

Why? Is there any fundamental reason or just a lack of implementation? VMEMMAP should be really only an implementation detail unless I am missing something subtle.

Thanks for checking.

VMEMMAP is one of two ways to convert a PFN to the corresponding 'struct page' in SPARSE model. I mentioned them as VMEMMAP case, or !VMEMMAP case because we called them like this previously when reviewed patches, hope it won't cause confusion.

Currently, config ZONE_DEVICE depends on SPARSEMEM_VMEMMAP. The subsection_map is added to struct mem_section_usage to track which sub section is present, VMEMMAP fills those bits which corresponding sub-sections are present, while !VMEMMAP, namely classic SPARSE, fills the whole map always.

As we know, VMEMMAP builds page table to map a cluster of 'struct page' into the corresponding area of 'vmemmap'. Subsection hotplug can be supported naturally, w/o any change, just map needed region related to sub-sections on demand. For !VMEMMAP, it allocates memmap with alloc_pages() or vmalloc, thing is a little complicated, e.g the mixed section, boot memory occupies the starting area, later pmem hot added to the rear part.

Quoted 4 lines

About !VMEMMAP which doesn't support sub-section hotplog, Dan said it's more because the effort and maintenance burden outweighs the benefit. And the current 64 bit ARCHes all enable SPARSEMEM_VMEMMAP_ENABLE by default.

OK, if this is the primary argument then make sure to document it in the changelog (cover letter).