[PATCH] cpufreq: conservative: fix requested_freq reduction issue

[PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Xiaoguang Chen]

When decreasing frequency, requested_freq may be less than
freq_target, So requested_freq minus freq_target may be negative,
But reqested_freq's unit is unsigned int, then the negative result
will be one larger interger which may be even higher than
requested_freq.

This patch is to fix such issue. when result becomes negative,
set requested_freq as the min value of policy.

Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
---
 drivers/cpufreq/cpufreq_conservative.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index f62d822..218460f 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -80,13 +80,18 @@ static void cs_check_cpu(int cpu, unsigned int load)
 
 	/* Check for frequency decrease */
 	if (load < cs_tuners->down_threshold) {
+		unsigned int freq_target;
 		/*
 		 * if we cannot reduce the frequency anymore, break out early
 		 */
 		if (policy->cur == policy->min)
 			return;
 
-		dbs_info->requested_freq -= get_freq_target(cs_tuners, policy);
+		freq_target = get_freq_target(cs_tuners, policy);
+		if (dbs_info->requested_freq > freq_target)
+			dbs_info->requested_freq -= freq_target;
+		else
+			dbs_info->requested_freq = policy->min;
 
 		__cpufreq_driver_target(policy, dbs_info->requested_freq,
 				CPUFREQ_RELATION_L);
-- 
1.8.0

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 7 November 2013 07:58, Xiaoguang Chen <chenxg@marvell.com> wrote:
> When decreasing frequency, requested_freq may be less than
> freq_target, So requested_freq minus freq_target may be negative,
> But reqested_freq's unit is unsigned int, then the negative result
> will be one larger interger which may be even higher than
> requested_freq.
>
> This patch is to fix such issue. when result becomes negative,
> set requested_freq as the min value of policy.
>
> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
> ---
>  drivers/cpufreq/cpufreq_conservative.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Good Catch.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

We need another patch for fixing the other part of code where we
increase freq..
We need to replace:

if (dbs_info->requested_freq == policy->max)
    return;

with

if (dbs_info->requested_freq >= policy->max)
    return;

So, that we don't run unnecessary code :)

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Rafael J. Wysocki]

On Thursday, November 07, 2013 10:39:38 AM Viresh Kumar wrote:
> On 7 November 2013 07:58, Xiaoguang Chen <chenxg@marvell.com> wrote:
> > When decreasing frequency, requested_freq may be less than
> > freq_target, So requested_freq minus freq_target may be negative,
> > But reqested_freq's unit is unsigned int, then the negative result
> > will be one larger interger which may be even higher than
> > requested_freq.
> >
> > This patch is to fix such issue. when result becomes negative,
> > set requested_freq as the min value of policy.
> >
> > Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
> > ---
> >  drivers/cpufreq/cpufreq_conservative.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Good Catch.
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied, thanks!

> We need another patch for fixing the other part of code where we
> increase freq..
> We need to replace:
> 
> if (dbs_info->requested_freq == policy->max)
>     return;
> 
> with
> 
> if (dbs_info->requested_freq >= policy->max)
>     return;
> 
> So, that we don't run unnecessary code :)

Care to prepare a patch?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 8 November 2013 00:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, November 07, 2013 10:39:38 AM Viresh Kumar wrote:

>> We need another patch for fixing the other part of code where we
>> increase freq..
>> We need to replace:
>>
>> if (dbs_info->requested_freq == policy->max)
>>     return;
>>
>> with
>>
>> if (dbs_info->requested_freq >= policy->max)
>>     return;
>>
>> So, that we don't run unnecessary code :)
>
> Care to prepare a patch?

I wanted to give a chance to Xiaoguang to generate a patch for us :)
Lets wait for his reply, otherwise I will do it.

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 8 November 2013 00:36, Stratos Karafotis <skarafotis@gmail.com> wrote:
> I think the existing code already checks if the requested_freq is greater
> than policy->max in __cpufreq_driver_target.

Yes it does. But the problem is:
- cs_check_cpu() sets requested_freq above policy->max
- We execute following code because (requested_freq != policy->max)

    dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
    __cpufreq_driver_target(policy, dbs_info->requested_freq,
CPUFREQ_RELATION_H);
- In __cpufreq_driver_target(), we don't do anything and return early..
- Above will keep on repeating all the time..

If we change the code as I have suggested it to be:
- After first loop where requested_freq went over policy->max, we will
return early from cs_check_cpu(), but we have already set freq to max..

> If we put this check earlier, cpufreq will never reach policy->max.

Can you please explain why do you see that happening?

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Xiaoguang Chen]

Hi, Viresh, Sorry for the late reply.
I'll prepare the patch.
BTW, do you think we should set requeste_freq to policy->max when such
condition happens?

