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: 23933 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-vb0-f70.google.com (mail-vb0-f70.google.com [209.85.212.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 3008D202B2 for ; Thu, 30 Jan 2014 17:28:57 +0000 (UTC) Received: by mail-vb0-f70.google.com with SMTP id w17sf7670377vbj.1 for ; Thu, 30 Jan 2014 09:28:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:cc:subject:references:in-reply-to:sender:precedence :list-id:x-original-sender:x-original-authentication-results :mailing-list:list-post:list-help:list-archive:list-unsubscribe :content-type:content-transfer-encoding; bh=gH6WolwuVmGlhsqhx7UDrMsbnsWsbXvZnP4Nh7vRCQo=; b=SzSc9IAKlqHWXYwAXqkb6pvKdbCmN5h0del/KjHHzwl51cPnHSl4JsQ1Z9jl3Ozuol SLBByLTeQ57AQMzkDnA9e/V5KxlYBeQWT3gHVRULJ31rVdyachcqCaEv6tVFvWhbjnuS zp1mYOYyE0qgf+nbFyd/2PHGXv70LtGlA4YQZhkTAMMD9eK12pNAAUFn32orjW0zFUSO TMovnuh38lEDG+G92eLvoHkrSUmibP4CnWasLhELUHrC6P021jcKvN5lfHQLSj2WNpK7 C+VaXVDQiLJv0UaZp7Z1gXXPFVM7suu8wMlMW1tCLf0y5PPcZLtKcLScmpwfzE1YqoDf LCFg== X-Gm-Message-State: ALoCoQmTsjVvs+22lytd4TEoK9jtomRE6Y619VruFNhqYxV4k/Te/hqHNScm/B/P6LA1PUgfjn9u X-Received: by 10.58.38.137 with SMTP id g9mr6073159vek.6.1391102936350; Thu, 30 Jan 2014 09:28:56 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.25.198 with SMTP id 64ls625190qgt.31.gmail; Thu, 30 Jan 2014 09:28:56 -0800 (PST) X-Received: by 10.52.171.68 with SMTP id as4mr10660826vdc.0.1391102936261; Thu, 30 Jan 2014 09:28:56 -0800 (PST) Received: from mail-ve0-f175.google.com (mail-ve0-f175.google.com [209.85.128.175]) by mx.google.com with ESMTPS id eo4si2318022vdb.108.2014.01.30.09.28.56 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 30 Jan 2014 09:28:56 -0800 (PST) Received-SPF: neutral (google.com: 209.85.128.175 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.175; Received: by mail-ve0-f175.google.com with SMTP id c14so2381759vea.34 for ; Thu, 30 Jan 2014 09:28:56 -0800 (PST) X-Received: by 10.52.171.39 with SMTP id ar7mr6789939vdc.5.1391102936168; Thu, 30 Jan 2014 09:28:56 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.220.174.196 with SMTP id u4csp30789vcz; Thu, 30 Jan 2014 09:28:55 -0800 (PST) X-Received: by 10.67.11.12 with SMTP id ee12mr15539286pad.132.1391102934916; Thu, 30 Jan 2014 09:28:54 -0800 (PST) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id q6si7210985pbf.34.2014.01.30.09.28.54; Thu, 30 Jan 2014 09:28:54 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751519AbaA3R2x (ORCPT + 11 others); Thu, 30 Jan 2014 12:28:53 -0500 Received: from mail-wg0-f46.google.com ([74.125.82.46]:44401 "EHLO mail-wg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751058AbaA3R2w (ORCPT ); Thu, 30 Jan 2014 12:28:52 -0500 Received: by mail-wg0-f46.google.com with SMTP id x12so6766993wgg.13 for ; Thu, 30 Jan 2014 09:28:51 -0800 (PST) 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-pm-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: daniel.lezcano@linaro.org X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.175 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , 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(); } /*