mm: online/offline_pages called w.o. mem_hotplug_lock

mm/memory_hotplug.c +5 -0

+{
+	lock_device_hotplug();
+	__remove_memory(nid, start, size);
+	unlock_device_hotplug();
+}
 EXPORT_SYMBOL_GPL(remove_memory);
 #endif /* CONFIG_MEMORY_HOTREMOVE */

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1905

Quoted 2 lines

mm/memory_hotplug.c +2 -0

+
+void __ref remove_memory(int nid, u64 start, u64 size)

Remove __ref, otherwise looks good:

Reviewed-by Pavel Tatashin <[email protected]>

Quoted 7 lines

mm/memory_hotplug.c +5 -0

+{
+	lock_device_hotplug();
+	__remove_memory(nid, start, size);
+	unlock_device_hotplug();
+}
 EXPORT_SYMBOL_GPL(remove_memory);
 #endif /* CONFIG_MEMORY_HOTREMOVE */

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1905

Quoted 2 lines

mm/memory_hotplug.c +2 -0

+
+void __ref remove_memory(int nid, u64 start, u64 size)

Remove __ref, otherwise looks good:

Reviewed-by Pavel Tatashin <[email protected]>

Quoted 7 lines

mm/memory_hotplug.c +5 -0

+{
+	lock_device_hotplug();
+	__remove_memory(nid, start, size);
+	unlock_device_hotplug();
+}
 EXPORT_SYMBOL_GPL(remove_memory);
 #endif /* CONFIG_MEMORY_HOTREMOVE */

add_memory() currently does not take the device_hotplug_lock, however is aleady called under the lock from
arch/powerpc/platforms/pseries/hotplug-memory.c drivers/acpi/acpi_memhotplug.c
to synchronize against CPU hot-remove and similar.

In general, we should hold the device_hotplug_lock when adding memory to synchronize against online/offline request (e.g. from user space) - which already resulted in lock inversions due to device_lock() and mem_hotplug_lock - see 30467e0b3be ("mm, hotplug: fix concurrent memory hot-add deadlock"). add_memory()/add_memory_resource() will create memory block devices, so this really feels like the right thing to do.

Holding the device_hotplug_lock makes sure that a memory block device can really only be accessed (e.g. via .online/.state) from user space, once the memory has been fully added to the system.

The lock is not held yet in

	drivers/xen/balloon.c
	arch/powerpc/platforms/powernv/memtrace.c
	drivers/s390/char/sclp_cmd.c
	drivers/hv/hv_balloon.c

So, let's either use the locked variants or take the lock.

Don't export add_memory_resource(), as it once was exported to be used by XEN, which is never built as a module. If somebody requires it, we also have to export a locked variant (as device_hotplug_lock is never exported).

add_memory() currently does not take the device_hotplug_lock, however is aleady called under the lock from
arch/powerpc/platforms/pseries/hotplug-memory.c drivers/acpi/acpi_memhotplug.c
to synchronize against CPU hot-remove and similar.

In general, we should hold the device_hotplug_lock when adding memory to synchronize against online/offline request (e.g. from user space) - which already resulted in lock inversions due to device_lock() and mem_hotplug_lock - see 30467e0b3be ("mm, hotplug: fix concurrent memory hot-add deadlock"). add_memory()/add_memory_resource() will create memory block devices, so this really feels like the right thing to do.

Holding the device_hotplug_lock makes sure that a memory block device can really only be accessed (e.g. via .online/.state) from user space, once the memory has been fully added to the system.

The lock is not held yet in

	drivers/xen/balloon.c
	arch/powerpc/platforms/powernv/memtrace.c
	drivers/s390/char/sclp_cmd.c
	drivers/hv/hv_balloon.c

So, let's either use the locked variants or take the lock.

Don't export add_memory_resource(), as it once was exported to be used by XEN, which is never built as a module. If somebody requires it, we also have to export a locked variant (as device_hotplug_lock is never exported).

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b), followed by a), exposing a possible deadlock.

