Re: [PATCH 4/6] cpuidle: Handle TIF_NR_POLLING on behalf of CPUIDLE_FLAG_MWAIT states

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Frederic,

On 1/2/2025 8:31 PM, Frederic Weisbecker wrote:
> [..snip..]
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 621696269584..9eece3df1080 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -114,12 +114,13 @@ void __cpuidle default_idle_call(void)
>   		stop_critical_timings();
>   
>   		ct_cpuidle_enter();
> -		arch_cpu_idle();
> +		arch_cpu_idle(); // XXX assumes !polling
>   		ct_cpuidle_exit();
>   
>   		start_critical_timings();
>   		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
>   		cond_tick_broadcast_exit();
> +		__current_set_polling();
>   	}
>   	local_irq_enable();
>   	instrumentation_end();
> @@ -128,31 +129,14 @@ void __cpuidle default_idle_call(void)
>   static int call_cpuidle_s2idle(struct cpuidle_driver *drv,
>   			       struct cpuidle_device *dev)
>   {
> +	int ret;
> +
>   	if (current_clr_polling_and_test())
>   		return -EBUSY;
>   
> -	return cpuidle_enter_s2idle(drv, dev);
> -}
> -
> -static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> -		      int next_state)
> -{
> -	/*
> -	 * The idle task must be scheduled, it is pointless to go to idle, just
> -	 * update no idle residency and return.
> -	 */
> -	if (current_clr_polling_and_test()) {

nit.

Previously, if TIF_NEED_RESCHED was set by this point, the CPU would
bail out early before entering the idle state but with this change, I
believe only at need_resched() in mwait_idle_with_hints() do we realize
we have a pending IPI / task wakeup. Is this a concern?

In my testing with ipistorm I could not see any difference in IPI
throughput to polling idle CPUs but a bailout before the entry method
for need_resched() can save a few cycles.

-- 
Thanks and Regards,
Prateek

> -		dev->last_residency_ns = 0;
> -		local_irq_enable();
> -		return -EBUSY;
> -	}
> -
> -	/*
> -	 * Enter the idle state previously returned by the governor decision.
> -	 * This function will block until an interrupt occurs and will take
> -	 * care of re-enabling the local interrupts
> -	 */
> -	return cpuidle_enter(drv, dev, next_state);
> +	ret = cpuidle_enter_s2idle(drv, dev);
> +	__current_set_polling();
> +	return ret;
>   }
>   
>   /**
> @@ -213,7 +197,7 @@ static void cpuidle_idle_call(void)
>   		tick_nohz_idle_stop_tick();
>   
>   		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> -		call_cpuidle(drv, dev, next_state);
> +		cpuidle_enter(drv, dev, next_state);
>   	} else {
>   		bool stop_tick = true;
>   
> @@ -227,7 +211,12 @@ static void cpuidle_idle_call	(void)
>   		else
>   			tick_nohz_idle_retain_tick();
>   
> -		entered_state = call_cpuidle(drv, dev, next_state);
> +		/*
> +		 * Enter the idle state previously returned by the governor decision.
> +		 * This function will block until an interrupt occurs and will take
> +		 * care of re-enabling the local interrupts.
> +		 */
> +		entered_state = cpuidle_enter(drv, dev, next_state);
>   		/*
>   		 * Give the governor an opportunity to reflect on the outcome
>   		 */
> @@ -235,7 +224,6 @@ static void cpuidle_idle_call(void)
>   	}
>   
>   exit_idle:
> -	__current_set_polling();
>   
>   	/*
>   	 * It is up to the idle functions to re-enable local interrupts