Thanks
Xiaoguang

2013/11/8 Viresh Kumar <viresh.kumar@linaro.org>:
> On 8 November 2013 00:36, Stratos Karafotis <skarafotis@gmail.com> wrote:
>> I think the existing code already checks if the requested_freq is greater
>> than policy->max in __cpufreq_driver_target.
>
> Yes it does. But the problem is:
> - cs_check_cpu() sets requested_freq above policy->max
> - We execute following code because (requested_freq != policy->max)
>
>     dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
>     __cpufreq_driver_target(policy, dbs_info->requested_freq,
> CPUFREQ_RELATION_H);
> - In __cpufreq_driver_target(), we don't do anything and return early..
> - Above will keep on repeating all the time..
>
> If we change the code as I have suggested it to be:
> - After first loop where requested_freq went over policy->max, we will
> return early from cs_check_cpu(), but we have already set freq to max..
>
>> If we put this check earlier, cpufreq will never reach policy->max.
>
> Can you please explain why do you see that happening?

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 8 November 2013 10:31, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
> Hi, Viresh, Sorry for the late reply.

That's fine :)

> I'll prepare the patch.

Thanks.

> BTW, do you think we should set requeste_freq to policy->max when such
> condition happens?

I thought about that earlier, but then thought this would be a cleaner solution.
And guess what, I was wrong.. There is one scenario for which we need to
set requested_freq correctly..

Suppose we have requested_freq currently greater than policy->max and load
decreases. We will start decreasing requested_freq but it will take
one iteration
to make it equal to max, which is not we want.. So we actually need to set
it to max when it gets over it.

So, don't do the change I asked for, i.e. replacing == with >=. But
update existing
code:

dbs_info->requested_freq += get_freq_target(cs_tuners, policy);

this way:

dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
if (dbs_info->requested_freq > policy->max)
    dbs_info->requested_freq = policy->max;

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Xiaoguang Chen]

2013/11/8 Viresh Kumar <viresh.kumar@linaro.org>:
> On 8 November 2013 10:31, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
>> Hi, Viresh, Sorry for the late reply.
>
> That's fine :)
>
>> I'll prepare the patch.
>
> Thanks.
>
>> BTW, do you think we should set requeste_freq to policy->max when such
>> condition happens?
>
> I thought about that earlier, but then thought this would be a cleaner solution.
> And guess what, I was wrong.. There is one scenario for which we need to
> set requested_freq correctly..
>
> Suppose we have requested_freq currently greater than policy->max and load
> decreases. We will start decreasing requested_freq but it will take
> one iteration
> to make it equal to max, which is not we want.. So we actually need to set
> it to max when it gets over it.
>
> So, don't do the change I asked for, i.e. replacing == with >=. But
> update existing
> code:
>
> dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
>
> this way:
>
> dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
> if (dbs_info->requested_freq > policy->max)
>     dbs_info->requested_freq = policy->max;

OK, patch will be sent out later

Xiaoguang

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Stratos Karafotis]

On Fri, Nov 8, 2013 at 6:55 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 November 2013 00:36, Stratos Karafotis <skarafotis@gmail.com> wrote:
>> I think the existing code already checks if the requested_freq is greater
>> than policy->max in __cpufreq_driver_target.
>
> Yes it does. But the problem is:
> - cs_check_cpu() sets requested_freq above policy->max
> - We execute following code because (requested_freq != policy->max)
>
>     dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
>     __cpufreq_driver_target(policy, dbs_info->requested_freq,
> CPUFREQ_RELATION_H);
> - In __cpufreq_driver_target(), we don't do anything and return early..
> - Above will keep on repeating all the time..
>
> If we change the code as I have suggested it to be:
> - After first loop where requested_freq went over policy->max, we will
> return early from cs_check_cpu(), but we have already set freq to max..
>
>> If we put this check earlier, cpufreq will never reach policy->max.
>
> Can you please explain why do you see that happening?

Please let me rephrase my previous post. In some circumstances (depending
on freq_step and freq_table values) CPU frequency will never reach to
policy->max.

For example suppose that (for simplicity values in MHz):
policy->max = 1000
policy->cur = 800
requested_freq = 800
freq_target = 300

In 'first' iteration, if we return early with this code (because
requested_freq will be
1100):
if (dbs_info->requested_freq >= policy->max)
     return;

CPU freq will never go over 800MHz.

I think the current code works correctly.
- The requested freq will go to 1100 in first iteration.
- __cpufreq_driver_target will change CPU freq to 1000
- dbs_cpufreq_notifier will adjust the requested_freq to 1000

In next iteration the code:
if (dbs_info->requested_freq == policy->max)
        return;

