Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1596044 > unrolled thread

[PATCH 1/2] mm: add private lock to serialize memory hotplug operations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] mm: add private lock to serialize memory hotplug operations Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-03-09 14:10 +0100
    Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-09 14:50 +0100
      Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations Dan Williams <dan.j.williams@intel.com> - 2017-03-09 19:20 +0100
        Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-09 23:30 +0100
          Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations Dan Williams <dan.j.williams@intel.com> - 2017-03-09 23:40 +0100
            Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-09 23:50 +0100
        Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-09 23:30 +0100
          Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations Dan Williams <dan.j.williams@intel.com> - 2017-03-09 23:40 +0100
            Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-09 23:50 +0100
              Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug  operations Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-03-13 20:00 +0100
                Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations Dan Williams <dan.j.williams@intel.com> - 2017-03-13 20:50 +0100
                  Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-13 22:30 +0100

#1596044 — [PATCH 1/2] mm: add private lock to serialize memory hotplug operations

Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
introduced new functions get/put_online_mems() and
mem_hotplug_begin/end() in order to allow similar semantics for memory
hotplug like for cpu hotplug.

The corresponding functions for cpu hotplug are get/put_online_cpus()
and cpu_hotplug_begin/done() for cpu hotplug.

The commit however missed to introduce functions that would serialize
memory hotplug operations like they are done for cpu hotplug with
cpu_maps_update_begin/done().

This basically leaves mem_hotplug.active_writer unprotected and allows
concurrent writers to modify it, which may lead to problems as
outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
use mem_hotplug_{begin, done}").

That commit was extended again with commit b5d24fda9c3d ("mm,
devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
done}") which serializes memory hotplug operations for some call
sites by using the device_hotplug lock.

In addition with commit 3fc21924100b ("mm: validate device_hotplug is
held for memory hotplug") a sanity check was added to
mem_hotplug_begin() to verify that the device_hotplug lock is held.

This in turn triggers the following warning on s390:

WARNING: CPU: 6 PID: 1 at drivers/base/core.c:643 assert_held_device_hotplug+0x4a/0x58
 Call Trace:
  assert_held_device_hotplug+0x40/0x58)
  mem_hotplug_begin+0x34/0xc8
  add_memory_resource+0x7e/0x1f8
  add_memory+0xda/0x130
  add_memory_merged+0x15c/0x178
  sclp_detect_standby_memory+0x2ae/0x2f8
  do_one_initcall+0xa2/0x150
  kernel_init_freeable+0x228/0x2d8
  kernel_init+0x2a/0x140
  kernel_thread_starter+0x6/0xc

One possible fix would be to add more lock_device_hotplug() and
unlock_device_hotplug() calls around each call site of
mem_hotplug_begin/end(). But that would give the device_hotplug lock
additional semantics it better should not have (serialize memory
hotplug operations).

Instead add a new memory_add_remove_lock which has the similar
semantics like cpu_add_remove_lock for cpu hotplug.

To keep things hopefully a bit easier the lock will be locked and
unlocked within the mem_hotplug_begin/end() functions.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/memremap.c   | 4 ----
 mm/memory_hotplug.c | 6 +++++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/memremap.c b/kernel/memremap.c
index 06123234f118..07e85e5229da 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -247,11 +247,9 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
 	align_start = res->start & ~(SECTION_SIZE - 1);
 	align_size = ALIGN(resource_size(res), SECTION_SIZE);
 
-	lock_device_hotplug();
 	mem_hotplug_begin();
 	arch_remove_memory(align_start, align_size);
 	mem_hotplug_done();
-	unlock_device_hotplug();
 
 	untrack_pfn(NULL, PHYS_PFN(align_start), align_size);
 	pgmap_radix_release(res);
@@ -364,11 +362,9 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
 	if (error)
 		goto err_pfn_remap;
 
-	lock_device_hotplug();
 	mem_hotplug_begin();
 	error = arch_add_memory(nid, align_start, align_size, true);
 	mem_hotplug_done();
-	unlock_device_hotplug();
 	if (error)
 		goto err_add_memory;
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 295479b792ec..6fa7208bcd56 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -125,9 +125,12 @@ void put_online_mems(void)
 
 }
 
