Re: [PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
On Mon, 2018-09-17 at 09:32 +0200, David Hildenbrand wrote:
> Am 03.09.18 um 02:36 schrieb Rashmica:
> > Hi David,
> >
> >
> > On 21/08/18 20:44, David Hildenbrand wrote:
> >
> > > 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.
> >
> > Do you mean "onlining of memory from user space first took a),
> > followed by b)"?
>
> Very right, thanks.
>
> >
> > > 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).
> >
> > Can you mention that you fixed this in later patches?
>
> Sure!
>
> >
> >
> > The series looks good to me. Feel free to add my reviewed-by:
> >
> > Reviewed-by: Rashmica Gupta
> >
>
> Thanks, r-b only for this patch or all of the series?
Sorry, I somehow missed this. To all of the series.
>
Re: [PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
Am 03.09.18 um 02:36 schrieb Rashmica:
> Hi David,
>
>
> On 21/08/18 20:44, David Hildenbrand wrote:
>
>> 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.
>
> Do you mean "onlining of memory from user space first took a),
> followed by b)"?
Very right, thanks.
>
>> 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).
>
> Can you mention that you fixed this in later patches?
Sure!
>
>
> The series looks good to me. Feel free to add my reviewed-by:
>
> Reviewed-by: Rashmica Gupta
>
Thanks, r-b only for this patch or all of the series?
--
Thanks,
David / dhildenb
Re: [PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
On 8/21/18 6:44 AM, David Hildenbrand wrote:
> 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.
Reviewed-by: Pavel Tatashin
[PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
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.
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: "Rafael J. Wysocki"
Cc: Len Brown
Cc: Greg Kroah-Hartman
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Stephen Hemminger
Cc: Martin Schwidefsky
Cc: Heiko Carstens
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Rashmica Gupta
Cc: Michael Neuling
Cc: Balbir Singh
Cc: Kate Stewart
Cc: Thomas Gleixner
Cc: Philippe Ombredanne
Cc: Andrew Morton
Cc: Michal Hocko
Cc: Pavel Tatashin
Cc: Vlastimil Babka
Cc: Dan Williams
Cc: Oscar Salvador
Cc: YASUAKI ISHIMATSU
Cc: Mathieu Malaterre
Signed-off-by: David Hildenbrand
---
drivers/base/memory.c | 13 +
mm/memory_hotplug.c | 28
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 5b0375be7f65..04be13539eb8 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -228,7 +228,6 @@ static bool pages_correctly_probed(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)
@@ -294,7 +293,6 @@ 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 */
@@ -341,19 +339,11 @@ 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(>dev);
break;
@@ -364,7 +354,6 @@ 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 e2b5c751e3ea..a2c6c87d83f3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -881,7 +881,6 @@ static struct zone * __meminit move_pfn_range(int
online_type, int nid,
return zone;
}
-/* Must be protected by mem_hotplug_begin() or
