Re: [PATCH 1/3] cpuidle: If disable_cpuidle() is called, set pm_idle to default_idle.

[Deepthi Dharwar <deepthi@linux.vnet.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3117 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 4572 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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