From patchwork Mon Jul 9 02:31:05 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yasuaki Ishimatsu X-Patchwork-Id: 1170591 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 85DEA40134 for ; Mon, 9 Jul 2012 02:48:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751430Ab2GICba (ORCPT ); Sun, 8 Jul 2012 22:31:30 -0400 Received: from fgwmail5.fujitsu.co.jp ([192.51.44.35]:41833 "EHLO fgwmail5.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751419Ab2GICb3 (ORCPT ); Sun, 8 Jul 2012 22:31:29 -0400 Received: from m3.gw.fujitsu.co.jp (unknown [10.0.50.73]) by fgwmail5.fujitsu.co.jp (Postfix) with ESMTP id 05F073EE0AE; Mon, 9 Jul 2012 11:31:29 +0900 (JST) Received: from smail (m3 [127.0.0.1]) by outgoing.m3.gw.fujitsu.co.jp (Postfix) with ESMTP id E270E45DE9E; Mon, 9 Jul 2012 11:31:28 +0900 (JST) Received: from s3.gw.fujitsu.co.jp (s3.gw.fujitsu.co.jp [10.0.50.93]) by m3.gw.fujitsu.co.jp (Postfix) with ESMTP id CCD2F45DE7E; Mon, 9 Jul 2012 11:31:28 +0900 (JST) Received: from s3.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1]) by s3.gw.fujitsu.co.jp (Postfix) with ESMTP id BF0FA1DB803E; Mon, 9 Jul 2012 11:31:28 +0900 (JST) Received: from g01jpexchkw12.g01.fujitsu.local (g01jpexchkw12.g01.fujitsu.local [10.0.194.51]) by s3.gw.fujitsu.co.jp (Postfix) with ESMTP id 7A3381DB803C; Mon, 9 Jul 2012 11:31:28 +0900 (JST) Received: from [127.0.0.1] (10.124.101.33) by g01jpexchkw12.g01.fujitsu.local (10.0.194.51) with Microsoft SMTP Server id 14.2.309.2; Mon, 9 Jul 2012 11:31:27 +0900 X-SecurityPolicyCheck: OK by SHieldMailChecker v1.7.4 Message-ID: <4FFA4269.5050808@jp.fujitsu.com> Date: Mon, 9 Jul 2012 11:31:05 +0900 From: Yasuaki Ishimatsu User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: "Srivatsa S. Bhat" CC: , , Subject: Re: [PATCH 1/2] acpi : cpu hot-remove returns error number when cpu_down() fails References: <4FF65898.3000301@jp.fujitsu.com> <4FF6B537.1030703@linux.vnet.ibm.com> In-Reply-To: <4FF6B537.1030703@linux.vnet.ibm.com> Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org Hi Srivatsa, Thank you for your reviewing. 2012/07/06 18:51, Srivatsa S. Bhat wrote: > On 07/06/2012 08:46 AM, Yasuaki Ishimatsu wrote: >> Even if cpu_down() fails, acpi_processor_remove() continues to remove the cpu. > > Ouch! > >> 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 cannot work well. >> >> Signed-off-by: Yasuaki Ishimatsu >> >> --- >> drivers/acpi/processor_driver.c | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> Index: linux-3.5-rc4/drivers/acpi/processor_driver.c >> =================================================================== >> --- linux-3.5-rc4.orig/drivers/acpi/processor_driver.c 2012-06-25 04:53:04.000000000 +0900 >> +++ linux-3.5-rc4/drivers/acpi/processor_driver.c 2012-07-05 21:02:58.711285382 +0900 >> @@ -610,7 +610,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; >> @@ -621,8 +621,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); >> @@ -841,12 +842,17 @@ static acpi_status acpi_processor_hotadd >> >> static int acpi_processor_handle_eject(struct acpi_processor *pr) >> { >> - if (cpu_online(pr->id)) >> - cpu_down(pr->id); >> + int ret; >> + >> + if (cpu_online(pr->id)) { >> + ret = cpu_down(pr->id); >> + if (ret) >> + return ret; >> + } >> > > Strictly speaking, this is not thorough enough. What prevents someone > from onlining that same cpu again, at this point? > So, IMHO, you need to wrap the contents of this function inside a > get_online_cpus()/put_online_cpus() block, to prevent anyone else > from messing with CPU hotplug at the same time. If I understand your comment by mistake, please let me know. If the contents is wrapped a inside get_online_cpus()/put_online_cpus() block as below, cpu_down() will stop since cpu_down() calls cpu_hotplug_begin() and cpu_hotplug_begin() waits for cpu_hotplug.refcount to become 0. + get_online_cpus() + if (cpu_online(pr->id)) { + ret = cpu_down(pr->id); + if (ret) + return ret; + } + put_online_cpus() I think following patch can prevent it correctly. How about the patch? --- drivers/acpi/processor_driver.c | 12 ++++++++++++ kernel/cpu.c | 8 +++++--- 2 files changed, 17 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: linux-3.5-rc4/drivers/acpi/processor_driver.c =================================================================== --- linux-3.5-rc4.orig/drivers/acpi/processor_driver.c 2012-07-09 09:59:01.280211202 +0900 +++ linux-3.5-rc4/drivers/acpi/processor_driver.c 2012-07-09 11:05:34.559859236 +0900 @@ -844,14 +844,26 @@ static int acpi_processor_handle_eject(s { int ret; +retry: if (cpu_online(pr->id)) { ret = cpu_down(pr->id); if (ret) return ret; } + get_online_cpus(); + /* + * Someone might online the cpu again at this point. So we check that + * cpu has been onlined or not. If cpu is online, we try to offline + * the cpu again. + */ + if (cpu_online(pr->id)) { + put_online_cpus(); + goto retry; + } arch_unregister_cpu(pr->id); acpi_unmap_lsapic(pr->id); + put_online_cpus(); return ret; } #else Index: linux-3.5-rc4/kernel/cpu.c =================================================================== --- linux-3.5-rc4.orig/kernel/cpu.c 2012-07-09 09:59:01.280211202 +0900 +++ linux-3.5-rc4/kernel/cpu.c 2012-07-09 09:59:02.903190965 +0900 @@ -343,11 +343,13 @@ static int __cpuinit _cpu_up(unsigned in unsigned 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);