Re: [PATCH 2/2] x86: cpuidle: remove broadcast timer initialization

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 08/20/2013 04:11 PM, Rafael J. Wysocki wrote:
> On Tuesday, July 30, 2013 03:54:28 PM Daniel Lezcano wrote:
>> Commit 89878baa73f0f1c679355006bd8632e5d78f96c2 introduced the flag
>> CPUIDLE_FLAG_TIMER_STOP where we specify a specific idle state stops the local
>> timer.
>>
>> Commit a06df062a189a8d5588babb8bf0bb78672497798 introduced the initialization
>> of the timer broadcast framework depending of the flag presence.
>>
>> We have now a way to set a flag for a specific state to tell if the local timer
>> will be down in deep idle state or not and we have cpu hotplug support to setup
>> the broadcast timer.
> 
> So this depends on [1/2], right?

Yes, that's correct.

>> Remove the timer broadcast initialization from init and at the cpu hotplug
>> time.
>>
>> Remove the timer broadcast activation in the idle loop.
>>
>> Remove the bitmap LAPIC_TIMER_ALWAYS_RELIABLE.
>>
>> Set the CPUIDLE_FLAG_TIMER_STOP on the states when they are greater than C1 and
>> the cpu has not X86_FEATURE_ARAT.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I generally like the changes made by this patch, but I wonder how much it
> has been tested?

Well it has been tested on my laptop which is an i5-3317.

With lapic_tsc_reliable_states 0xffffffff

> The subject is slightly misleading too, because it's about the intel_idle
> driver.  Please put intel_idle in the subject.

Ok, sure.

>> ---
>>  drivers/idle/intel_idle.c |   41 ++++-------------------------------------
>>  1 file changed, 4 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index fa6964d..a975868 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -55,7 +55,6 @@
>>  
>>  #include <linux/kernel.h>
>>  #include <linux/cpuidle.h>
>> -#include <linux/clockchips.h>
>>  #include <trace/events/power.h>
>>  #include <linux/sched.h>
>>  #include <linux/notifier.h>
>> @@ -77,10 +76,6 @@ static int max_cstate = CPUIDLE_STATE_MAX - 1;
>>  
>>  static unsigned int mwait_substates;
>>  
>> -#define LAPIC_TIMER_ALWAYS_RELIABLE 0xFFFFFFFF
>> -/* Reliable LAPIC Timer States, bit 1 for C1 etc.  */
>> -static unsigned int lapic_timer_reliable_states = (1 << 1);	 /* Default to only C1 */
>> -
>>  struct idle_cpu {
>>  	struct cpuidle_state *state_table;
>>  
>> @@ -356,9 +351,6 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
>>  		leave_mm(cpu);
>>  
>> -	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>> -
>>  	if (!need_resched()) {
>>  
>>  		__monitor((void *)&current_thread_info()->flags, 0, 0);
>> @@ -367,23 +359,9 @@ static int intel_idle(struct cpuidle_device *dev,
>>  			__mwait(eax, ecx);
>>  	}
>>  
>> -	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>> -
>>  	return index;
>>  }
>>  
>> -static void __setup_broadcast_timer(void *arg)
>> -{
>> -	unsigned long reason = (unsigned long)arg;
>> -	int cpu = smp_processor_id();
>> -
>> -	reason = reason ?
>> -		CLOCK_EVT_NOTIFY_BROADCAST_ON : CLOCK_EVT_NOTIFY_BROADCAST_OFF;
>> -
>> -	clockevents_notify(reason, &cpu);
>> -}
>> -
>>  static int cpu_hotplug_notify(struct notifier_block *n,
>>  			      unsigned long action, void *hcpu)
>>  {
>> @@ -393,10 +371,6 @@ static int cpu_hotplug_notify(struct notifier_block *n,
>>  	switch (action & 0xf) {
>>  	case CPU_ONLINE:
>>  
>> -		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>> -			smp_call_function_single(hotcpu, __setup_broadcast_timer,
>> -						 (void *)true, 1);
>> -
>>  		/*
>>  		 * Some systems can hotplug a cpu at runtime after
>>  		 * the kernel has booted, we have to initialize the
>> @@ -524,16 +498,9 @@ static int intel_idle_probe(void)
>>  	icpu = (const struct idle_cpu *)id->driver_data;
>>  	cpuidle_state_table = icpu->state_table;
>>  
>> -	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>> -		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>> -	else
>> -		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
>> -
>>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>>  		" model 0x%X\n", boot_cpu_data.x86_model);
>>  
>> -	pr_debug(PREFIX "lapic_timer_reliable_states 0x%x\n",
>> -		lapic_timer_reliable_states);
>>  	return 0;
>>  }
>>  
>> @@ -594,6 +561,10 @@ static int intel_idle_cpuidle_driver_init(void)
>>  			mark_tsc_unstable("TSC halts in idle"
>>  					" states deeper than C2");
>>  
>> +		if (cstate > 1 && !boot_cpu_has(X86_FEATURE_ARAT))
>> +			cpuidle_state_table[cstate].flags |=
>> +				CPUIDLE_FLAG_TIMER_STOP;
>> +
>>  		drv->states[drv->state_count] =	/* structure copy */
>>  			cpuidle_state_table[cstate];
>>  
>> @@ -705,10 +676,6 @@ static void __exit intel_idle_exit(void)
>>  {
>>  	intel_idle_cpuidle_devices_uninit();
>>  	cpuidle_unregister_driver(&intel_idle_driver);
>> -
>> -
>> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>> -		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
>>  	unregister_cpu_notifier(&cpu_hotplug_notifier);
>>  
>>  	return;
>>


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog