From mboxrd@z Thu Jan 1 00:00:00 1970 From: Deepthi Dharwar Subject: Re: [PATCH 1/3] cpuidle: If disable_cpuidle() is called, set pm_idle to default_idle. Date: Wed, 23 Nov 2011 16:36:24 +0530 Message-ID: <4ECCD3B0.2010207@linux.vnet.ibm.com> References: <1320786914-10541-1-git-send-email-konrad.wilk@oracle.com> <1320786914-10541-2-git-send-email-konrad.wilk@oracle.com> <4EBA53C5.9040603@linux.vnet.ibm.com> <20111109144440.GB8410@phenom.dumpdata.com> <4EBD09FF.4030002@linux.vnet.ibm.com> <20111115144004.GE22675@phenom.dumpdata.com> <20111122134612.GA29668@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3808524714696937928==" Return-path: In-Reply-To: <20111122134612.GA29668@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Konrad Rzeszutek Wilk Cc: len.brown@intel.com, jeremy@goop.org, xen-devel@lists.xensource.com, x86@kernel.org, linux-kernel@vger.kernel.org, stable@kernel.org, mingo@redhat.com, bp@alien8.de, hpa@zytor.com, tj@kernel.org, tglx@linutronix.de, trenn@suse.de List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============3808524714696937928== Content-Type: multipart/alternative; boundary="------------020707000304080803020504" This is a multi-part message in MIME format. --------------020707000304080803020504 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Hi Konrad, On 11/22/2011 07:16 PM, Konrad Rzeszutek Wilk wrote: > On Tue, Nov 15, 2011 at 09:40:04AM -0500, Konrad Rzeszutek Wilk wrote: >>> Well I was with a view that if cpuidle is disabled, mwait and arm_e400 >>> would take precedence over default_idle. But if is ok to have default_idle instead, >>> then go ahead. >>> >>>> Perhaps there is another way do this is: >>>> >>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >>>> index becd6d9..04b10a4 100644 >>>> --- a/drivers/cpuidle/cpuidle.c >>>> +++ b/drivers/cpuidle/cpuidle.c >>>> @@ -38,6 +38,7 @@ int cpuidle_disabled(void) >>>> void disable_cpuidle(void) >>>> { >>>> off = 1; >>>> + pm_idle = default_idle; >>>> } >>>> >>> Brining pm_idle pointer back to cpuidle code is going a step back coz >>> we wanted to complete remove using pm_idle going forward. As having >>> a pointer exported into various files is not a good thing. That's the >>> reason we started the clean up in the first place :) >> >> Perhaps then something along these lines, where we still set default_idle >> but don't fiddle with the pm_idle (and can still rip out the >> EXPORT_SYMBOL_GPL(default_idle) in 2012): >> >> (Hadn't tested this yet). > > Now have tested it. > As long as you dont bring back exporting pm_idle and using it in cpuidle code it should be ok. So one should not use pm_idle in disable_cpuidle function. >> >> diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h >> index c2ff2a1..2d2f01c 100644 >> --- a/arch/x86/include/asm/system.h >> +++ b/arch/x86/include/asm/system.h >> @@ -401,6 +401,7 @@ extern unsigned long arch_align_stack(unsigned long sp); >> extern void free_init_pages(char *what, unsigned long begin, unsigned long end); >> >> void default_idle(void); >> +bool set_pm_idle_to_default(void); >> >> void stop_this_cpu(void *dummy); >> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> index e7e3b01..bfb113f 100644 >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> @@ -403,6 +403,14 @@ void default_idle(void) >> EXPORT_SYMBOL(default_idle); >> #endif >> >> +bool set_pm_idle_to_default() >> +{ >> + if (!pm_idle) { >> + pm_idle = default_idle; >> + return true; >> + } >> + return false; >> +} >> void stop_this_cpu(void *dummy) >> { >> local_irq_disable(); >> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c >> index 46d6d21..7506181 100644 >> --- a/arch/x86/xen/setup.c >> +++ b/arch/x86/xen/setup.c >> @@ -448,6 +448,6 @@ void __init xen_arch_setup(void) >> #endif >> disable_cpuidle(); >> boot_option_idle_override = IDLE_HALT; >> - >> + WARN_ON(!set_pm_idle_to_default()); >> fiddle_vdso(); >> } >> This looks good, where we see setting of default_idle only in Xen setup code and not the in the generic cpuidle code path. This would allow other architectures to use mwait or arm_e400 to be enabled when cpuidle is disabled. >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > Cheers, Deepthi --------------020707000304080803020504 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Hi Konrad,

