diff mbox series

[bionic:linux,1/2] Revert "mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock"

Message ID 20200211174359.12230-2-marcelo.cerri@canonical.com
State New
Headers show
Series LP:#1862312 - Segmentation fault (kernel oops) with memory-hotplug in ubuntu_kernel_selftests on Bionic kernel | expand

Commit Message

Marcelo Henrique Cerri Feb. 11, 2020, 5:43 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1862312

This reverts commit 483735440add2530c03da656ede83ee81414a566.

The backport for upstream commit 381eab4a6ee8 (mm/memory_hotplug: fix
online/offline_pages called w.o. mem_hotplug_lock) placed the call to
mem_hotplug_begin() on the wrong location and caused a regression.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
---
 drivers/base/memory.c | 13 ++++++++++++-
 mm/memory_hotplug.c   | 28 +++++++---------------------
 2 files changed, 19 insertions(+), 22 deletions(-)
diff mbox series

Patch

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index a3b8aebb95cb..fe1557aa9b10 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -224,6 +224,7 @@  static bool pages_correctly_reserved(unsigned long start_pfn)
 /*
  * MEMORY_HOTPLUG depends on SPARSEMEM in mm/Kconfig, so it is
  * OK to have direct references to sparsemem variables in here.
+ * Must already be protected by mem_hotplug_begin().
  */
 static int
 memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
@@ -289,6 +290,7 @@  static int memory_subsys_online(struct device *dev)
 	if (mem->online_type < 0)
 		mem->online_type = MMOP_ONLINE_KEEP;
 
+	/* Already under protection of mem_hotplug_begin() */
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
 	/* clear online_type */
@@ -335,11 +337,19 @@  store_mem_state(struct device *dev,
 		goto err;
 	}
 
+	/*
+	 * Memory hotplug needs to hold mem_hotplug_begin() for probe to find
+	 * the correct memory block to online before doing device_online(dev),
+	 * which will take dev->mutex.  Take the lock early to prevent an
+	 * inversion, memory_subsys_online() callbacks will be implemented by
+	 * assuming it's already protected.
+	 */
+	mem_hotplug_begin();
+
 	switch (online_type) {
 	case MMOP_ONLINE_KERNEL:
 	case MMOP_ONLINE_MOVABLE:
 	case MMOP_ONLINE_KEEP:
-		/* mem->online_type is protected by device_hotplug_lock */
 		mem->online_type = online_type;
 		ret = device_online(&mem->dev);
 		break;
@@ -350,6 +360,7 @@  store_mem_state(struct device *dev,
 		ret = -EINVAL; /* should never happen */
 	}
 
+	mem_hotplug_done();
 err:
 	unlock_device_hotplug();
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6cd0c4a144d5..6f9e895ce21a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -881,6 +881,7 @@  static struct zone * __meminit move_pfn_range(int online_type, int nid,
 	return zone;
 }
 
+/* Must be protected by mem_hotplug_begin() or a device_lock */
 int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_type)
 {
 	unsigned long flags;
@@ -904,8 +905,6 @@  int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	if (ret)
 		goto failed_addition;
 
-	mem_hotplug_begin();
-
 	/*
 	 * If this zone is not populated, then it is not in zonelist.
 	 * This means the page allocator ignores this zone.
@@ -951,7 +950,6 @@  int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 
 	if (onlined_pages)
 		memory_notify(MEM_ONLINE, &arg);
-	mem_hotplug_done();
 	return 0;
 
 failed_addition:
@@ -959,7 +957,6 @@  int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 		 (unsigned long long) pfn << PAGE_SHIFT,
 		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
 	memory_notify(MEM_CANCEL_ONLINE, &arg);
-	mem_hotplug_done();
 	return ret;
 }
 #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
@@ -1170,22 +1167,20 @@  int __ref add_memory_resource(int nid, struct resource *res, bool online)
 	/* create new memmap entry */
 	firmware_map_add_hotplug(start, start + size, "System RAM");
 
-	/* device_online() will take the lock when calling online_pages() */
-	mem_hotplug_done();
-
 	/* online pages if requested */
 	if (online)
 		walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1),
 				  NULL, online_memory_block);
 
-
-	return ret;
+	goto out;
 
 error:
 	/* rollback pgdat allocation and others */
 	if (new_pgdat && pgdat)
 		rollback_node_hotadd(nid, pgdat);
 	memblock_remove(start, size);
+
+out:
 	mem_hotplug_done();
 	return ret;
 }
@@ -1626,16 +1621,10 @@  static int __ref __offline_pages(unsigned long start_pfn,
 		return -EINVAL;
 	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
 		return -EINVAL;
-
-	mem_hotplug_begin();
-
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/
-	if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start,
-				  &valid_end)) {
-		mem_hotplug_done();
+	if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start, &valid_end))
 		return -EINVAL;
-	}
 
 	zone = page_zone(pfn_to_page(valid_start));
 	node = zone_to_nid(zone);
@@ -1644,10 +1633,8 @@  static int __ref __offline_pages(unsigned long start_pfn,
 	/* set above range as isolated */
 	ret = start_isolate_page_range(start_pfn, end_pfn,
 				       MIGRATE_MOVABLE, true);
-	if (ret) {
-		mem_hotplug_done();
+	if (ret)
 		return ret;
-	}
 
 	arg.start_pfn = start_pfn;
 	arg.nr_pages = nr_pages;
@@ -1718,7 +1705,6 @@  static int __ref __offline_pages(unsigned long start_pfn,
 	writeback_set_ratelimit();
 
 	memory_notify(MEM_OFFLINE, &arg);
-	mem_hotplug_done();
 	return 0;
 
 failed_removal:
@@ -1728,10 +1714,10 @@  static int __ref __offline_pages(unsigned long start_pfn,
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 	/* pushback to free area */
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
-	mem_hotplug_done();
 	return ret;
 }
 
+/* Must be protected by mem_hotplug_begin() or a device_lock */
 int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {
 	return __offline_pages(start_pfn, start_pfn + nr_pages);