+/* Serializes write accesses to mem_hotplug.active_writer. */
+static DEFINE_MUTEX(memory_add_remove_lock);
+
 void mem_hotplug_begin(void)
 {
-	assert_held_device_hotplug();
+	mutex_lock(&memory_add_remove_lock);
 
 	mem_hotplug.active_writer = current;
 
@@ -147,6 +150,7 @@ void mem_hotplug_done(void)
 	mem_hotplug.active_writer = NULL;
 	mutex_unlock(&mem_hotplug.lock);
 	memhp_lock_release();
+	mutex_unlock(&memory_add_remove_lock);
 }
 
 /* add this memory to iomem resource */
-- 
2.8.4

[toc] | [next] | [standalone]

#1596075

On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
> introduced new functions get/put_online_mems() and
> mem_hotplug_begin/end() in order to allow similar semantics for memory
> hotplug like for cpu hotplug.
> 
> The corresponding functions for cpu hotplug are get/put_online_cpus()
> and cpu_hotplug_begin/done() for cpu hotplug.
> 
> The commit however missed to introduce functions that would serialize
> memory hotplug operations like they are done for cpu hotplug with
> cpu_maps_update_begin/done().
> 
> This basically leaves mem_hotplug.active_writer unprotected and allows
> concurrent writers to modify it, which may lead to problems as
> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
> use mem_hotplug_{begin, done}").
> 
> That commit was extended again with commit b5d24fda9c3d ("mm,
> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
> done}") which serializes memory hotplug operations for some call
> sites by using the device_hotplug lock.
> 
> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
> held for memory hotplug") a sanity check was added to
> mem_hotplug_begin() to verify that the device_hotplug lock is held.

Admittedly, I haven't looked at all of the code paths involved in detail yet,
but there's one concern regarding lock/unlock_device_hotplug().

The actual main purpose of it is to ensure safe removal of devices in cases
when they cannot be removed separately, like when a whole CPU package
(including possibly an entire NUMA node with memory and all) is removed.

One of the code paths doing that is acpi_scan_hot_remove() which first
tries to offline devices slated for removal and then finally removes them.

The reason why this needs to be done in two stages is because the offlining
can fail, in which case we will fail the entire operation, while the final
removal step is, well, final (meaning that the devices are gone after it no
matter what).

This is done under device_hotplug_lock, so that the devices that were taken
offline in stage 1 cannot be brought back online before stage 2 is carried
out entirely, which surely would be bad if it happened.

Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
question actually affects this mechanism, but this in case it does, it is one
thing to double check before going ahead with this patch.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1596295

On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
>> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
>> introduced new functions get/put_online_mems() and
>> mem_hotplug_begin/end() in order to allow similar semantics for memory
>> hotplug like for cpu hotplug.
>>
>> The corresponding functions for cpu hotplug are get/put_online_cpus()
>> and cpu_hotplug_begin/done() for cpu hotplug.
>>
>> The commit however missed to introduce functions that would serialize
>> memory hotplug operations like they are done for cpu hotplug with
>> cpu_maps_update_begin/done().
>>
>> This basically leaves mem_hotplug.active_writer unprotected and allows
>> concurrent writers to modify it, which may lead to problems as
>> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
>> use mem_hotplug_{begin, done}").
>>
>> That commit was extended again with commit b5d24fda9c3d ("mm,
>> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
>> done}") which serializes memory hotplug operations for some call
>> sites by using the device_hotplug lock.
>>
>> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
>> held for memory hotplug") a sanity check was added to
>> mem_hotplug_begin() to verify that the device_hotplug lock is held.
>
> Admittedly, I haven't looked at all of the code paths involved in detail yet,
> but there's one concern regarding lock/unlock_device_hotplug().
>
> The actual main purpose of it is to ensure safe removal of devices in cases
> when they cannot be removed separately, like when a whole CPU package
> (including possibly an entire NUMA node with memory and all) is removed.
>
> One of the code paths doing that is acpi_scan_hot_remove() which first
> tries to offline devices slated for removal and then finally removes them.
>
> The reason why this needs to be done in two stages is because the offlining
> can fail, in which case we will fail the entire operation, while the final
> removal step is, well, final (meaning that the devices are gone after it no
> matter what).
>
> This is done under device_hotplug_lock, so that the devices that were taken
> offline in stage 1 cannot be brought back online before stage 2 is carried
> out entirely, which surely would be bad if it happened.
>
> Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
> question actually affects this mechanism, but this in case it does, it is one
> thing to double check before going ahead with this patch.
>

I *think* we're ok in this case because unplugging the CPU package
that contains a persistent memory device will trigger
devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
device can't fail. It may be held off while pages are pinned for DMA
memory, but it will eventually complete.