In [1], and it was decided to not make use of device_hotplug_lock, but rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which takes device_lock() - .online does no longer call mem_hotplug_begin(), so effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() without locks. This was introduced after 30467e0b3be. And skimming over the code, I assume it could need some more care in regards to locking (e.g. device_online() called without device_hotplug_lock - but I'll not touch that for now).

Now that we hold the device_hotplug_lock when
- adding memory (e.g. via add_memory()/add_memory_resource())
- removing memory (e.g. via remove_memory())
- device_online()/device_offline()

We can move mem_hotplug_lock usage back into
online_pages()/offline_pages().

Why is mem_hotplug_lock still needed? Essentially to make get_online_mems()/put_online_mems() be very fast (relying on device_hotplug_lock would be very slow), and to serialize against addition of memory that does not create memory block devices (hmm).

[1] http://driverdev.linuxdriverproject.org/pipermail/ driverdev-devel/

    2015-February/065324.html

This patch is partly based on a patch by Vitaly Kuznetsov.

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b), followed by a), exposing a possible deadlock.

In [1], and it was decided to not make use of device_hotplug_lock, but rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which takes device_lock() - .online does no longer call mem_hotplug_begin(), so effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() without locks. This was introduced after 30467e0b3be. And skimming over the code, I assume it could need some more care in regards to locking (e.g. device_online() called without device_hotplug_lock - but I'll not touch that for now).

Now that we hold the device_hotplug_lock when
- adding memory (e.g. via add_memory()/add_memory_resource())
- removing memory (e.g. via remove_memory())
- device_online()/device_offline()

We can move mem_hotplug_lock usage back into
online_pages()/offline_pages().

Why is mem_hotplug_lock still needed? Essentially to make get_online_mems()/put_online_mems() be very fast (relying on device_hotplug_lock would be very slow), and to serialize against addition of memory that does not create memory block devices (hmm).

[1] http://driverdev.linuxdriverproject.org/pipermail/ driverdev-devel/

    2015-February/065324.html

This patch is partly based on a patch by Vitaly Kuznetsov.

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b), followed by a), exposing a possible deadlock.

In [1], and it was decided to not make use of device_hotplug_lock, but rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which takes device_lock() - .online does no longer call mem_hotplug_begin(), so effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() without locks. This was introduced after 30467e0b3be. And skimming over the code, I assume it could need some more care in regards to locking (e.g. device_online() called without device_hotplug_lock - but I'll not touch that for now).

Hi David,

Quoted 6 lines · David Hildenbrand

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

Quoted 3 lines

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b), followed by a), exposing a possible deadlock.

Do you mean "onlining of memory from user space first took a), followed by b)"?

Quoted 16 lines

In [1], and it was decided to not make use of device_hotplug_lock, but rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which takes device_lock() - .online does no longer call mem_hotplug_begin(), so effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

Quoted 5 lines

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() without locks. This was introduced after 30467e0b3be. And skimming over the code, I assume it could need some more care in regards to locking (e.g. device_online() called without device_hotplug_lock - but I'll not touch that for now).

Can you mention that you fixed this in later patches?

The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by Rashmica Gupta <[email protected]>

Am 03.09.18 um 02:36 schrieb Rashmica:

Quoted 13 lines

Hi David,

Quoted 8 lines · David Hildenbrand

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible
lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

Quoted 7 lines

Quoted 4 lines

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b),
followed by a), exposing a possible deadlock.

Do you mean "onlining of memory from user space first took a), followed by b)"?

Very right, thanks.

Quoted 32 lines

Quoted 22 lines

In [1], and it was decided to not make use of
device_hotplug_lock, but
rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which
takes
device_lock() - .online does no longer call
mem_hotplug_begin(), so
effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however
onlining memory during add_memory() does not take care of
that.

In addition, I think there is also something wrong about the locking in

Quoted 9 lines

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages()
without locks. This was introduced after 30467e0b3be. And
skimming over
the code, I assume it could need some more care in regards to
locking
(e.g. device_online() called without device_hotplug_lock - but
I'll
not touch that for now).

Quoted 1 line

Can you mention that you fixed this in later patches?

Sure!

Quoted 2 lines

Quoted 4 lines

The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by Rashmica Gupta <[email protected]>

Thanks, r-b only for this patch or all of the series?

Am 03.09.18 um 02:36 schrieb Rashmica:

Quoted 13 lines

Hi David,

Quoted 8 lines · David Hildenbrand

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible
lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

Quoted 7 lines

