ACPI / HOTPLUG: fix device->physical_node_lock deadlock
PATCH 1/1 ACPI / HOTPLUG: fix device->physical_node_lock deadlock
I meet a deadlock during cpu hotplug. The code path is bellow:
Call Trace: [<ffffffff816e373c>] dump_stack+0x19/0x1b [<ffffffff810fd85a>] validate_chain.isra.43+0xf4a/0x1120 [<ffffffff810236c9>] ? sched_clock+0x9/0x10 [<ffffffff810ca8bd>] ? sched_clock_local+0x1d/0x80 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810fe846>] __lock_acquire+0x3c6/0xb70 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810ff7e2>] lock_acquire+0xa2/0x1f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff816e7a14>] mutex_lock_nested+0x94/0x3f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff810fe0fd>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff813ba132>] acpi_scan_is_offline+0x2c/0xa3 --> LOCK (DEADLOCK) [<ffffffff813fdac8>] acpi_container_offline+0x32/0x4e [<ffffffff81469e59>] container_offline+0x19/0x20 [<ffffffff81462955>] device_offline+0x95/0xc0 [<ffffffff813b9e53>] acpi_bus_offline+0xbc/0x126 --> LOCK [<ffffffff813bb83d>] acpi_device_hotplug+0x236/0x46b [<ffffffff813b4c75>] acpi_hotplug_work_fn+0x1e/0x29 [<ffffffff810a6c10>] process_one_work+0x220/0x710 [<ffffffff810a6ba4>] ? process_one_work+0x1b4/0x710 [<ffffffff810a721b>] worker_thread+0x11b/0x3a0 [<ffffffff810a7100>] ? process_one_work+0x710/0x710 [<ffffffff810b061d>] kthread+0xed/0x100 [<ffffffff810b0530>] ? insert_kthread_work+0x80/0x80 [<ffffffff816f663c>] ret_from_fork+0x7c/0xb0 [<ffffffff810b0530>] ? insert_kthread_work+0x80/0x80
This deadlock was introduced by commit caa73ea
("ACPI / hotplug / driver core: Handle containers in a special way").
In this patch, we just introduced a lockless version __acpi_scan_is_offline() for acpi_container_offline(), to avoid this deadlock.
--- drivers/acpi/container.c | 2 +- drivers/acpi/internal.h | 1 + drivers/acpi/scan.c | 15 ++++++++++++--- 3 files changed, 14 insertions(+), 4 deletions(-)
drivers/acpi/container.c +1 -1
@@ -50,7 +50,7 @@ static int acpi_container_offline(struct container_dev *cdev)/* Check all of the dependent devices' physical companions. */ list_for_each_entry (child, &adev->children, node) -if (!acpi_scan_is_offline (child, false)) +if (!__acpi_scan_is_offline (child, false))return -EBUSY ;return 0 ;
drivers/acpi/internal.h +1 -0
@@ -80,6 +80,7 @@ void acpi_apd_init(void);acpi_status acpi_hotplug_schedule (struct acpi_device *adev,u32 src);bool acpi_queue_hotplug_work (struct work_struct *work);void acpi_device_hotplug (struct acpi_device *adev,u32 src); +bool __acpi_scan_is_offline (struct acpi_device *adev,bool uevent);bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent);/* --------------------------------------------------------------------------
drivers/acpi/scan.c +12 -3
@@ -293,13 +293,12 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha } static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL); -bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent) +/* Must be called under physical_node_lock. */ +bool __acpi_scan_is_offline (struct acpi_device *adev,bool uevent) {struct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); -list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {if (uevent)
@@ -309,7 +308,17 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)break ; } +return offline; +} + +bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent) +{ +bool offline = true; + +mutex_lock (&adev->physical_node_lock); + offline =__acpi_scan_is_offline (adev, uevent);mutex_unlock (&adev->physical_node_lock); +return offline; }
Signature · 1 line
1.8.3.1
Quoted 17 lines · Xie XiuQi
I meet a deadlock during cpu hotplug. The code path is bellow:
Call Trace: [<ffffffff816e373c>] dump_stack+0x19/0x1b [<ffffffff810fd85a>] validate_chain.isra.43+0xf4a/0x1120 [<ffffffff810236c9>] ? sched_clock+0x9/0x10 [<ffffffff810ca8bd>] ? sched_clock_local+0x1d/0x80 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810fe846>] __lock_acquire+0x3c6/0xb70 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810ff7e2>] lock_acquire+0xa2/0x1f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff816e7a14>] mutex_lock_nested+0x94/0x3f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff810fe0fd>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff813ba132>] acpi_scan_is_offline+0x2c/0xa3 --> LOCK (DEADLOCK)
Is it the same device, actually? acpi_container_offline() walks the *children* of the container while acpi_bus_offline() locks the container itself.
Is it not the case?
Quoted 15 lines
[<ffffffff813fdac8>] acpi_container_offline+0x32/0x4e [<ffffffff81469e59>] container_offline+0x19/0x20 [<ffffffff81462955>] device_offline+0x95/0xc0 [<ffffffff813b9e53>] acpi_bus_offline+0xbc/0x126 --> LOCK [<ffffffff813bb83d>] acpi_device_hotplug+0x236/0x46b [<ffffffff813b4c75>] acpi_hotplug_work_fn+0x1e/0x29 [<ffffffff810a6c10>] process_one_work+0x220/0x710 [<ffffffff810a6ba4>] ? process_one_work+0x1b4/0x710 [<ffffffff810a721b>] worker_thread+0x11b/0x3a0 [<ffffffff810a7100>] ? process_one_work+0x710/0x710 [<ffffffff810b061d>] kthread+0xed/0x100 [<ffffffff810b0530>] ? insert_kthread_work+0x80/0x80 [<ffffffff816f663c>] ret_from_fork+0x7c/0xb0 [<ffffffff810b0530>] ? insert_kthread_work+0x80/0x80
Quoted 5 lines
This deadlock was introduced by commit caa73ea
("ACPI / hotplug / driver core: Handle containers in a special way").In this patch, we just introduced a lockless version
__acpi_scan_is_offline()foracpi_container_offline(), to avoid this deadlock.
So why is this a correct approach? Why can acpi_container_offline() suddenly call __acpi_scan_is_offline() without the lock?
Quoted 72 lines
Cc <[email protected]> # v3.14+Signed-off-by Xie XiuQi <[email protected]>--- drivers/acpi/container.c | 2 +- drivers/acpi/internal.h | 1 + drivers/acpi/scan.c | 15 ++++++++++++--- 3 files changed, 14 insertions(+), 4 deletions(-)drivers/acpi/container.c +1
-1@@ -50,7 +50,7 @@ static int acpi_container_offline(struct container_dev *cdev)/* Check all of the dependent devices' physical companions. */ list_for_each_entry (child, &adev->children, node) -if (!acpi_scan_is_offline (child, false)) +if (!__acpi_scan_is_offline (child, false))return -EBUSY ;return 0 ;drivers/acpi/internal.h +1
-0@@ -80,6 +80,7 @@ void acpi_apd_init(void);acpi_status acpi_hotplug_schedule (struct acpi_device *adev,u32 src);bool acpi_queue_hotplug_work (struct work_struct *work);void acpi_device_hotplug (struct acpi_device *adev,u32 src); +bool __acpi_scan_is_offline (struct acpi_device *adev,bool uevent);bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent);/* -------------------------------------------------------------------------- drivers/acpi/scan.c +12
-3@@ -293,13 +293,12 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha } static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL); -bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent) +/* Must be called under physical_node_lock. */ +bool __acpi_scan_is_offline (struct acpi_device *adev,bool uevent) {struct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); -list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {if (uevent)@@ -309,7 +308,17 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent)break ; } +return offline; +} + +bool acpi_scan_is_offline (struct acpi_device *adev,bool uevent) +{ +bool offline = true; + +mutex_lock (&adev->physical_node_lock); + offline =__acpi_scan_is_offline (adev, uevent);mutex_unlock (&adev->physical_node_lock); +return offline; }
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 19 lines · Rafael J. Wysocki
Quoted 17 lines · Xie XiuQi
I meet a deadlock during cpu hotplug. The code path is bellow:
Call Trace: [<ffffffff816e373c>] dump_stack+0x19/0x1b [<ffffffff810fd85a>] validate_chain.isra.43+0xf4a/0x1120 [<ffffffff810236c9>] ? sched_clock+0x9/0x10 [<ffffffff810ca8bd>] ? sched_clock_local+0x1d/0x80 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810fe846>] __lock_acquire+0x3c6/0xb70 [<ffffffff810caa88>] ? sched_clock_cpu+0xa8/0x100 [<ffffffff810ff7e2>] lock_acquire+0xa2/0x1f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff816e7a14>] mutex_lock_nested+0x94/0x3f0 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff813ba132>] ? acpi_scan_is_offline+0x2c/0xa3 [<ffffffff810fe0fd>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff813ba132>] acpi_scan_is_offline+0x2c/0xa3 --> LOCK (DEADLOCK)
Quoted 2 lines
Is it the same device, actually?
acpi_container_offline()walks the *children* of the container whileacpi_bus_offline()locks the container itself.
So the patch below should make the splat go away too if I'm not mistaken.
--- drivers/acpi/scan.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/acpi/scan.c +5 -1
@@ -298,7 +298,11 @@ bool acpi_scan_is_offline(struct acpi_destruct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); +/* +* acpi_container_offline() calls this for all of the container's +* children under the container's physical_node_lock lock. +*/ +mutex_lock_nested (&adev->physical_node_lock,SINGLE_DEPTH_NESTING );list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {
PATCH 1/1 ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline() may be called under the physical_node_lock of the given device object's parent, so prevent lockdep from complaining about that by annotating that instance with SINGLE_DEPTH_NESTING.
--- drivers/acpi/scan.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/acpi/scan.c +5 -1
@@ -298,7 +298,11 @@ bool acpi_scan_is_offline(struct acpi_destruct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); +/* +* acpi_container_offline() calls this for all of the container's +* children under the container's physical_node_lock lock. +*/ +mutex_lock_nested (&adev->physical_node_lock,SINGLE_DEPTH_NESTING );list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {
Re: [PATCH] ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
Quoted 9 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline()may be called under thephysical_node_lockof the given device object's parent, so prevent lockdep from complaining about that by annotating that instance withSINGLE_DEPTH_NESTING.Reported-by Xie XiuQi <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Thank you for your patch, I'll test soon.
Thanks,
Xie XiuQi
Quoted 25 lines
--- drivers/acpi/scan.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/acpi/scan.c
===================================================================drivers/acpi/scan.c +5
-1@@ -298,7 +298,11 @@ bool acpi_scan_is_offline(struct acpi_destruct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); +/* +* acpi_container_offline() calls this for all of the container's +* children under the container's physical_node_lock lock. +*/ +mutex_lock_nested (&adev->physical_node_lock,SINGLE_DEPTH_NESTING );list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {.
Re: [PATCH] ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
In reply to Rafael J. WysockiQuoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline()may be called under thephysical_node_lockof the given device object's parent, so prevent lockdep from complaining about that by annotating that instance withSINGLE_DEPTH_NESTING.
I think this is trigged by setting acpi_force_hot_remove to 1, in acpi_scan_hot_remove():
if (device->handler && device->handler->hotplug.demand_offline
&& !acpi_force_hot_remove) {
if (!acpi_scan_is_offline(device, true))
return -EBUSY;
} else {
int error = acpi_scan_try_to_offline(device);
if (error)
return error;
}then the container device will be removed by acpi_scan_try_to_offline(), let's wait for Xiuqi's test result.
Thanks
Hanjun
Quoted 25 lines
Reported-by Xie XiuQi <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/acpi/scan.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/acpi/scan.c
===================================================================drivers/acpi/scan.c +5
-1@@ -298,7 +298,11 @@ bool acpi_scan_is_offline(struct acpi_destruct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); +/* +* acpi_container_offline() calls this for all of the container's +* children under the container's physical_node_lock lock. +*/ +mutex_lock_nested (&adev->physical_node_lock,SINGLE_DEPTH_NESTING );list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {
Re: [PATCH] ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
Quoted 21 lines · Hanjun Guo
Quoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline()may be called under thephysical_node_lockof the given device object's parent, so prevent lockdep from complaining about that by annotating that instance withSINGLE_DEPTH_NESTING.I think this is trigged by setting
acpi_force_hot_removeto 1, inacpi_scan_hot_remove():if (device->handler && device->handler->hotplug.demand_offline && !acpi_force_hot_remove) { if (!acpi_scan_is_offline(device, true)) return -EBUSY; } else { int error = acpi_scan_try_to_offline(device); if (error) return error; }
Quoted 2 lines
then the container device will be removed by
acpi_scan_try_to_offline(), let's wait for Xiuqi's test result.
I'm not sure what you mean. demand_offline is 'true' for containers, so acpi_force_hot_remove doesn't matter here.
Signature · 6 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
In reply to Rafael J. WysockiQuoted 9 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline()may be called under thephysical_node_lockof the given device object's parent, so prevent lockdep from complaining about that by annotating that instance withSINGLE_DEPTH_NESTING.Reported-by Xie XiuQi <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Agreed that the issue is likely with the lock-class. Assuming Xie's test goes well (we will need to wait for that :),
Thanks,
-Toshi
Re: [PATCH] ACPI / scan: Annotate physical_node_lock in acpi_scan_is_offline()
In reply to Rafael J. WysockiQuoted 9 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
acpi_scan_is_offline()may be called under thephysical_node_lockof the given device object's parent, so prevent lockdep from complaining about that by annotating that instance withSINGLE_DEPTH_NESTING.Reported-by Xie XiuQi <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Thanks,
Xie XiuQi
Quoted 25 lines
--- drivers/acpi/scan.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/acpi/scan.c
===================================================================drivers/acpi/scan.c +5
-1@@ -298,7 +298,11 @@ bool acpi_scan_is_offline(struct acpi_destruct acpi_device_physical_node *pn;bool offline = true; -mutex_lock (&adev->physical_node_lock); +/* +* acpi_container_offline() calls this for all of the container's +* children under the container's physical_node_lock lock. +*/ +mutex_lock_nested (&adev->physical_node_lock,SINGLE_DEPTH_NESTING );list_for_each_entry (pn, &adev->physical_node_list, node)if (device_supports_offline (pn->dev) && !pn->dev->offline) {.