Message-ID: <53955CE0.1090408@linux.vnet.ibm.com>
Date:	Mon, 09 Jun 2014 12:36:08 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>, rjw@...ysocki.net
CC:	linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, arvind.chauhan@....com,
	svaidy@...ux.vnet.ibm.com, ego@...ux.vnet.ibm.com, pavel@....cz
Subject: Re: [PATCH] cpufreq: governor: remove copy_prev_load from 'struct
 cpu_dbs_common_info'

On 06/09/2014 10:54 AM, Viresh Kumar wrote:
> 'copy_prev_load' was recently added by commit: 18b46ab (cpufreq: governor: Be
> friendly towards latency-sensitive bursty workloads).
> 
> It actually is a bit redundant as we also have 'prev_load' which can store any
> integer value and can be used instead of 'copy_prev_load' by setting it to zero
> when we don't want to use previous load.
> 
> So, drop 'copy_prev_load' and use 'prev_load' instead.

Well, to be honest, I'm not a great fan of writing tricky code (i.e.,
overloading multiple semantics onto one single thing). However, in this case,
it does save a bool for every CPU in the system, and this savings might be
worthwhile on some systems.

Perhaps you should also mention in the changelog that the true load can also
turn out to be zero during long idle intervals (and hence the actual value of
'prev_load' and the overloaded value can clash). However this is not a problem
because, if the true load was really zero in the previous interval, it makes
more sense to evaluate the load afresh for the current interval rather than
copying the previous load.

> 
> Update comments as well to make it more clear.
> 
> There is another change here which was probably missed by Srivatsa during the
> last version of updates he made. The unlikely in the 'if' statement was covering
> only half of the condition and the whole line should actually come under it.
> 

Well, I had thought about that too, but I felt that the second AND condition
was actually not that infrequent when compared to the first AND condition. And
the C language already guarantees that the second part won't be evaluated if
the first one turns out to be false. So I thought of making use of that and
hence wrapped only the first part within unlikely(), so that we can just
evaluate a subset of the 'if' condition most of the time. But anyway, your
version looks good too, so I don't mind.

> Also checkpatch is made more silent as it was reporting this (--strict option):
> 
> CHECK: Alignment should match open parenthesis
> +		if (unlikely(wall_time > (2 * sampling_rate) &&
> +						j_cdbs->prev_load)) {

Sure, this one is a welcome cleanup, thanks!

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/cpufreq/cpufreq_governor.c | 13 ++++++++-----
>  drivers/cpufreq/cpufreq_governor.h |  8 ++++----
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 9004450..a1ad804 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -132,14 +132,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  		 * an unusually large 'wall_time' (as compared to the sampling
>  		 * rate) indicates this scenario.
>  		 */
> -		if (unlikely(wall_time > (2 * sampling_rate)) &&
> -						j_cdbs->copy_prev_load) {
> +		if (unlikely(wall_time > (2 * sampling_rate) &&
> +			     j_cdbs->prev_load)) {
>  			load = j_cdbs->prev_load;
> -			j_cdbs->copy_prev_load = false;
> +
> +			/*
> +			 * Ensure that we copy the previous load only once, upon
> +			 * the first wake-up from idle.
> +			 */

Can you also add the term 'destructive copy' to the comment, while you are at it?
Because, that term (which means that we destroy the source during a copy) explains
the situation very aptly. Something like this:

Perform a destructive copy, to ensure that we copy the previous load only once,
upon the first wake-up from idle.

> +			j_cdbs->prev_load = 0;
>  		} else {
>  			load = 100 * (wall_time - idle_time) / wall_time;
>  			j_cdbs->prev_load = load;
> -			j_cdbs->copy_prev_load = true;
>  		}
> 
>  		if (load > max_load)
> @@ -373,7 +377,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  				(j_cdbs->prev_cpu_wall - j_cdbs->prev_cpu_idle);
>  			j_cdbs->prev_load = 100 * prev_load /
>  					(unsigned int) j_cdbs->prev_cpu_wall;
> -			j_cdbs->copy_prev_load = true;
> 
>  			if (ignore_nice)
>  				j_cdbs->prev_cpu_nice =
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index c2a5b7e..d3082ee 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -134,12 +134,12 @@ struct cpu_dbs_common_info {
>  	u64 prev_cpu_idle;
>  	u64 prev_cpu_wall;
>  	u64 prev_cpu_nice;
> -	unsigned int prev_load;
>  	/*
> -	 * Flag to ensure that we copy the previous load only once, upon the
> -	 * first wake-up from idle.
> +	 * Used to store system load before going into idle, when set to zero:
> +	 * used as a flag to ensure that we copy the previous load only once,
> +	 * upon the first wake-up from idle.

Perhaps a bit of a touch-up to the comment would be worthwhile. Something
like this:

Used to keep track of the load in the previous interval. However, when
explicitly set to zero, it is used as a flag to ensure that we copy the
previous load to the current interval only once, upon the first wake-up
from idle.

>  	 */
> -	bool copy_prev_load;
> +	unsigned int prev_load;
>  	struct cpufreq_policy *cur_policy;
>  	struct delayed_work work;
>  	/*
> 

Thank you!

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists