linux-kernel - Re: [PATCH v1 1/5] cpuidle: menu: Clean up variables usage in menu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Aug 2018 10:32:14 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Ramesh Thomas <ramesh.thomas@...el.com>,
        linux-kernel@...r.kernel.org, Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v1 1/5] cpuidle: menu: Clean up variables usage in menu_select()

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@...aro.org>
> ---
>  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