[toc] | [prev] | [next] | [standalone]

#1596428

On Thursday, March 09, 2017 11:15:47 PM Rafael J. Wysocki wrote:
> On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
> > On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
> > >> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
> > >> introduced new functions get/put_online_mems() and
> > >> mem_hotplug_begin/end() in order to allow similar semantics for memory
> > >> hotplug like for cpu hotplug.
> > >>
> > >> The corresponding functions for cpu hotplug are get/put_online_cpus()
> > >> and cpu_hotplug_begin/done() for cpu hotplug.
> > >>
> > >> The commit however missed to introduce functions that would serialize
> > >> memory hotplug operations like they are done for cpu hotplug with
> > >> cpu_maps_update_begin/done().
> > >>
> > >> This basically leaves mem_hotplug.active_writer unprotected and allows
> > >> concurrent writers to modify it, which may lead to problems as
> > >> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
> > >> use mem_hotplug_{begin, done}").
> > >>
> > >> That commit was extended again with commit b5d24fda9c3d ("mm,
> > >> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
> > >> done}") which serializes memory hotplug operations for some call
> > >> sites by using the device_hotplug lock.
> > >>
> > >> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
> > >> held for memory hotplug") a sanity check was added to
> > >> mem_hotplug_begin() to verify that the device_hotplug lock is held.
> > >
> > > Admittedly, I haven't looked at all of the code paths involved in detail yet,
> > > but there's one concern regarding lock/unlock_device_hotplug().
> > >
> > > The actual main purpose of it is to ensure safe removal of devices in cases
> > > when they cannot be removed separately, like when a whole CPU package
> > > (including possibly an entire NUMA node with memory and all) is removed.
> > >
> > > One of the code paths doing that is acpi_scan_hot_remove() which first
> > > tries to offline devices slated for removal and then finally removes them.
> > >
> > > The reason why this needs to be done in two stages is because the offlining
> > > can fail, in which case we will fail the entire operation, while the final
> > > removal step is, well, final (meaning that the devices are gone after it no
> > > matter what).
> > >
> > > This is done under device_hotplug_lock, so that the devices that were taken
> > > offline in stage 1 cannot be brought back online before stage 2 is carried
> > > out entirely, which surely would be bad if it happened.
> > >
> > > Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
> > > question actually affects this mechanism, but this in case it does, it is one
> > > thing to double check before going ahead with this patch.
> > >
> > 
> > I *think* we're ok in this case because unplugging the CPU package
> > that contains a persistent memory device will trigger
> > devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
> > device can't fail. It may be held off while pages are pinned for DMA
> > memory, but it will eventually complete.
> 
> What about the offlining, though?  Is it guaranteed that no memory from those
> ranges will go back online after the acpi_scan_try_to_offline() call in
> acpi_scan_hot_remove()?

My point is that after the acpi_evaluate_ej0() in acpi_scan_hot_remove() the
hardware is physically gone, so if anything is still doing DMA to that memory at
that point, then the user is going to be unhappy.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1596437

On Thu, Mar 9, 2017 at 2:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 09, 2017 11:15:47 PM Rafael J. Wysocki wrote:
>> On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
>> > On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
[..]
>> > I *think* we're ok in this case because unplugging the CPU package
>> > that contains a persistent memory device will trigger
>> > devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
>> > device can't fail. It may be held off while pages are pinned for DMA
>> > memory, but it will eventually complete.
>>
>> What about the offlining, though?  Is it guaranteed that no memory from those
>> ranges will go back online after the acpi_scan_try_to_offline() call in
>> acpi_scan_hot_remove()?
>
> My point is that after the acpi_evaluate_ej0() in acpi_scan_hot_remove() the
> hardware is physically gone, so if anything is still doing DMA to that memory at
> that point, then the user is going to be unhappy.

Hmm, ACPI 6.1 does not have any text about what _EJ0 means for ACPI0012.

[toc] | [prev] | [next] | [standalone]

#1596440

