Message-Id: <20180925091457.28651-1-david@redhat.com>
Date:   Tue, 25 Sep 2018 11:14:51 +0200
From:   David Hildenbrand <david@...hat.com>
To:     linux-mm@...ck.org
Cc:     linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, linux-acpi@...r.kernel.org,
        xen-devel@...ts.xenproject.org, devel@...uxdriverproject.org,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Balbir Singh <bsingharora@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        John Allen <jallen@...ux.vnet.ibm.com>,
        Jonathan Corbet <corbet@....net>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Juergen Gross <jgross@...e.com>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Len Brown <lenb@...nel.org>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Mathieu Malaterre <malat@...ian.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Michael Neuling <mikey@...ling.org>,
        Michal Hocko <mhocko@...e.com>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>,
        Oscar Salvador <osalvador@...e.de>,
        Paul Mackerras <paulus@...ba.org>,
        Pavel Tatashin <pasha.tatashin@...cle.com>,
        Pavel Tatashin <pavel.tatashin@...rosoft.com>,
        Philippe Ombredanne <pombredanne@...b.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Rashmica Gupta <rashmica.g@...il.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vlastimil Babka <vbabka@...e.cz>,
        YASUAKI ISHIMATSU <yasu.isimatu@...il.com>
Subject: [PATCH v2 0/6] mm: online/offline_pages called w.o. mem_hotplug_lock

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.


v1 -> v2:
- Upstream changes in powerpc/powernv code required modifications to
  patch #1, #4 and #5.
- Minor patch description changes.
- Added more locking details in patch #6.
- Added rb's

RFCv2 -> v1:
- Dropped an unnecessary _ref from remove_memory() in patch #1
- Minor patch description fixes.
- Added rb's

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 when calling
    memtrace_offline_pages()
  memory-hotplug.txt: Add some details about locking internals

 Documentation/memory-hotplug.txt              | 42 ++++++++++++-
 arch/powerpc/platforms/powernv/memtrace.c     |  8 ++-
 .../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, 114 insertions(+), 36 deletions(-)

-- 
2.17.1

Powered by blists - more mailing lists