From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752843Ab1K1LCg (ORCPT ); Mon, 28 Nov 2011 06:02:36 -0500 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:56493 "EHLO e23smtp09.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752546Ab1K1LCe (ORCPT ); Mon, 28 Nov 2011 06:02:34 -0500 Message-ID: <4ED36A37.3030409@linux.vnet.ibm.com> Date: Mon, 28 Nov 2011 16:32:15 +0530 From: Deepthi Dharwar User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.23) Gecko/20110922 Thunderbird/3.1.15 MIME-Version: 1.0 Newsgroups: gmane.linux.power-management.general,gmane.linux.ports.ppc64.devel,gmane.linux.kernel To: Benjamin Herrenschmidt CC: linuxppc-dev@ozlabs.org, linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines References: <20111117112815.9191.2322.stgit@localhost6.localdomain6> <20111117112830.9191.1951.stgit@localhost6.localdomain6> <1322434096.23348.6.camel@pasglop> In-Reply-To: <1322434096.23348.6.camel@pasglop> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit x-cbid: 11112801-3568-0000-0000-000000CB7A76 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ben, Thanks a lot for the review. On 11/28/2011 04:18 AM, Benjamin Herrenschmidt wrote: > On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote: >> This patch provides cpu_idle_wait() routine for the powerpc >> platform which is required by the cpuidle subsystem. This >> routine is requied to change the idle handler on SMP systems. >> The equivalent routine for x86 is in arch/x86/kernel/process.c >> but the powerpc implementation is different. >> >> Signed-off-by: Deepthi Dharwar >> Signed-off-by: Trinabh Gupta >> Signed-off-by: Arun R Bharadwaj >> --- > > No, that patch also adds this idle boot override thing (can you pick a > shorter name for boot_option_idle_override btw ?) which seems unrelated > and without any explanation as to what it's supposed to be about. Yes, we can pick a better and shorter name for this variable. This variable is used to determine if cpuidle framework needs to be enabled and pseries_driver to be loaded or not. We disable cpuidle framework only when powersave_off option is set or not enabled by the user. > > Additionally, I'm a bit worried (but maybe we already discussed that a > while back, I don't know) but cpu_idle_wait() has "wait" in the name, > which makes me think it might need to actually -wait- for all cpus to > have come out of the function. cpu_idle_wait is used to ensure that all the CPUs discard old idle handler and update to new one. Required while changing idle handler on SMP systems. > Now your implementation doesn't provide that guarantee. It might be > fine, I don't know, but if it is, you'd better document it well in the > comments surrounding the code, because as it is, all you do is shoot an > interrupt which will cause the target CPU to eventually come out of idle > some time in the future. I was hoping that sending an explicit reschedule to the cpus would do the trick but sure we can add some documentation around the code. > > Cheers, > Ben. > >> arch/powerpc/Kconfig | 4 ++++ >> arch/powerpc/include/asm/processor.h | 2 ++ >> arch/powerpc/include/asm/system.h | 1 + >> arch/powerpc/kernel/idle.c | 26 ++++++++++++++++++++++++++ >> 4 files changed, 33 insertions(+), 0 deletions(-) >> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index b177caa..87f8604 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -87,6 +87,10 @@ config ARCH_HAS_ILOG2_U64 >> bool >> default y if 64BIT >> >> +config ARCH_HAS_CPU_IDLE_WAIT >> + bool >> + default y >> + >> config GENERIC_HWEIGHT >> bool >> default y >> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h >> index eb11a44..811b7e7 100644 >> --- a/arch/powerpc/include/asm/processor.h >> +++ b/arch/powerpc/include/asm/processor.h >> @@ -382,6 +382,8 @@ static inline unsigned long get_clean_sp(struct pt_regs *regs, int is_32) >> } >> #endif >> >> +enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF}; >> + >> #endif /* __KERNEL__ */ >> #endif /* __ASSEMBLY__ */ >> #endif /* _ASM_POWERPC_PROCESSOR_H */ >> diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h >> index e30a13d..ff66680 100644 >> --- a/arch/powerpc/include/asm/system.h >> +++ b/arch/powerpc/include/asm/system.h >> @@ -221,6 +221,7 @@ extern unsigned long klimit; >> extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask); >> >> extern int powersave_nap; /* set if nap mode can be used in idle loop */ >> +void cpu_idle_wait(void); >> >> /* >> * Atomic exchange >> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c >> index 39a2baa..b478c72 100644 >> --- a/arch/powerpc/kernel/idle.c >> +++ b/arch/powerpc/kernel/idle.c >> @@ -39,9 +39,13 @@ >> #define cpu_should_die() 0 >> #endif >> >> +unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE; >> +EXPORT_SYMBOL(boot_option_idle_override); >> + >> static int __init powersave_off(char *arg) >> { >> ppc_md.power_save = NULL; >> + boot_option_idle_override = IDLE_POWERSAVE_OFF; >> return 0; >> } >> __setup("powersave=off", powersave_off); >> @@ -102,6 +106,28 @@ void cpu_idle(void) >> } >> } >> >> + >> +/* >> + * cpu_idle_wait - Used to ensure that all the CPUs come out of the old >> + * idle loop and start using the new idle loop. >> + * Required while changing idle handler on SMP systems. >> + * Caller must have changed idle handler to the new value before the call. >> + */ >> +void cpu_idle_wait(void) >> +{ >> + int cpu; >> + smp_mb(); >> + >> + /* kick all the CPUs so that they exit out of old idle routine */ >> + get_online_cpus(); >> + for_each_online_cpu(cpu) { >> + if (cpu != smp_processor_id()) >> + smp_send_reschedule(cpu); >> + } >> + put_online_cpus(); >> +} >> +EXPORT_SYMBOL_GPL(cpu_idle_wait); >> + >> int powersave_nap; >> >> #ifdef CONFIG_SYSCTL >> >> _______________________________________________ >> Linuxppc-dev mailing list >> Linuxppc-dev@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/linuxppc-dev > > Regards, Deepthi