[PATCH] cpufreq: governor: remove copy_prev_load from 'struct cpu_dbs_common_info'

[PATCH] cpufreq: governor: remove copy_prev_load from 'struct cpu_dbs_common_info'

[Viresh Kumar]

'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.

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.

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)) {

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.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.
+			 */
+			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.
 	 */
-	bool copy_prev_load;
+	unsigned int prev_load;
 	struct cpufreq_policy *cur_policy;
 	struct delayed_work work;
 	/*
-- 
2.0.0.rc2

Re: [PATCH] cpufreq: governor: remove copy_prev_load from 'struct cpu_dbs_common_info'

[Srivatsa S. Bhat]

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@linaro.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

Re: [PATCH] cpufreq: governor: remove copy_prev_load from 'struct cpu_dbs_common_info'

[Viresh Kumar]

On 9 June 2014 12:36, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 06/09/2014 10:54 AM, Viresh Kumar wrote:

> 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,

I am somewhat, but it shouldn't be confusing at all. Here the same variable
can be used without changing its meaning and that's why I did it :)

> 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.

Hmm.

>> 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.

I am not a compiler expert but that sounds confusing :(, not sure.

This looks wrong: "so that we can just evaluate a subset of the 'if'
condition most
of the time"..

It doesn't matter what we got into the unlikely() block, C will work the way it
is supposed to. And we will evaluate only the first one here if it fails.

AFAIK, unlikely() is just a compiler flag and doesn't have anything to do with
how code is going to execute.

The only use of unlikely() is to avoid branches (Which will also mean avoid
pipeline flushes and better performance). So, with only half of the expression
in unlikely there are still some chances that we will do a branch to reach the
else block and as that's the most common case overall performance will get
hit..

And so we *must* get complete expression inside unlikely() ..

> 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@linaro.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.

ok.

>> +                     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.

ok.

Re: [PATCH] cpufreq: governor: remove copy_prev_load from 'struct cpu_dbs_common_info'

[Pavel Machek]

On Mon 2014-06-09 10:54:17, 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.
> 
> 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.
> 
> 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)) {
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html