Quoted 4 lines

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b),
followed by a), exposing a possible deadlock.

Do you mean "onlining of memory from user space first took a), followed by b)"?

Very right, thanks.

Quoted 32 lines

Quoted 22 lines

In [1], and it was decided to not make use of
device_hotplug_lock, but
rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which
takes
device_lock() - .online does no longer call
mem_hotplug_begin(), so
effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however
onlining memory during add_memory() does not take care of
that.

In addition, I think there is also something wrong about the locking in

Quoted 9 lines

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages()
without locks. This was introduced after 30467e0b3be. And
skimming over
the code, I assume it could need some more care in regards to
locking
(e.g. device_online() called without device_hotplug_lock - but
I'll
not touch that for now).

Quoted 1 line

Can you mention that you fixed this in later patches?

Sure!

Quoted 2 lines

Quoted 4 lines

The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by Rashmica Gupta <[email protected]>

Thanks, r-b only for this patch or all of the series?

Hi David,

Quoted 6 lines · David Hildenbrand

There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible lock inversion reported and discussed in [1] due to the two locks
a) device_lock()
b) mem_hotplug_lock

Quoted 3 lines

While add_memory() first takes b), followed by a) during bus_probe_device(), onlining of memory from user space first took b), followed by a), exposing a possible deadlock.

Do you mean "onlining of memory from user space first took a), followed by b)"?

Quoted 16 lines

In [1], and it was decided to not make use of device_hotplug_lock, but rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls mem_hotplug_begin() and the calls device_online() - which takes device_lock() - .online does no longer call mem_hotplug_begin(), so effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

