Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

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

On 02/24/2014 05:05 PM, Peter Zijlstra wrote:
> On Mon, Feb 24, 2014 at 04:39:08PM +0100, Daniel Lezcano wrote:
>
>>> And when you do that; you can also push down the
>>> current_clr_polling_and_test() muck so it doesn't cover the actual
>>> cpuidle policy code.
>>
>> I am not getting it. Where do you suggest to move it ?
>
> A little something like so I suppose;

Thanks for taking the time to send me this patch.

> only call
> current_clr_polling_and_test() right before calling the actual idle
> function. Although I suppose cpuidle_enter() can still do all sorts of
> weird.

Well there is the polling idle state for the x86 and ppc cpuidle 
drivers. Except that, I think we have something more or less clean.

> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -78,37 +78,35 @@ static int cpuidle_idle_call(void)
>   	stop_critical_timings();
>   	rcu_idle_enter();
>
> -	next_state = cpuidle_enabled(drv, dev);
> -	if (next_state < 0) {
> -		arch_cpu_idle();
> -		goto out;
> -	}
> -
> -	/*
> -	 * Ask the governor for the next state, this call can fail for
> -	 * different reasons: cpuidle is not enabled or an idle state
> -	 * fulfilling the constraints was not found. In this case, we
> -	 * fall back to the default idle function
> -	 */
> -	next_state = cpuidle_select(drv, dev);
> +	if (cpuidle_enabled(drv, dev) == 0) {
> +		/*
> +		 * Ask the governor for the next state, this call can fail for
> +		 * different reasons: cpuidle is not enabled or an idle state
> +		 * fulfilling the constraints was not found. In this case, we
> +		 * fall back to the default idle function
> +		 */
> +		next_state = cpuidle_select(drv, dev);
> +
> +		if (current_clr_polling_and_test()) {
> +			dev->last_residency = 0;
> +			entered_state = next_state;
> +			local_irq_enable();
> +		} else {
> +			trace_cpu_idle_rcuidle(next_state, dev->cpu);
> +			entered_state = cpuidle_enter(drv, dev, next_state);
> +			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> +		}
>
> -	if (need_resched()) {

Ok. The need_resched is now replaced by 'current_clr_polling_and_test', 
with a call to '__current_set_polling()' to set the flag back, right ?

For my personal information, what is the subtlety with:

if (tif_need_resched())
	set_preempt_need_resched();

?

> -		dev->last_residency = 0;
>   		/* give the governor an opportunity to reflect on the outcome */
> -		cpuidle_reflect(dev, next_state);
> -		local_irq_enable();
> -		goto out;
> +		cpuidle_reflect(dev, entered_state);
> +	} else {
> +		if (current_clr_polling_and_test())
> +			local_irq_enable();
> +		else
> +			arch_cpu_idle();
>   	}
> +	__current_set_polling();
>
> -	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> -
> -	entered_state = cpuidle_enter(drv, dev, next_state);
> -
> -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> -
> -	/* give the governor an opportunity to reflect on the outcome */
> -	cpuidle_reflect(dev, entered_state);
> -out:
>   	if (WARN_ON_ONCE(irqs_disabled()))
>   		local_irq_enable();
>
> @@ -145,16 +143,11 @@ static void cpu_idle_loop(void)
>   			 * know that the IPI is going to arrive right
>   			 * away
>   			 */
> -			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
> +			if (cpu_idle_force_poll || tick_check_broadcast_expired())
>   				cpu_idle_poll();
> -			} else {
> -				if (!current_clr_polling_and_test()) {
> -					cpuidle_idle_call();
> -				} else {
> -					local_irq_enable();
> -				}
> -				__current_set_polling();
> -			}
> +			else
> +				cpu_idle_call();
> +
>   			arch_cpu_idle_exit();
>   			/*
>   			 * We need to test and propagate the TIF_NEED_RESCHED
>


-- 
  <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