acpi : cpu hot-remove returns error when cpu_down() fails
Subsystems (2)
PATCH 1/2 acpi : cpu hot-remove returns error when cpu_down() fails
Even if cpu_down() fails, acpi_processor_remove() continues to remove the cpu. But in this case, it should return error number since some process may run on the cpu. If the cpu has a running process and the cpu is turned the power off, the system may not work well.
--- drivers/acpi/processor_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/acpi/processor_driver.c +12 -6
@@ -605,7 +605,7 @@ err_free_pr:static int acpi_processor_remove (struct acpi_device *device,int type) {struct acpi_processor *pr =NULL ; - +int ret;if (!device || !acpi_driver_data (device))return -EINVAL ;
@@ -616,8 +616,9 @@ static int acpi_processor_remove(struct goto free;if (type ==ACPI_BUS_REMOVAL_EJECT ) { -if (acpi_processor_handle_eject (pr)) -return -EINVAL ; + ret =acpi_processor_handle_eject (pr); +if (ret) +return ret; }acpi_processor_power_exit (pr, device);
@@ -848,12 +849,17 @@ static acpi_status acpi_processor_hotaddstatic int acpi_processor_handle_eject (struct acpi_processor *pr) { -if (cpu_online (pr->id)) -cpu_down (pr->id); +int ret =0 ; + +if (cpu_online (pr->id)) { + ret =cpu_down (pr->id); +if (ret) +return ret; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); -return (0 ); +return ret; }#else static acpi_status acpi_processor_hotadd_init (struct acpi_processor *pr)
PATCH 2/2 acpi : prevent cpu from becoming online
Even if acpi_processor_handle_eject() offlines cpu, there is a chance to online the cpu after that. So the patch closes the window by using get/put_online_cpus().
Why does the patch change _cpu_up() logic?
The patch cares the race of hot-remove cpu and _cpu_up(). If the patch does not change it, there is the following race.
hot-remove cpu | _cpu_up()
------------------------------------- ------------------------------------
call acpi_processor_handle_eject() |
call cpu_down() |
call get_online_cpus() |
| call cpu_hotplug_begin() and stop here
call arch_unregister_cpu() |
call acpi_unmap_lsapic() |
call put_online_cpus() |
| start and continue _cpu_up()
return acpi_processor_remove() |
continue hot-remove the cpu |So _cpu_up() can continue to itself. And hot-remove cpu can also continue itself. If the patch changes _cpu_up() logic, the race disappears as below:
hot-remove cpu | _cpu_up()
-----------------------------------------------------------------------
call acpi_processor_handle_eject() |
call cpu_down() |
call get_online_cpus() |
| call cpu_hotplug_begin() and stop here
call arch_unregister_cpu() |
call acpi_unmap_lsapic() |
cpu's cpu_present is set |
to false by set_cpu_present()|
call put_online_cpus() |
| start _cpu_up()
| check cpu_present() and return -EINVAL
return acpi_processor_remove() |
continue hot-remove the cpu |--- drivers/acpi/processor_driver.c | 14 ++++++++++++++ kernel/cpu.c | 8 +++++--- 2 files changed, 19 insertions(+), 3 deletions(-)
drivers/acpi/processor_driver.c +14 -0
@@ -857,8 +857,22 @@ static int acpi_processor_handle_eject(s return ret; } +get_online_cpus (); +/* +* The cpu might become online again at this point. So we check whether +* the cpu has been onlined or not. If the cpu became online, it means +* that someone wants to use the cpu. So acpi_processor_handle_eject() +* returns -EAGAIN. +*/ +if (unlikely (cpu_online (pr->id))) { +put_online_cpus (); +pr_warn ("Failed to remove CPU %d, because other task " +"brought the CPU back online\n" , pr->id); +return -EAGAIN ; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); +put_online_cpus ();return ret; }#else
kernel/cpu.c +5 -3
@@ -343,11 +343,13 @@ static int __cpuinit _cpu_up(unsigned inunsigned long mod = tasks_frozen ?CPU_TASKS_FROZEN :0 ;struct task_struct *idle; -if (cpu_online (cpu) || !cpu_present (cpu)) -return -EINVAL ; -cpu_hotplug_begin (); +if (cpu_online (cpu) || !cpu_present (cpu)) { + ret = -EINVAL ; + goto out; + } + idle =idle_thread_get (cpu);if (IS_ERR (idle)) { ret =PTR_ERR (idle);
Quoted 39 lines · On Friday 28 of September 2012 19:36:02 Yasuaki Ishimatsu wrote:
Even if
cpu_down()fails,acpi_processor_remove()continues to remove the cpu. But in this case, it should return error number since some process may run on the cpu. If the cpu has a running process and the cpu is turned the power off, the system may not work well.Reviewed-by Srivatsa S. Bhat <[email protected]>Reviewed-by Toshi Kani <[email protected]>Signed-off-by Yasuaki Ishimatsu <[email protected]>--- drivers/acpi/processor_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)Index:
linux-3.6-rc7/drivers/acpi/processor_driver.c
===================================================================drivers/acpi/processor_driver.c +4
-3@@ -605,7 +605,7 @@ err_free_pr:static int acpi_processor_remove (struct acpi_device *device,int type) {struct acpi_processor *pr =NULL ; - +int ret;if (!device || !acpi_driver_data (device))return -EINVAL ;@@ -616,8 +616,9 @@ static int acpi_processor_remove(struct goto free;if (type ==ACPI_BUS_REMOVAL_EJECT ) { -if (acpi_processor_handle_eject (pr)) -return -EINVAL ; + ret =acpi_processor_handle_eject (pr); +if (ret) +return ret; }acpi_processor_power_exit (pr, device);
drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 855
Quoted 9 lines
drivers/acpi/processor_driver.c +4
-2@@ -848,12 +849,17 @@ static acpi_status acpi_processor_hotaddstatic int acpi_processor_handle_eject (struct acpi_processor *pr) { -if (cpu_online (pr->id)) -cpu_down (pr->id); +int ret =0 ; + +if (cpu_online (pr->id)) { + ret =cpu_down (pr->id);
If you defined ret here ...
drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 862
Quoted 8 lines
drivers/acpi/processor_driver.c +4
-1+if (ret) +return ret; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); -return (0 ); +return ret;
... this line wouldn't need to be changed.
drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 865
Quoted 3 lines
drivers/acpi/processor_driver.c +0
-0}#else static acpi_status acpi_processor_hotadd_init (struct acpi_processor *pr)
Thanks,
Rafael
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Hi Rafael,
Quoted 57 lines · Rafael J. Wysocki
Quoted 39 lines · On Friday 28 of September 2012 19:36:02 Yasuaki Ishimatsu wrote:
Even if
cpu_down()fails,acpi_processor_remove()continues to remove the cpu. But in this case, it should return error number since some process may run on the cpu. If the cpu has a running process and the cpu is turned the power off, the system may not work well.Reviewed-by Srivatsa S. Bhat <[email protected]>Reviewed-by Toshi Kani <[email protected]>Signed-off-by Yasuaki Ishimatsu <[email protected]>--- drivers/acpi/processor_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)Index:
linux-3.6-rc7/drivers/acpi/processor_driver.c
===================================================================drivers/acpi/processor_driver.c +4
-3@@ -605,7 +605,7 @@ err_free_pr:static int acpi_processor_remove (struct acpi_device *device,int type) {struct acpi_processor *pr =NULL ; - +int ret;if (!device || !acpi_driver_data (device))return -EINVAL ;@@ -616,8 +616,9 @@ static int acpi_processor_remove(struct goto free;if (type ==ACPI_BUS_REMOVAL_EJECT ) { -if (acpi_processor_handle_eject (pr)) -return -EINVAL ; + ret =acpi_processor_handle_eject (pr); +if (ret) +return ret; }acpi_processor_power_exit (pr, device);drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 855
Quoted 9 lines
drivers/acpi/processor_driver.c +4
-2@@ -848,12 +849,17 @@ static acpi_status acpi_processor_hotaddstatic int acpi_processor_handle_eject (struct acpi_processor *pr) { -if (cpu_online (pr->id)) -cpu_down (pr->id); +int ret =0 ; + +if (cpu_online (pr->id)) { + ret =cpu_down (pr->id);If you defined ret here ...
Quoted 4 lines
drivers/acpi/processor_driver.c +3
-0+if (ret) +return ret; + }
Quoted 6 lines
drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 862
Quoted 4 lines
drivers/acpi/processor_driver.c +1
-1arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); -return (0 ); +return ret;... this line wouldn't need to be changed.
Thank you for your review.
O.K. I'll put the return code back.
Thanks,
Yasuaki Ishimatsu
Quoted 9 lines
drivers/acpi/processor_driver.c static acpi_status acpi_processor_hotadd · line 865
Quoted 4 lines
drivers/acpi/processor_driver.c +0
-0}#else static acpi_status acpi_processor_hotadd_init (struct acpi_processor *pr)Thanks,
Rafael
PATCH v2 1/2 acpi : cpu hot-remove returns error when cpu_down() fails
v1 -> v2
- update Rafael's comment.
Quoted 16 lines
Review
Quoted 8 lines
static int acpi_processor_handle_eject(struct acpi_processor *pr) {(quoted diff) +4
-2- if (cpu_online(pr->id)) - cpu_down(pr->id); + int ret = 0; + + if (cpu_online(pr->id)) { + ret = cpu_down(pr->id);If you defined ret here ...
Quoted 4 lines
(quoted diff) +3
-0+ if (ret) + return ret; + }
Quoted 6 lines
Review
Quoted 4 lines
arch_unregister_cpu(pr->id); acpi_unmap_lsapic(pr->id);(quoted diff) +1
-1- return (0); + return ret;... this line wouldn't need to be changed.
---
Even if cpu_down() fails, acpi_processor_remove() continues to remove the cpu. But in this case, it should return error number since some process may run on the cpu. If the cpu has a running process and the cpu is turned the power off, the system may not work well.
--- drivers/acpi/processor_driver.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/acpi/processor_driver.c +11 -5
@@ -605,7 +605,7 @@ err_free_pr:static int acpi_processor_remove (struct acpi_device *device,int type) {struct acpi_processor *pr =NULL ; - +int ret;if (!device || !acpi_driver_data (device))return -EINVAL ;
@@ -616,8 +616,9 @@ static int acpi_processor_remove(struct goto free;if (type ==ACPI_BUS_REMOVAL_EJECT ) { -if (acpi_processor_handle_eject (pr)) -return -EINVAL ; + ret =acpi_processor_handle_eject (pr); +if (ret) +return ret; }acpi_processor_power_exit (pr);
@@ -848,8 +849,13 @@ static acpi_status acpi_processor_hotaddstatic int acpi_processor_handle_eject (struct acpi_processor *pr) { -if (cpu_online (pr->id)) -cpu_down (pr->id); +int ret =0 ; + +if (cpu_online (pr->id)) { + ret =cpu_down (pr->id); +if (ret) +return ret; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id);