From patchwork Thu Jan 30 17:28:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Lezcano X-Patchwork-Id: 3558391 Return-Path: X-Original-To: patchwork-linux-sh@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 2960AC02DC for ; Thu, 30 Jan 2014 17:28:56 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 189CD20181 for ; Thu, 30 Jan 2014 17:28:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DB8B82016C for ; Thu, 30 Jan 2014 17:28:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751433AbaA3R2x (ORCPT ); Thu, 30 Jan 2014 12:28:53 -0500 Received: from mail-wg0-f51.google.com ([74.125.82.51]:59670 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751057AbaA3R2w (ORCPT ); Thu, 30 Jan 2014 12:28:52 -0500 Received: by mail-wg0-f51.google.com with SMTP id z12so6776806wgg.6 for ; Thu, 30 Jan 2014 09:28:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=I8+PcqS1ViFe5aD2E9fHRc6RIpGa2Qcj0ycDjqkJexU=; b=FRS460y57/HHwXatSotiXek7zuurUVtA4lSEkMETYwRsz8X4yhtkTZTIsy2MBtOZjw Y/rs5n87grUE2Y4lMTtU0wi9rkk4fFzHRq2FVbIvm+NxwrcXK5PbVvLplXrSCJhyLGHT 4sPL45MNLfVNjOTYJCLhryAvQ/DA0KPLUlVZ4gg/GxaT/CaHWlsOjJ2vKprP0mmkGoyA cBynPDC8ihOLU/GYfuBybJfjoEz2/3xHyfgB7HbTyYZA8Qa8Yro5Dm8WYNihMWAHG2au Q1zgNdbcfejxAGAO+kW2bsDqChyAnoRhOi6JBxtNpGoj0Zcs2jb234reRSUQFo8iI783 7lFQ== X-Gm-Message-State: ALoCoQmyw+pVSsS6Kt8NQXJiNt89fN17U1l1gbbIP2le2NVp2+v9l+suexthOOlmy9jdz/7RAOEs X-Received: by 10.180.12.238 with SMTP id b14mr10572780wic.42.1391102930921; Thu, 30 Jan 2014 09:28:50 -0800 (PST) Received: from [192.168.1.150] (AToulouse-654-1-350-230.w90-55.abo.wanadoo.fr. [90.55.189.230]) by mx.google.com with ESMTPSA id 12sm13626877wjm.10.2014.01.30.09.28.49 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 30 Jan 2014 09:28:50 -0800 (PST) Message-ID: <52EA8BD4.6020803@linaro.org> Date: Thu, 30 Jan 2014 18:28:52 +0100 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Nicolas Pitre CC: Preeti U Murthy , Olof Johansson , Russell King , Benjamin Herrenschmidt , Paul Mundt , Thomas Gleixner , Ingo Molnar , Peter Zijlstra , "Rafael J. Wysocki" , linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 1/6] idle: move the cpuidle entry point to the generic idle loop References: <1391017513-12995-1-git-send-email-nicolas.pitre@linaro.org> <1391017513-12995-2-git-send-email-nicolas.pitre@linaro.org> <52E9C946.50704@linux.vnet.ibm.com> <52EA5720.8010000@linaro.org> In-Reply-To: Sender: linux-sh-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 01/30/2014 05:07 PM, Nicolas Pitre wrote: > On Thu, 30 Jan 2014, Daniel Lezcano wrote: > >> On 01/30/2014 06:28 AM, Nicolas Pitre wrote: >>> On Thu, 30 Jan 2014, Preeti U Murthy wrote: >>> >>>> Hi Nicolas, >>>> >>>> On 01/30/2014 02:01 AM, Nicolas Pitre wrote: >>>>> On Wed, 29 Jan 2014, Nicolas Pitre wrote: >>>>> >>>>>> In order to integrate cpuidle with the scheduler, we must have a >>>>>> better >>>>>> proximity in the core code with what cpuidle is doing and not delegate >>>>>> such interaction to arch code. >>>>>> >>>>>> Architectures implementing arch_cpu_idle() should simply enter >>>>>> a cheap idle mode in the absence of a proper cpuidle driver. >>>>>> >>>>>> Signed-off-by: Nicolas Pitre >>>>>> Acked-by: Daniel Lezcano >>>>> >>>>> As mentioned in my reply to Olof's comment on patch #5/6, here's a new >>>>> version of this patch adding the safety local_irq_enable() to the core >>>>> code. >>>>> >>>>> ----- >8 >>>>> >>>>> From: Nicolas Pitre >>>>> Subject: idle: move the cpuidle entry point to the generic idle loop >>>>> >>>>> In order to integrate cpuidle with the scheduler, we must have a better >>>>> proximity in the core code with what cpuidle is doing and not delegate >>>>> such interaction to arch code. >>>>> >>>>> Architectures implementing arch_cpu_idle() should simply enter >>>>> a cheap idle mode in the absence of a proper cpuidle driver. >>>>> >>>>> In both cases i.e. whether it is a cpuidle driver or the default >>>>> arch_cpu_idle(), the calling convention expects IRQs to be disabled >>>>> on entry and enabled on exit. There is a warning in place already but >>>>> let's add a forced IRQ enable here as well. This will allow for >>>>> removing the forced IRQ enable some implementations do locally and >>>> >>>> Why would this patch allow for removing the forced IRQ enable that are >>>> being done on some archs in arch_cpu_idle()? Isn't this patch expecting >>>> the default arch_cpu_idle() to have re-enabled the interrupts after >>>> exiting from the default idle state? Its supposed to only catch faulty >>>> cpuidle drivers that haven't enabled IRQs on exit from idle state but >>>> are expected to have done so, isn't it? >>> >>> Exact. However x86 currently does this: >>> >>> if (cpuidle_idle_call()) >>> x86_idle(); >>> else >>> local_irq_enable(); >>> >>> So whenever cpuidle_idle_call() is successful then IRQs are >>> unconditionally enabled whether or not the underlying cpuidle driver has >>> properly done it or not. And the reason is that some of the x86 cpuidle >>> do fail to enable IRQs before returning. >>> >>> So the idea is to get rid of this unconditional IRQ enabling and let the >>> core issue a warning instead (as well as enabling IRQs to allow the >>> system to run). >> >> But what I don't get with your comment is the local_irq_enable is done from >> the cpuidle common framework in 'cpuidle_enter_state' it is not done from the >> arch specific backend cpuidle driver. > > Oh well... This certainly means we'll have to clean this mess as some > drivers do it on their own while some others don't. Some drivers also > loop on !need_resched() while some others simply return on the first > interrupt. Ok, I think the mess is coming from 'default_idle' which does not re-enable the local_irq but used from different places like amd_e400_idle and apm_cpu_idle. void default_idle(void) { trace_cpu_idle_rcuidle(1, smp_processor_id()); safe_halt(); trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id()); } Considering the system configured without cpuidle because this one *always* enable the local irq, we have the different cases: x86_idle = default_idle(); ==> local_irq_enable is missing x86_idle = amd_e400_idle(); ==> it calls local_irq_disable(); but in the idle loop context where the local irqs are already disabled. ==> if amd_e400_c1e_detected is true, the local_irq are enabled ==> otherwise no ==> default_idle is called from there and does not enable local_irqs >> So the code above could be: >> >> if (cpuidle_idle_call()) >> x86_idle(); >> >> without the else section, this local_irq_enable is pointless. Or may be I >> missed something ? > > A later patch removes it anyway. But if it is really necessary to > enable interrupts then the core will do it but with a warning now. This WARN should disappear. It was there because it was up to the backend cpuidle driver to enable the irq. But in the meantime, that was consolidated into a single place in the cpuidle framework so no need to try to catch errors. What about (based on this patchset). diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index 4505e2a..2d60cbb 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -299,6 +299,7 @@ void arch_cpu_idle_dead(void) void arch_cpu_idle(void) { x86_idle(); + local_irq_enable(); } /*