Re: memory hotplug and force_remove

On Friday, March 31, 2017 02:02:36 PM Michal Hocko wrote:

On Fri 31-03-17 19:55:30, Joey Lee wrote:

quoted

On Fri, Mar 31, 2017 at 12:55:05PM +0200, Michal Hocko wrote:

quoted

On Fri 31-03-17 18:49:05, Joey Lee wrote:

quoted

Hi Michal,

On Fri, Mar 31, 2017 at 10:30:17AM +0200, Michal Hocko wrote:

[...]

quoted

quoted

@@ -241,11 +232,10 @@ static int acpi_scan_try_to_offline(struct acpi_device *device)
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
 				    NULL, acpi_bus_offline, (void *)true,
 				    (void **)&errdev);
-		if (!errdev || acpi_force_hot_remove)
+		if (!errdev)
 			acpi_bus_offline(handle, 0, (void *)true,
 					 (void **)&errdev);
-
-		if (errdev && !acpi_force_hot_remove) {
+		else {

              ^^^^^^^^^^^^^
Here should still checks the parent's errdev state then rollback
parent/children to online state:

-		if (errdev && !acpi_force_hot_remove) {
+		if (errdev) {

You are right, I have missed that acpi_bus_offline modifies errdev.
Thanks for spotting that! Updated patch is below.
---

quoted

From 8df0abd29988ffb52b6df52407b96d6015861bb7 Mon Sep 17 00:00:00 2001

From: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
Date: Fri, 31 Mar 2017 10:08:41 +0200
Subject: [PATCH] acpi: drop support for force_remove

/sys/firmware/acpi/hotplug/force_remove was presumably added to support
auto offlining in the past. This is, however, inherently dangerous for
some hotplugable resources like memory. The memory offlining fails when
the memory is still in use and cannot be dropped or migrated. If we
ignore the failure we are basically allowing for subtle memory
corruption or a crash.

We have actually noticed the later while hitting BUG() during the memory
hotremove (remove_memory):
	ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
			check_memblock_offlined_cb);
	if (ret)
		BUG();

it took us quite non-trivial time realize that the customer had
force_remove enabled. Even if the BUG was removed here and we could
propagate the error up the call chain it wouldn't help at all because
then we would hit a crash or a memory corruption later and harder to
debug. So force_remove is unfixable for the memory hotremove. We haven't
checked other hotplugable resources to be prone to a similar problems.

Remove the force_remove functionality because it is not fixable currently.
Keep the sysfs file and report an error if somebody tries to enable it.
Encourage users to report about the missing functionality and work with
them with an alternative solution.

Signed-off-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>

This patch is good to me. Please feel free to add:

Reviewed-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>

Thanks for the review Joey!

Can you please resend it with a CC to linux-acpi to give the people on that list
a chance to speak up if they have any concerns?

Thanks,
Rafael