Quoted 5 lines

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() without locks. This was introduced after 30467e0b3be. And skimming over the code, I assume it could need some more care in regards to locking (e.g. device_online() called without device_hotplug_lock - but I'll not touch that for now).

Can you mention that you fixed this in later patches?

The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by Rashmica Gupta <[email protected]>

device_online() should be called with device_hotplug_lock() held.

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Rashmica Gupta <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Michael Neuling <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 arch/powerpc/platforms/powernv/memtrace.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/powernv/memtrace.c +2 -0

index 8f1cd4f3bfd5..ef7181d4fe68 100644
@@ -229,9 +229,11 @@ static int memtrace_online(void)
 		 * we need to online the memory ourselves.
 		 */
 		if (!memhp_auto_online) {
+			lock_device_hotplug();
 			walk_memory_range(PFN_DOWN(ent->start),
 					  PFN_UP(ent->start + ent->size - 1),
 					  NULL, online_mem_block);
+			unlock_device_hotplug();
 		}
 
 		/*

device_online() should be called with device_hotplug_lock() held.

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Rashmica Gupta <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Michael Neuling <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 arch/powerpc/platforms/powernv/memtrace.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/powernv/memtrace.c +0 -0

index 8f1cd4f3bfd5..ef7181d4fe68 100644
@@ -229,9 +229,11 @@ static int memtrace_online(void)
 		 * we need to online the memory ourselves.

Let's perform all checking + offlining + removing under device_hotplug_lock, so nobody can mess with these devices via sysfs concurrently.

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Rashmica Gupta <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Michael Neuling <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 arch/powerpc/platforms/powernv/memtrace.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

arch/powerpc/platforms/powernv/memtrace.c +8 -2

index ef7181d4fe68..473e59842ec5 100644
@@ -74,9 +74,13 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
 {
 	u64 end_pfn = start_pfn + nr_pages - 1;
 
+	lock_device_hotplug();
+
 	if (walk_memory_range(start_pfn, end_pfn, NULL,
-	    check_memblock_online))
+	    check_memblock_online)) {
+		unlock_device_hotplug();
 		return false;
+	}
 
 	walk_memory_range(start_pfn, end_pfn, (void *)MEM_GOING_OFFLINE,
 			  change_memblock_state);

@@ -84,14 +88,16 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
 	if (offline_pages(start_pfn, nr_pages)) {
 		walk_memory_range(start_pfn, end_pfn, (void *)MEM_ONLINE,
 				  change_memblock_state);
+		unlock_device_hotplug();
 		return false;
 	}
 
 	walk_memory_range(start_pfn, end_pfn, (void *)MEM_OFFLINE,
 			  change_memblock_state);
 
-	remove_memory(nid, start_pfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
+	__remove_memory(nid, start_pfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
 
+	unlock_device_hotplug();
 	return true;
 }
 

Let's perform all checking + offlining + removing under device_hotplug_lock, so nobody can mess with these devices via sysfs concurrently.

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Rashmica Gupta <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Michael Neuling <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 arch/powerpc/platforms/powernv/memtrace.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

arch/powerpc/platforms/powernv/memtrace.c +6 -1

index ef7181d4fe68..473e59842ec5 100644
@@ -74,9 +74,13 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
 {
 	u64 end_pfn = start_pfn + nr_pages - 1;
 
+	lock_device_hotplug();
+
 	if (walk_memory_range(start_pfn, end_pfn, NULL,
-	    check_memblock_online))
+	    check_memblock_online)) {
+		unlock_device_hotplug();
 		return false;
+	}
 
 	walk_memory_range(start_pfn, end_pfn, (void *)MEM_GOING_OFFLINE,
 			  change_memblock_state);

@@ -84,14 +88,16 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
 	if (offline_pages(start_pfn, nr_pages)) {
 		walk_memory_range(start_pfn, end_pfn, (void *)MEM_ONLINE,
 				  change_memblock_state);
+		unlock_device_hotplug();
 		return false;
 	}
 

Let's document the magic a bit, especially why device_hotplug_lock is required when adding/removing memory and how it all play together with requests to online/offline memory from user space.

Let's document the magic a bit, especially why device_hotplug_lock is required when adding/removing memory and how it all play together with requests to online/offline memory from user space.

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

--------------------------------------------------------------------------

Reading through the code and studying how mem_hotplug_lock is to be used, I noticed that there are two places where we can end up calling device_online()/device_offline() - online_pages()/offline_pages() without the mem_hotplug_lock. And there are other places where we call device_online()/device_offline() without the device_hotplug_lock.

While e.g.
echo "online" > /sys/devices/system/memory/memory9/state
is fine, e.g.
echo 1 > /sys/devices/system/memory/memory9/online
Will not take the mem_hotplug_lock. However the device_lock() and device_hotplug_lock.

E.g. via memory_probe_store(), we can end up calling add_memory()->online_pages() without the device_hotplug_lock. So we can have concurrent callers in online_pages(). We e.g. touch in online_pages() basically unprotected zone->present_pages then.

Looks like there is a longer history to that (see Patch #2 for details), and fixing it to work the way it was intended is not really possible. We would e.g. have to take the mem_hotplug_lock in device/base/core.c, which sounds wrong.

Summary: We had a lock inversion on mem_hotplug_lock and device_lock(). More details can be found in patch 3 and patch 6.

I propose the general rules (documentation added in patch 6):

1. add_memory/add_memory_resource() must only be called with device_hotplug_lock.
2. remove_memory() must only be called with device_hotplug_lock. This is already documented and holds for all callers.
3. device_online()/device_offline() must only be called with device_hotplug_lock. This is already documented and true for now in core code. Other callers (related to memory hotplug) have to be fixed up.
4. mem_hotplug_lock is taken inside of add_memory/remove_memory/ online_pages/offline_pages.

To me, this looks way cleaner than what we have right now (and easier to verify). And looking at the documentation of remove_memory, using lock_device_hotplug also for add_memory() feels natural.

RFC -> RFCv2:
- Don't export device_hotplug_lock, provide proper remove_memory/add_memory wrappers.
- Split up the patches a bit.
- Try to improve powernv memtrace locking
- Add some documentation for locking that matches my knowledge

David Hildenbrand (6):
mm/memory_hotplug: make remove_memory() take the device_hotplug_lock mm/memory_hotplug: make add_memory() take the device_hotplug_lock mm/memory_hotplug: fix online/offline_pages called w.o.
mem_hotplug_lock
powerpc/powernv: hold device_hotplug_lock when calling device_online() powerpc/powernv: hold device_hotplug_lock in memtrace_offline_pages() memory-hotplug.txt: Add some details about locking internals

 Documentation/memory-hotplug.txt              | 39 +++++++++++-
 arch/powerpc/platforms/powernv/memtrace.c     | 14 +++--
 .../platforms/pseries/hotplug-memory.c        |  8 +--
 drivers/acpi/acpi_memhotplug.c                |  4 +-
 drivers/base/memory.c                         | 22 +++----
 drivers/xen/balloon.c                         |  3 +
 include/linux/memory_hotplug.h                |  4 +-
 mm/memory_hotplug.c                           | 59 +++++++++++++++----
 8 files changed, 115 insertions(+), 38 deletions(-)

Quoted 4 lines · David Hildenbrand

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

I'll be on vacation for two weeks starting on Saturday. If there are no comments I'll send this as !RFC once I return.

Quoted 4 lines · David Hildenbrand

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

I'll be on vacation for two weeks starting on Saturday. If there are no comments I'll send this as !RFC once I return.

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

--------------------------------------------------------------------------

Reading through the code and studying how mem_hotplug_lock is to be used, I noticed that there are two places where we can end up calling device_online()/device_offline() - online_pages()/offline_pages() without the mem_hotplug_lock. And there are other places where we call device_online()/device_offline() without the device_hotplug_lock.

While e.g.
echo "online" > /sys/devices/system/memory/memory9/state
is fine, e.g.
echo 1 > /sys/devices/system/memory/memory9/online
Will not take the mem_hotplug_lock. However the device_lock() and device_hotplug_lock.

E.g. via memory_probe_store(), we can end up calling add_memory()->online_pages() without the device_hotplug_lock. So we can have concurrent callers in online_pages(). We e.g. touch in online_pages() basically unprotected zone->present_pages then.

Looks like there is a longer history to that (see Patch #2 for details), and fixing it to work the way it was intended is not really possible. We would e.g. have to take the mem_hotplug_lock in device/base/core.c, which sounds wrong.

Summary: We had a lock inversion on mem_hotplug_lock and device_lock(). More details can be found in patch 3 and patch 6.

I propose the general rules (documentation added in patch 6):

1. add_memory/add_memory_resource() must only be called with device_hotplug_lock.
2. remove_memory() must only be called with device_hotplug_lock. This is already documented and holds for all callers.
3. device_online()/device_offline() must only be called with device_hotplug_lock. This is already documented and true for now in core code. Other callers (related to memory hotplug) have to be fixed up.
4. mem_hotplug_lock is taken inside of add_memory/remove_memory/ online_pages/offline_pages.

To me, this looks way cleaner than what we have right now (and easier to verify). And looking at the documentation of remove_memory, using lock_device_hotplug also for add_memory() feels natural.

RFC -> RFCv2:
- Don't export device_hotplug_lock, provide proper remove_memory/add_memory wrappers.
- Split up the patches a bit.
- Try to improve powernv memtrace locking
- Add some documentation for locking that matches my knowledge

David Hildenbrand (6):
mm/memory_hotplug: make remove_memory() take the device_hotplug_lock mm/memory_hotplug: make add_memory() take the device_hotplug_lock mm/memory_hotplug: fix online/offline_pages called w.o.
mem_hotplug_lock
powerpc/powernv: hold device_hotplug_lock when calling device_online() powerpc/powernv: hold device_hotplug_lock in memtrace_offline_pages() memory-hotplug.txt: Add some details about locking internals

 Documentation/memory-hotplug.txt              | 39 +++++++++++-
 arch/powerpc/platforms/powernv/memtrace.c     | 14 +++--
 .../platforms/pseries/hotplug-memory.c        |  8 +--
 drivers/acpi/acpi_memhotplug.c                |  4 +-
 drivers/base/memory.c                         | 22 +++----
 drivers/xen/balloon.c                         |  3 +
 include/linux/memory_hotplug.h                |  4 +-
 mm/memory_hotplug.c                           | 59 +++++++++++++++----
 8 files changed, 115 insertions(+), 38 deletions(-)

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

Quoted 3 lines · David Hildenbrand

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

Hi David,

I would like to review this but I am on vacation, so I will not be able to get to it soon.
I plan to do it once I am back.

Thanks

Quoted 3 lines · David Hildenbrand

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.

Hi David,

I would like to review this but I am on vacation, so I will not be able to get to it soon.
I plan to do it once I am back.

Thanks

This is the same approach as in the first RFC, but this time without exporting device_hotplug_lock (requested by Greg) and with some more details and documentation regarding locking. Tested only on x86 so far.