will keep the freq to max and break out early.

So, I think there is no need for an extra check because of
dbs_cpufreq_notifier code.


Thanks,
Stratos Karafotis

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 8 November 2013 23:13, Stratos Karafotis <skarafotis@gmail.com> wrote:
> Please let me rephrase my previous post. In some circumstances (depending
> on freq_step and freq_table values) CPU frequency will never reach to
> policy->max.
>
> For example suppose that (for simplicity values in MHz):
> policy->max = 1000
> policy->cur = 800
> requested_freq = 800
> freq_target = 300
>
> In 'first' iteration, if we return early with this code (because
> requested_freq will be
> 1100):
> if (dbs_info->requested_freq >= policy->max)
>      return;

That's not correct. At this point requested_freq would have been
800 only, and would have increased after this instruction to 1100.
So, in the first transition we will go to max freq, but not from the
second.

Though this piece of code is more simplified by the new solution
I gave.

> CPU freq will never go over 800MHz.
>
> I think the current code works correctly.
> - The requested freq will go to 1100 in first iteration.
> - __cpufreq_driver_target will change CPU freq to 1000
> - dbs_cpufreq_notifier will adjust the requested_freq to 1000

> So, I think there is no need for an extra check because of
> dbs_cpufreq_notifier code.

Now with the new code in place we are correcting requested_freq
in cs_check_cpu(), then why do we need dbs_cpufreq_notifier()?

What do you think?

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Stratos Karafotis]

On Fri, Nov 8, 2013 at 8:16 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 November 2013 23:13, Stratos Karafotis <skarafotis@gmail.com> wrote:
>> Please let me rephrase my previous post. In some circumstances (depending
>> on freq_step and freq_table values) CPU frequency will never reach to
>> policy->max.
>>
>> For example suppose that (for simplicity values in MHz):
>> policy->max = 1000
>> policy->cur = 800
>> requested_freq = 800
>> freq_target = 300
>>
>> In 'first' iteration, if we return early with this code (because
>> requested_freq will be
>> 1100):
>> if (dbs_info->requested_freq >= policy->max)
>>      return;
>
> That's not correct. At this point requested_freq would have been
> 800 only, and would have increased after this instruction to 1100.
> So, in the first transition we will go to max freq, but not from the
> second.
>
> Though this piece of code is more simplified by the new solution
> I gave.
>

Yes, you are right.

>> CPU freq will never go over 800MHz.
>>
>> I think the current code works correctly.
>> - The requested freq will go to 1100 in first iteration.
>> - __cpufreq_driver_target will change CPU freq to 1000
>> - dbs_cpufreq_notifier will adjust the requested_freq to 1000
>
>> So, I think there is no need for an extra check because of
>> dbs_cpufreq_notifier code.
>
> Now with the new code in place we are correcting requested_freq
> in cs_check_cpu(), then why do we need dbs_cpufreq_notifier()?
>
> What do you think?

I removed the check you proposed in this commit 934dac1ea072 to avoid
the duplicate check in cs_check_cpu and in dbs_cpufreq_notifier.

I agree that we don't need dbs_cpufreq_notifier if we transfer checks in
cs_check_cpu. But I'm not 100% sure if the notifier also covers
other cases and if it can be safely removed.


Stratos Karafotis

Re: [PATCH] cpufreq: conservative: fix requested_freq reduction issue

[Viresh Kumar]

On 9 November 2013 00:59, Stratos Karafotis <skarafotis@gmail.com> wrote:
> I removed the check you proposed in this commit 934dac1ea072 to avoid
> the duplicate check in cs_check_cpu and in dbs_cpufreq_notifier.
>
> I agree that we don't need dbs_cpufreq_notifier if we transfer checks in
> cs_check_cpu. But I'm not 100% sure if the notifier also covers
> other cases and if it can be safely removed.

It is there to take care of out-of-sync issues, and was introduced by this
commit, so probably it will stay as is:

commit a8d7c3bc2396aff14f9e920677072cb55b016040
Author: Elias Oltmanns <eo@nebensachen.de>
Date:   Mon Oct 22 09:50:13 2007 +0200

    [CPUFREQ] Make cpufreq_conservative handle out-of-sync events properly

    Make cpufreq_conservative handle out-of-sync events properly

    Currently, the cpufreq_conservative governor doesn't get notified when the
    actual frequency the cpu is running at differs from what cpufreq thought it
    was. As a result the cpu may stay at the maximum frequency after a s2ram /
    resume cycle even though the system is idle.

    Signed-off-by: Elias Oltmanns <eo@nebensachen.de>
    Signed-off-by: Dave Jones <davej@redhat.com>
---
 drivers/cpufreq/cpufreq_conservative.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)