On Thursday, March 09, 2017 02:37:55 PM Dan Williams wrote:
> On Thu, Mar 9, 2017 at 2:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 09, 2017 11:15:47 PM Rafael J. Wysocki wrote:
> >> On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
> >> > On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> [..]
> >> > I *think* we're ok in this case because unplugging the CPU package
> >> > that contains a persistent memory device will trigger
> >> > devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
> >> > device can't fail. It may be held off while pages are pinned for DMA
> >> > memory, but it will eventually complete.
> >>
> >> What about the offlining, though?  Is it guaranteed that no memory from those
> >> ranges will go back online after the acpi_scan_try_to_offline() call in
> >> acpi_scan_hot_remove()?
> >
> > My point is that after the acpi_evaluate_ej0() in acpi_scan_hot_remove() the
> > hardware is physically gone, so if anything is still doing DMA to that memory at
> > that point, then the user is going to be unhappy.
> 
> Hmm, ACPI 6.1 does not have any text about what _EJ0 means for ACPI0012.

ACPI0012 is exceptional, but in general _EJ0 does not have to be present under
a particular device for it to be affected.  It can be under the device's parent, for
example, in which case the entire subtree under a device with _EJ0 goes away in
one go.  And that very well may mean disconnect at the physical level (voltage
goes away IOW).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1596430

On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
> On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
> >> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
> >> introduced new functions get/put_online_mems() and
> >> mem_hotplug_begin/end() in order to allow similar semantics for memory
> >> hotplug like for cpu hotplug.
> >>
> >> The corresponding functions for cpu hotplug are get/put_online_cpus()
> >> and cpu_hotplug_begin/done() for cpu hotplug.
> >>
> >> The commit however missed to introduce functions that would serialize
> >> memory hotplug operations like they are done for cpu hotplug with
> >> cpu_maps_update_begin/done().
> >>
> >> This basically leaves mem_hotplug.active_writer unprotected and allows
> >> concurrent writers to modify it, which may lead to problems as
> >> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
> >> use mem_hotplug_{begin, done}").
> >>
> >> That commit was extended again with commit b5d24fda9c3d ("mm,
> >> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
> >> done}") which serializes memory hotplug operations for some call
> >> sites by using the device_hotplug lock.
> >>
> >> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
> >> held for memory hotplug") a sanity check was added to
> >> mem_hotplug_begin() to verify that the device_hotplug lock is held.
> >
> > Admittedly, I haven't looked at all of the code paths involved in detail yet,
> > but there's one concern regarding lock/unlock_device_hotplug().
> >
> > The actual main purpose of it is to ensure safe removal of devices in cases
> > when they cannot be removed separately, like when a whole CPU package
> > (including possibly an entire NUMA node with memory and all) is removed.
> >
> > One of the code paths doing that is acpi_scan_hot_remove() which first
> > tries to offline devices slated for removal and then finally removes them.
> >
> > The reason why this needs to be done in two stages is because the offlining
> > can fail, in which case we will fail the entire operation, while the final
> > removal step is, well, final (meaning that the devices are gone after it no
> > matter what).
> >
> > This is done under device_hotplug_lock, so that the devices that were taken
> > offline in stage 1 cannot be brought back online before stage 2 is carried
> > out entirely, which surely would be bad if it happened.
> >
> > Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
> > question actually affects this mechanism, but this in case it does, it is one
> > thing to double check before going ahead with this patch.
> >
> 
> I *think* we're ok in this case because unplugging the CPU package
> that contains a persistent memory device will trigger
> devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
> device can't fail. It may be held off while pages are pinned for DMA
> memory, but it will eventually complete.

What about the offlining, though?  Is it guaranteed that no memory from those
ranges will go back online after the acpi_scan_try_to_offline() call in
acpi_scan_hot_remove()?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1596436