On 11/22/2011 07:16 PM, Konrad Rzeszutek Wilk wrote:

> On Tue, Nov 15, 2011 at 09:40:04AM -0500, Konrad Rzeszutek Wilk wrote:
>>> Well I was with a view that if cpuidle is disabled, mwait and arm_e400
>>> would take precedence over default_idle. But if is ok to have default_idle instead,
>>> then go ahead. 
>>>
>>>> Perhaps there is another way do this is:
>>>>
>>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>>> index becd6d9..04b10a4 100644
>>>> --- a/drivers/cpuidle/cpuidle.c
>>>> +++ b/drivers/cpuidle/cpuidle.c
>>>> @@ -38,6 +38,7 @@ int cpuidle_disabled(void)
>>>>  void disable_cpuidle(void)
>>>>  {
>>>>  	off = 1;
>>>> +	pm_idle = default_idle;
>>>>  }
>>>>
>>> Brining pm_idle pointer back to cpuidle code is going a step back coz
>>> we wanted to complete remove using pm_idle going forward. As having 
>>> a pointer exported into various files is not a good thing. That's the 
>>> reason we started the clean up in the first place :) 
>>
>> Perhaps then something along these lines, where we still set default_idle
>> but don't fiddle with the pm_idle (and can still rip out the
>> EXPORT_SYMBOL_GPL(default_idle) in 2012):
>>
>> (Hadn't tested this yet).
>
> Now have tested it.
>


As long as you dont bring back exporting pm_idle and using it in cpuidle code
it should be ok.  So one should not use pm_idle in disable_cpuidle function.

>>
>> diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
>> index c2ff2a1..2d2f01c 100644
>> --- a/arch/x86/include/asm/system.h
>> +++ b/arch/x86/include/asm/system.h
>> @@ -401,6 +401,7 @@ extern unsigned long arch_align_stack(unsigned long sp);
>>  extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
>>  
>>  void default_idle(void);
>> +bool set_pm_idle_to_default(void);
>>  
>>  void stop_this_cpu(void *dummy);
>>  
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index e7e3b01..bfb113f 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -403,6 +403,14 @@ void default_idle(void)
>>  EXPORT_SYMBOL(default_idle);
>>  #endif
>>  
>> +bool set_pm_idle_to_default()
>> +{
>> +	if (!pm_idle) {
>> +		pm_idle = default_idle;
>> +		return true;
>> +	}
>> +	return false;
>> +}
>>  void stop_this_cpu(void *dummy)
>>  {
>>  	local_irq_disable();
>> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
>> index 46d6d21..7506181 100644
>> --- a/arch/x86/xen/setup.c
>> +++ b/arch/x86/xen/setup.c
>> @@ -448,6 +448,6 @@ void __init xen_arch_setup(void)
>>  #endif
>>  	disable_cpuidle();
>>  	boot_option_idle_override = IDLE_HALT;
>> -
>> +	WARN_ON(!set_pm_idle_to_default());
>>  	fiddle_vdso();
>>  }
>>


This looks good, where we see setting of default_idle only  in
Xen setup code and not the in the generic cpuidle code path.
This would allow other architectures to use mwait or arm_e400
to be enabled when cpuidle is disabled.

>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>


Cheers,
Deepthi

 --------------020707000304080803020504-- --===============3808524714696937928== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============3808524714696937928==--