Re: [PATCH 1/1] cpuidle/menu: avoid prioritizing physical state over polling state

[Christian Loehle <christian.loehle@arm.com>]

On 8/9/24 08:31, Aboorva Devarajan wrote:
> Update the cpuidle menu governor to avoid prioritizing physical states
> over polling states when predicted idle duration is lesser than the
> physical states target residency duration for performance gains.

I would use something like this as wording:

[PATCH] cpuidle: menu: Select polling on short predicted idle
Select a shallow state matching our predicted_ns even if it is a
polling one.

Additionally to querying the next timer event, menu also employs an
interval tracking strategy of most recent idle durations for workloads
that aren't woken up by predictable timer events. The logic predicts
the next wakeup as predicted_ns, but the logic handling that skipped
any polling states.
In the worst-case on POWER we might only have snooze as polling and
CEDE. This makes the entire logic around it a NOP as it never let's
predicted_ns choose an appropriate idle state since it requires a
non-polling one.
To actually enforce predicted_ns for idle state selection actually use
states even though they are polling ones.

Even for a perfect recent intervals of
[1, 1, 1, 1, 1, 1, 1, 1]
menu previously chose the first non-idle state.


-----

To mitigate potential side-effects since most platforms have a
shallower idle state we could add something based on that, but
really your patch would be the only consistent IMO.

I'm not quite sure why this was put there in the first place.
It was essentially introduced in
commit ("69d25870f20c cpuidle: fix the menu governor to boost IO performance")
with a 20us lower limit.

> 
> Signed-off-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
> ---
>  drivers/cpuidle/governors/menu.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index f3c9d49f0f2a..cf99ca103f9b 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -354,17 +354,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  			idx = i; /* first enabled state */
>  
>  		if (s->target_residency_ns > predicted_ns) {
> -			/*
> -			 * Use a physical idle state, not busy polling, unless
> -			 * a timer is going to trigger soon enough.
> -			 */
> -			if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> -			    s->exit_latency_ns <= latency_req &&
> -			    s->target_residency_ns <= data->next_timer_ns) {
> -				predicted_ns = s->target_residency_ns;
> -				idx = i;
> -				break;
> -			}
>  			if (predicted_ns < TICK_NSEC)
>  				break;
>