On Thu, Mar 9, 2017 at 2:15 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
>> On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
>> >> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
>> >> introduced new functions get/put_online_mems() and
>> >> mem_hotplug_begin/end() in order to allow similar semantics for memory
>> >> hotplug like for cpu hotplug.
>> >>
>> >> The corresponding functions for cpu hotplug are get/put_online_cpus()
>> >> and cpu_hotplug_begin/done() for cpu hotplug.
>> >>
>> >> The commit however missed to introduce functions that would serialize
>> >> memory hotplug operations like they are done for cpu hotplug with
>> >> cpu_maps_update_begin/done().
>> >>
>> >> This basically leaves mem_hotplug.active_writer unprotected and allows
>> >> concurrent writers to modify it, which may lead to problems as
>> >> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
>> >> use mem_hotplug_{begin, done}").
>> >>
>> >> That commit was extended again with commit b5d24fda9c3d ("mm,
>> >> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
>> >> done}") which serializes memory hotplug operations for some call
>> >> sites by using the device_hotplug lock.
>> >>
>> >> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
>> >> held for memory hotplug") a sanity check was added to
>> >> mem_hotplug_begin() to verify that the device_hotplug lock is held.
>> >
>> > Admittedly, I haven't looked at all of the code paths involved in detail yet,
>> > but there's one concern regarding lock/unlock_device_hotplug().
>> >
>> > The actual main purpose of it is to ensure safe removal of devices in cases
>> > when they cannot be removed separately, like when a whole CPU package
>> > (including possibly an entire NUMA node with memory and all) is removed.
>> >
>> > One of the code paths doing that is acpi_scan_hot_remove() which first
>> > tries to offline devices slated for removal and then finally removes them.
>> >
>> > The reason why this needs to be done in two stages is because the offlining
>> > can fail, in which case we will fail the entire operation, while the final
>> > removal step is, well, final (meaning that the devices are gone after it no
>> > matter what).
>> >
>> > This is done under device_hotplug_lock, so that the devices that were taken
>> > offline in stage 1 cannot be brought back online before stage 2 is carried
>> > out entirely, which surely would be bad if it happened.
>> >
>> > Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
>> > question actually affects this mechanism, but this in case it does, it is one
>> > thing to double check before going ahead with this patch.
>> >
>>
>> I *think* we're ok in this case because unplugging the CPU package
>> that contains a persistent memory device will trigger
>> devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
>> device can't fail. It may be held off while pages are pinned for DMA
>> memory, but it will eventually complete.
>
> What about the offlining, though?  Is it guaranteed that no memory from those
> ranges will go back online after the acpi_scan_try_to_offline() call in
> acpi_scan_hot_remove()?

The memory described by devm_memremap_pages() is never "onlined" to
the core mm. We're only using arch_add_memory() to get a linear
mapping and page structures. The rest of memory hotplug is skipped,
and this ZONE_DEVICE memory is otherwise hidden from the core mm.

Are ACPI devices disabled by this point? For example, If we have
disabled the nfit bus device (_HID ACPI0012) then the associated child
pmem device(s) will be gone and not coming back.

Now, that said, the ACPI0012 bus device is global for the entire
system. So we'd need more plumbing to target the pmem on a given
socket without touching the others.

[toc] | [prev] | [next] | [standalone]

#1596443

