From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp07.au.ibm.com (e23smtp07.au.ibm.com [202.81.31.140]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e23smtp07.au.ibm.com", Issuer "GeoTrust SSL CA" (verified OK)) by ozlabs.org (Postfix) with ESMTPS id BF6AFB6F84 for ; Mon, 28 Nov 2011 22:02:42 +1100 (EST) Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 28 Nov 2011 11:00:25 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pASAx1OA3338306 for ; Mon, 28 Nov 2011 21:59:01 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pASB2ORr026347 for ; Mon, 28 Nov 2011 22:02:25 +1100 Message-ID: <4ED36A37.3030409@linux.vnet.ibm.com> Date: Mon, 28 Nov 2011 16:32:15 +0530 From: Deepthi Dharwar MIME-Version: 1.0 To: Benjamin Herrenschmidt 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 Cc: linuxppc-dev@ozlabs.org, linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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