Re: [PATCH RFCv2 0/6] mm: online/offline_pages called w.o. mem_hotplug_lock
From: David Hildenbrand
Date: Thu Aug 30 2018 - 08:31:27 EST
On 21.08.2018 12:44, David Hildenbrand wrote:
> 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.
> 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
> 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
> 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/
> 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
> - 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.
> 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(-)
David / dhildenb