On Thursday, March 09, 2017 02:33:43 PM Dan Williams wrote:
> On Thu, Mar 9, 2017 at 2:15 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 09, 2017 10:10:31 AM Dan Williams wrote:
> >> On Thu, Mar 9, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Thursday, March 09, 2017 02:06:15 PM Heiko Carstens wrote:
> >> >> Commit bfc8c90139eb ("mem-hotplug: implement get/put_online_mems")
> >> >> introduced new functions get/put_online_mems() and
> >> >> mem_hotplug_begin/end() in order to allow similar semantics for memory
> >> >> hotplug like for cpu hotplug.
> >> >>
> >> >> The corresponding functions for cpu hotplug are get/put_online_cpus()
> >> >> and cpu_hotplug_begin/done() for cpu hotplug.
> >> >>
> >> >> The commit however missed to introduce functions that would serialize
> >> >> memory hotplug operations like they are done for cpu hotplug with
> >> >> cpu_maps_update_begin/done().
> >> >>
> >> >> This basically leaves mem_hotplug.active_writer unprotected and allows
> >> >> concurrent writers to modify it, which may lead to problems as
> >> >> outlined by commit f931ab479dd2 ("mm: fix devm_memremap_pages crash,
> >> >> use mem_hotplug_{begin, done}").
> >> >>
> >> >> That commit was extended again with commit b5d24fda9c3d ("mm,
> >> >> devm_memremap_pages: hold device_hotplug lock over mem_hotplug_{begin,
> >> >> done}") which serializes memory hotplug operations for some call
> >> >> sites by using the device_hotplug lock.
> >> >>
> >> >> In addition with commit 3fc21924100b ("mm: validate device_hotplug is
> >> >> held for memory hotplug") a sanity check was added to
> >> >> mem_hotplug_begin() to verify that the device_hotplug lock is held.
> >> >
> >> > Admittedly, I haven't looked at all of the code paths involved in detail yet,
> >> > but there's one concern regarding lock/unlock_device_hotplug().
> >> >
> >> > The actual main purpose of it is to ensure safe removal of devices in cases
> >> > when they cannot be removed separately, like when a whole CPU package
> >> > (including possibly an entire NUMA node with memory and all) is removed.
> >> >
> >> > One of the code paths doing that is acpi_scan_hot_remove() which first
> >> > tries to offline devices slated for removal and then finally removes them.
> >> >
> >> > The reason why this needs to be done in two stages is because the offlining
> >> > can fail, in which case we will fail the entire operation, while the final
> >> > removal step is, well, final (meaning that the devices are gone after it no
> >> > matter what).
> >> >
> >> > This is done under device_hotplug_lock, so that the devices that were taken
> >> > offline in stage 1 cannot be brought back online before stage 2 is carried
> >> > out entirely, which surely would be bad if it happened.
> >> >
> >> > Now, I'm not sure if removing lock/unlock_device_hotplug() from the code in
> >> > question actually affects this mechanism, but this in case it does, it is one
> >> > thing to double check before going ahead with this patch.
> >> >
> >>
> >> I *think* we're ok in this case because unplugging the CPU package
> >> that contains a persistent memory device will trigger
> >> devm_memremap_pages() to call arch_remove_memory(). Removing a pmem
> >> device can't fail. It may be held off while pages are pinned for DMA
> >> memory, but it will eventually complete.
> >
> > What about the offlining, though?  Is it guaranteed that no memory from those
> > ranges will go back online after the acpi_scan_try_to_offline() call in
> > acpi_scan_hot_remove()?
> 
> The memory described by devm_memremap_pages() is never "onlined" to
> the core mm. We're only using arch_add_memory() to get a linear
> mapping and page structures. The rest of memory hotplug is skipped,
> and this ZONE_DEVICE memory is otherwise hidden from the core mm.

OK, that should be fine then.

> Are ACPI devices disabled by this point? For example, If we have
> disabled the nfit bus device (_HID ACPI0012) then the associated child
> pmem device(s) will be gone and not coming back.

We call acpi_bus_trim() on the root of the subtree in question before calling
acpi_evaluat_ej0(), so the driver's ->remove() should be called before that,
but it can't leave any delayed works behind.

> Now, that said, the ACPI0012 bus device is global for the entire
> system. So we'd need more plumbing to target the pmem on a given
> socket without touching the others.

Well, it's all a bit academic at this point AFAICS.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1599695 — Re: [PATCH 1/2] mm: add private lock to serialize memory hotplug operations

On Thu, Mar 09, 2017 at 11:34:44PM +0100, Rafael J. Wysocki wrote:
> > The memory described by devm_memremap_pages() is never "onlined" to
> > the core mm. We're only using arch_add_memory() to get a linear
> > mapping and page structures. The rest of memory hotplug is skipped,
> > and this ZONE_DEVICE memory is otherwise hidden from the core mm.
> 
> OK, that should be fine then.

So, does that mean that the patch is ok as it is? If so, it would be good
to get an Ack from both, you and Dan, please.

Thanks,
Heiko

[toc] | [prev] | [next] | [standalone]

#1599739

On Mon, Mar 13, 2017 at 11:57 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Thu, Mar 09, 2017 at 11:34:44PM +0100, Rafael J. Wysocki wrote:
>> > The memory described by devm_memremap_pages() is never "onlined" to
>> > the core mm. We're only using arch_add_memory() to get a linear
>> > mapping and page structures. The rest of memory hotplug is skipped,
>> > and this ZONE_DEVICE memory is otherwise hidden from the core mm.
>>
>> OK, that should be fine then.
>
> So, does that mean that the patch is ok as it is? If so, it would be good
> to get an Ack from both, you and Dan, please.

Acked-by: Dan Williams <dan.j.williams@intel.com>

[toc] | [prev] | [next] | [standalone]

#1599835

On Monday, March 13, 2017 12:44:25 PM Dan Williams wrote:
> On Mon, Mar 13, 2017 at 11:57 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > On Thu, Mar 09, 2017 at 11:34:44PM +0100, Rafael J. Wysocki wrote:
> >> > The memory described by devm_memremap_pages() is never "onlined" to
> >> > the core mm. We're only using arch_add_memory() to get a linear
> >> > mapping and page structures. The rest of memory hotplug is skipped,
> >> > and this ZONE_DEVICE memory is otherwise hidden from the core mm.
> >>
> >> OK, that should be fine then.
> >
> > So, does that mean that the patch is ok as it is? If so, it would be good
> > to get an Ack from both, you and Dan, please.
> 
> Acked-by: Dan Williams <dan.j.williams@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web