Re: [PATCH v1 1/5] cpuidle: menu: Clean up variables usage in menu_select()

[Rafael J. Wysocki]

On Sunday, August 12, 2018 6:09:27 PM CEST Leo Yan wrote:
> The usage for two variables 'data->predicted_us' and 'expected_interval'
> in menu_select() are confused, especially these two variables are
> assigned with each other: firstly 'data->predicted_us' is assigned to
> the minimum value between 'data->predicted_us' and 'expected_interval',
> so it presents the prediction period for taking account different
> factors and include consideration for expected interval; but later
> 'data->predicted_us' is assigned back to 'expected_interval' and from
> then on the function uses 'expected_interval' to select idle state; this
> results in 'expected_interval' has two different semantics between the
> top half and the bottom half of the same function.
> 
> This patch is to clean up the usage of these two variables, we always
> use 'data->predicted_us' to present the idle duration predictions and
> it can be used to compare with idle state target residency or tick
> boundary for choosing idle state; we purely use 'expected_interval' to
> record the expected interval value, which is mainly for interval
> interrupt estimation.
> 
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
>  drivers/cpuidle/governors/menu.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 5eb7d6f..b972db1 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -363,7 +363,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		latency_req = interactivity_req;
>  
>  select:
> -	expected_interval = data->predicted_us;
>  	/*
>  	 * Find the idle state with the lowest power while satisfying
>  	 * our constraints.
> @@ -386,7 +385,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  			 * expected idle duration so that the tick is retained
>  			 * as long as that target residency is low enough.
>  			 */
> -			expected_interval = drv->states[idx].target_residency;
> +			data->predicted_us = drv->states[idx].target_residency;

This is not what is predicted though, so the name of the field isn't quite
adequate for this use IMO.

Besides, I'm not sure in what way using a structure field is simpler than
using a local variable.

>  			break;
>  		}
>  		idx = i;
> @@ -400,7 +399,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  	 * expected idle duration is shorter than the tick period length.
>  	 */
>  	if (((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) ||
> -	    expected_interval < TICK_USEC) && !tick_nohz_tick_stopped()) {
> +	    data->predicted_us < TICK_USEC) && !tick_nohz_tick_stopped()) {
>  		unsigned int delta_next_us = ktime_to_us(delta_next);
>  
>  		*stop_tick = false;
> 

Yes, it can be simplified along these lines, but then please note that
data->predicted_us is only used in menu_select(), so it doesn't even need to
be there in struct menu_device.  And if you make it a local variable and
call it something like duration_us, then yes, it will be fine to use it like
in this patch in my view.

Thanks,
Rafael