Re: Ask for help on governor

Re: Ask for help on governor

[Viresh Kumar]

On 12-12-17, 02:46, Andy Tang wrote:
> Hi,
> 
> I run into a question that conservative governor can't work while ondemand governor works well.
> When cpu load gets higher, if ondemand governor is used, cpu frequency can get higher and higher.
> But if I use conservative governor, cpu frequency stays on lowest frequency no matter what cpu load is.
> 
> This issue was found on kernel 4.14.0, on kernel 4.9 kernel. It acts well.

You mean to say that the problem exists on 4.14 but not on 4.9?

Strange, as we haven't seen any such reports from anyone else.

> I think it may related to cpufreq framework, not driver itself. Could you guys give me some clues on how to debug it?
> I don't expect a direct answer; any suggestions are welcome.

Please provide output of below:

grep . /sys/devices/system/cpu/cpufreq/policy*/*

And then you may need to bisect the problem as well by going to kernel version
in between 4.9 and 4.14.

-- 
viresh

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.11 23:31 Viresh Kumar wrote:
> On 12-12-17, 02:46, Andy Tang wrote:
>> Hi,
>> 
>> I run into a question that conservative governor can't work while ondemand governor works well.
>> When cpu load gets higher, if ondemand governor is used, cpu frequency can get higher and higher.
>> But if I use conservative governor, cpu frequency stays on lowest frequency no matter what cpu load is.
>> 
>> This issue was found on kernel 4.14.0, on kernel 4.9 kernel. It acts well.
>
> You mean to say that the problem exists on 4.14 but not on 4.9?
>
> Strange, as we haven't seen any such reports from anyone else.

Well, it is easy enough for us to check.
Indeed, it is broken in kernel 4.14-rc1, but O.K. in kernel 4.13.
It remains broken in kernel 4.15-rc1 (I am not on -rc3 yet).

Because it was easier for me to do so, I used intel_cpufreq (i.e. intel_pstate in passive mode).

>
>> I think it may related to cpufreq framework, not driver itself. Could you guys give me some clues on how to debug it?
>> I don't expect a direct answer; any suggestions are welcome.
>
> Please provide output of below:
>
> grep . /sys/devices/system/cpu/cpufreq/policy*/*
>
> And then you may need to bisect the problem as well by going to kernel version
> in between 4.9 and 4.14.

@Andy: Have you started bisecting the kernel yet? I suppose I could if you haven't,
or if you do not know how.

... Doug

Re: Ask for help on governor

[Rafael J. Wysocki]

On Tue, Dec 12, 2017 at 5:18 PM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2017.12.11 23:31 Viresh Kumar wrote:
>> On 12-12-17, 02:46, Andy Tang wrote:
>>> Hi,
>>>
>>> I run into a question that conservative governor can't work while ondemand governor works well.
>>> When cpu load gets higher, if ondemand governor is used, cpu frequency can get higher and higher.
>>> But if I use conservative governor, cpu frequency stays on lowest frequency no matter what cpu load is.
>>>
>>> This issue was found on kernel 4.14.0, on kernel 4.9 kernel. It acts well.
>>
>> You mean to say that the problem exists on 4.14 but not on 4.9?
>>
>> Strange, as we haven't seen any such reports from anyone else.
>
> Well, it is easy enough for us to check.
> Indeed, it is broken in kernel 4.14-rc1, but O.K. in kernel 4.13.
> It remains broken in kernel 4.15-rc1 (I am not on -rc3 yet).
>
> Because it was easier for me to do so, I used intel_cpufreq (i.e. intel_pstate in passive mode).
>
>>
>>> I think it may related to cpufreq framework, not driver itself. Could you guys give me some clues on how to debug it?
>>> I don't expect a direct answer; any suggestions are welcome.
>>
>> Please provide output of below:
>>
>> grep . /sys/devices/system/cpu/cpufreq/policy*/*
>>
>> And then you may need to bisect the problem as well by going to kernel version
>> in between 4.9 and 4.14.
>
> @Andy: Have you started bisecting the kernel yet? I suppose I could if you haven't,
> or if you do not know how.

There is only one commit touching conservative in 4.14:

2d045036322c cpufreq: governor: Drop min_sampling_rate

Maybe try to revert this one?

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.12 08:51 Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 5:18 PM, Doug Smythies <dsmythies@telus.net> wrote:
>> On 2017.12.11 23:31 Viresh Kumar wrote:
>>> On 12-12-17, 02:46, Andy Tang wrote:
>>>> Hi,
>>>>
>>>> I run into a question that conservative governor can't work while ondemand governor works well.
>>>> When cpu load gets higher, if ondemand governor is used, cpu frequency can get higher and higher.
>>>> But if I use conservative governor, cpu frequency stays on lowest frequency no matter what cpu load is.
>>>>
>>>> This issue was found on kernel 4.14.0, on kernel 4.9 kernel. It acts well.
>>>
>>> You mean to say that the problem exists on 4.14 but not on 4.9?
>>>
>>> Strange, as we haven't seen any such reports from anyone else.
>>
>> Well, it is easy enough for us to check.
>> Indeed, it is broken in kernel 4.14-rc1, but O.K. in kernel 4.13.
>> It remains broken in kernel 4.15-rc1 (I am not on -rc3 yet).
>>
>> Because it was easier for me to do so, I used intel_cpufreq (i.e. intel_pstate in passive mode).
>>
>>>
>>>> I think it may related to cpufreq framework, not driver itself. Could you guys give me some clues on how to debug it?
>>>> I don't expect a direct answer; any suggestions are welcome.
>>>
>>> Please provide output of below:
>>>
>>> grep . /sys/devices/system/cpu/cpufreq/policy*/*
>>>
>>> And then you may need to bisect the problem as well by going to kernel version
>>> in between 4.9 and 4.14.
>>
>> @Andy: Have you started bisecting the kernel yet? I suppose I could if you haven't,
>> or if you do not know how.
>
> There is only one commit touching conservative in 4.14:
> 
> 2d045036322c cpufreq: governor: Drop min_sampling_rate
>
> Maybe try to revert this one?

Thanks for the suggestion. It was so very close, the problem commit is the very
next one, aa7519af450d.

Bisect result:

aa7519af450d3c62a057aece24877c34562fa25a is the first bad commit
commit aa7519af450d3c62a057aece24877c34562fa25a
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Wed Jul 19 15:42:42 2017 +0530

    cpufreq: Use transition_delay_us for legacy governors as well

    The policy->transition_delay_us field is used only by the schedutil
    governor currently, and this field describes how fast the driver wants
    the cpufreq governor to change CPUs frequency. It should rather be a
    common thing across all governors, as it doesn't have any schedutil
    dependency here.

    Create a new helper cpufreq_policy_transition_delay_us() to get the
    transition delay across all governors.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: Ask for help on governor

[Viresh Kumar]

On 12-12-17, 19:10, Doug Smythies wrote:
> Thanks for the suggestion. It was so very close, the problem commit is the very
> next one, aa7519af450d.
> 
> Bisect result:
> 
> aa7519af450d3c62a057aece24877c34562fa25a is the first bad commit
> commit aa7519af450d3c62a057aece24877c34562fa25a
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Wed Jul 19 15:42:42 2017 +0530
> 
>     cpufreq: Use transition_delay_us for legacy governors as well
> 
>     The policy->transition_delay_us field is used only by the schedutil
>     governor currently, and this field describes how fast the driver wants
>     the cpufreq governor to change CPUs frequency. It should rather be a
>     common thing across all governors, as it doesn't have any schedutil
>     dependency here.
> 
>     Create a new helper cpufreq_policy_transition_delay_us() to get the
>     transition delay across all governors.
> 
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

What has changed for intel-pstate after this patch ?

sampling_rate for conservative and ondemand is 500 us now, which was 20000 us
earlier. So we reevaluate load very frequently now, but in case load isn't over
80% then we wouldn't increase the frequency.

@Doug/Andy: Can you please try following:

- Checkout 4.14 or mainline (the broken kernels).
- Set governor to conservative.
- Monitor /sys/devices/system/cpu/cpufreq/policy*/stats/total_trans (to check if
  frequency is getting changed or not).
- Run some dummy load, I did this:

  perf bench sched messaging --pipe --thread --group 8 

This should result in frequency updates, so its not that we aren't changing
frequency at all now with conservative.

Then do this:

echo 20000 > /sys/devices/system/cpu/cpufreq/conservative/sampling_rate

and this should take you back to the original behavior which was there pre-4.14.

@Andy: What's your hardware and cpufreq driver ? intel-pstate? As I am not sure
why things will change for any other driver than intel-pstate.

I am not really sure what's the right way forward here now.

-- 
viresh

RE: Ask for help on governor

[Andy Tang]

Hi,

Thanks you all for help. 
Anyway I found the root cause myself.

It was caused by commit: 00bfe05889e91b5112893b001e4a47b0a0f8bdd7.

In this commit, the part of code is as below:
+       if (policy_dbs->idle_periods < UINT_MAX) {
+               unsigned int freq_steps = policy_dbs->idle_periods * freq_step;
+
+               if (requested_freq > freq_steps)
+                       requested_freq -= freq_steps;
+               else
+                       requested_freq = policy->min;
+
+               policy_dbs->idle_periods = UINT_MAX;
+       }

On my platform, there are 3 frequency available which are: 1600Mhz, 800Mhz, 400Mhz.
Freq_steps is 80Mhz.
Due to this code, the requested_freq is always 480Mhz. Since we want 800Mhz as next target frequency, the cpu frequency can't be changed.

Please work out a patch to fix this.

This issue exists on all our LS* platforms.

Regards,
Andy

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Wednesday, December 13, 2017 2:18 PM
> To: Doug Smythies <dsmythies@telus.net>
> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; Andy Tang
> <andy.tang@nxp.com>; 'Rafael J. Wysocki' <rjw@rjwysocki.net>; 'Linux PM'
> <linux-pm@vger.kernel.org>
> Subject: Re: Ask for help on governor
> 
> On 12-12-17, 19:10, Doug Smythies wrote:
> > Thanks for the suggestion. It was so very close, the problem commit is
> > the very next one, aa7519af450d.
> >
> > Bisect result:
> >
> > aa7519af450d3c62a057aece24877c34562fa25a is the first bad commit
> > commit aa7519af450d3c62a057aece24877c34562fa25a
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Wed Jul 19 15:42:42 2017 +0530
> >
> >     cpufreq: Use transition_delay_us for legacy governors as well
> >
> >     The policy->transition_delay_us field is used only by the schedutil
> >     governor currently, and this field describes how fast the driver wants
> >     the cpufreq governor to change CPUs frequency. It should rather be a
> >     common thing across all governors, as it doesn't have any schedutil
> >     dependency here.
> >
> >     Create a new helper cpufreq_policy_transition_delay_us() to get the
> >     transition delay across all governors.
> >
> >     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> What has changed for intel-pstate after this patch ?
> 
> sampling_rate for conservative and ondemand is 500 us now, which was
> 20000 us earlier. So we reevaluate load very frequently now, but in case load
> isn't over 80% then we wouldn't increase the frequency.
> 
> @Doug/Andy: Can you please try following:
> 
> - Checkout 4.14 or mainline (the broken kernels).
> - Set governor to conservative.
> - Monitor /sys/devices/system/cpu/cpufreq/policy*/stats/total_trans (to
> check if
>   frequency is getting changed or not).
> - Run some dummy load, I did this:
> 
>   perf bench sched messaging --pipe --thread --group 8
> 
> This should result in frequency updates, so its not that we aren't changing
> frequency at all now with conservative.
> 
> Then do this:
> 
> echo 20000 > /sys/devices/system/cpu/cpufreq/conservative/sampling_rate
> 
> and this should take you back to the original behavior which was there pre-
> 4.14.
> 
> @Andy: What's your hardware and cpufreq driver ? intel-pstate? As I am not
> sure why things will change for any other driver than intel-pstate.
> 
> I am not really sure what's the right way forward here now.
> 
> --
> viresh

Re: Ask for help on governor

[Viresh Kumar]

On 13-12-17, 06:22, Andy Tang wrote:
> Hi,
> 
> Thanks you all for help. 
> Anyway I found the root cause myself.

Well, I was quite sure that the problem you reported is different than what
Dough was talking about..

> It was caused by commit: 00bfe05889e91b5112893b001e4a47b0a0f8bdd7.
> 
> In this commit, the part of code is as below:
> +       if (policy_dbs->idle_periods < UINT_MAX) {
> +               unsigned int freq_steps = policy_dbs->idle_periods * freq_step;
> +
> +               if (requested_freq > freq_steps)
> +                       requested_freq -= freq_steps;
> +               else
> +                       requested_freq = policy->min;
> +
> +               policy_dbs->idle_periods = UINT_MAX;
> +       }
> 
> On my platform, there are 3 frequency available which are: 1600Mhz, 800Mhz, 400Mhz.
> Freq_steps is 80Mhz.
> Due to this code, the requested_freq is always 480Mhz. Since we want 800Mhz as next target frequency, the cpu frequency can't be changed.

I am not sure I follow what you are saying. The above code is only taking care
of idle periods (i.e. while the CPU was idle and we couldn't decrease the
frequency). If there are no idle periods, then the requested_freq will increase
by 80 MHz every time we try to change the frequency and after the 5th call its
value will become 800 MHz and so we should see an update.

Have you tried reverting the commit 00bfe0588 on top of 4.14? Does it make your
regression go away?

> Please work out a patch to fix this.

Sure we can help, but I didn't understand the root cause yet.

> This issue exists on all our LS* platforms.

LS -> loongson ?

-- 
viresh

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.12 22:22 Andy Tang wrote:

> Hi,
>
> Thanks you all for help. 
> Anyway I found the root cause myself.
>
> It was caused by commit: 00bfe05889e91b5112893b001e4a47b0a0f8bdd7.

O.K. It seems my attempt to help merely diluted your original
issue, because it appears I found and bisected a different issue.

Sorry for the distraction.

... Doug

RE: Ask for help on governor

[Doug Smythies]

Note: adding Stratos, the commit author.
 
On 2017.12.12 22:22 Andy Tang wrote:

> Anyway I found the root cause myself.
>
> It was caused by commit: 00bfe05889e91b5112893b001e4a47b0a0f8bdd7.

Agreed.

Then why did my kernel bisection come to a different conclusion?
Because, in my case, the issue only manifested itself when the
sampling rate (which should really be called sampling period), became
low enough to bring the issue to the surface.

The other important thing to note for my system is that I use (O.K.,
steal) the Ubuntu kernel configuration and so my tick rate is 250 Hz, not
1000 Hz.

I think the math for the idle periods calculation breaks down here
(from drivers/cpufreq/cpufreq_governor.c):

                if (time_elapsed > 2 * sampling_rate) {
                        unsigned int periods = time_elapsed / sampling_rate;

                        if (periods < idle_periods)
                                idle_periods = periods;
                }

if 2 * sampling_rate is less than one jiffy.
I.E. isn't time_elapsed always exactly one jiffy for a fully loaded CPU?
Important note: on my system a jiffy is just over 4 milliseconds.
So, for my test which is 100% load on one CPU, basically, idle_periods is always 2,
maybe more and the conservative code is always resetting the target CPU frequency
to minimum.

For whatever reason, on my system, a frequency step of 5% will not raise the pstate,
even though it should (the math works out to 1790 MHz, or pstate 17, but I never see it.
If I raise the frequency step to anything else, the math makes complete sense. Example:
frequency step = 10% so 3800 * 0.1 + 1600 = 1980 which means I should see pstate 19 being
asked for. I do. However, it does not continue to increase because of the
idle_periods problem, driving it back as an intermediate calculation, so all I ever see is
requested pstate of 19.

O.K. so if all this is true, then a 1000 Hz kernel shouldn't have a problem. Sort of, it
doesn't. Why "sort of"? Because the default sample period of 500 usec is right
on the edge, and sometimes the requested pstate does drop as a result. I used this
command to watch the requested pstate:

watch -d -n 1 sudo rdmsr --bitfield 15:8 -d -a 0x199

(translation: watch the actual requested pstates for all CPUs, by reading the processor itself.)

and while for CPU 7, it should clamp at 38 (for my system) it doesn't. O.K. so just
increase the sampling period a little, to say 510 uSec, and then yes, it clamps at 38.

O.K. so finally, I reverted the commit (but in a cheating way):

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 58d4f4e..3493ca7 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -222,6 +222,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
                        max_load = load;
        }

+       idle_periods = 0;
        policy_dbs->idle_periods = idle_periods;

        return max_load;
 
And everything is fine.

... Doug

RE: Ask for help on governor

[Andy Tang]

> -----Original Message-----
> From: Doug Smythies [mailto:dsmythies@telus.net]
> Sent: Thursday, December 14, 2017 9:21 AM
> To: Andy Tang <andy.tang@nxp.com>; 'Viresh Kumar'
> <viresh.kumar@linaro.org>; 'Stratos Karafotis' <stratosk@semaphore.gr>
> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; 'Rafael J. Wysocki'
> <rjw@rjwysocki.net>; 'Linux PM' <linux-pm@vger.kernel.org>; Doug
> Smythies <dsmythies@telus.net>
> Subject: RE: Ask for help on governor
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c
> b/drivers/cpufreq/cpufreq_governor.c
> index 58d4f4e..3493ca7 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -222,6 +222,7 @@ unsigned int dbs_update(struct cpufreq_policy
> *policy)
>                         max_load = load;
>         }
> 
> +       idle_periods = 0;

With this line added, conservative governor works well.

Ondemand governor still can't work well.
As I mentioned in earlier email, load varies from different sampling_rate.

Regards,
Andy

>         policy_dbs->idle_periods = idle_periods;
> 
>         return max_load;
> 
> And everything is fine.
> 
> ... Doug
> 

Re: Ask for help on governor

[Stratos Karafotis]

Hi

On 14/12/2017 04:42 πμ, Andy Tang wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Smythies [mailto:dsmythies@telus.net]
>> Sent: Thursday, December 14, 2017 9:21 AM
>> To: Andy Tang <andy.tang@nxp.com>; 'Viresh Kumar'
>> <viresh.kumar@linaro.org>; 'Stratos Karafotis' <stratosk@semaphore.gr>
>> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; 'Rafael J. Wysocki'
>> <rjw@rjwysocki.net>; 'Linux PM' <linux-pm@vger.kernel.org>; Doug
>> Smythies <dsmythies@telus.net>
>> Subject: RE: Ask for help on governor
>>
>> diff --git a/drivers/cpufreq/cpufreq_governor.c
>> b/drivers/cpufreq/cpufreq_governor.c
>> index 58d4f4e..3493ca7 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -222,6 +222,7 @@ unsigned int dbs_update(struct cpufreq_policy
>> *policy)
>>                         max_load = load;
>>         }
>>
>> +       idle_periods = 0;
> 
> With this line added, conservative governor works well.
> 
> Ondemand governor still can't work well.
> As I mentioned in earlier email, load varies from different sampling_rate.
> 
> Regards,
> Andy
> 

I'm not sure I understood what the problem is (maybe because I can't see
the description of the issue in the quotes).

Commit 00bfe05889e91b5112893b001e4a47b0a0f8bdd7 affects only conservative
governor.
Do you have the same issue with ondemand too?

Stratos

RE: Ask for help on governor

[Doug Smythies]

Hi,

On 2017.12.14 Stratos Karafotis wrote:

> I'm not sure I understood what the problem is (maybe because I can't see
> the description of the issue in the quotes).

For my part of it, the problem is that the default sample_period for
the intel_cpufreq scaling driver (i.e. intel_pstate in passive mode)
is 500 uSec. Everything, as far as I can determine, becomes busted if
the sampling period is less than a jiffy.

By the way, the default sampling period for the acpi_cpufreq driver seems to be
10 milliseconds, and everything is fine. I do not know why the defaults for
the intel_cpufreq driver are so short, and suggest that they shouldn't be,
in which case your commit might be O.K.

>
> Commit 00bfe05889e91b5112893b001e4a47b0a0f8bdd7 affects only conservative
> governor.
> Do you have the same issue with ondemand too?

Yes, this entire thread has been about the conservative governor.

That was the first time Andy mentioned issues with on demand, but
I do see it misbehave with the intel_cpufreq default sample period
(a.k.a sampling_rate) of 500uSec. See another e-mail I send in a moment.

... Doug

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.13 18:42 Andy Tang wrote:
> On 2017.12.14 09:21 Doug wrote: 
>> 
>> diff --git a/drivers/cpufreq/cpufreq_governor.c
>> b/drivers/cpufreq/cpufreq_governor.c
>> index 58d4f4e..3493ca7 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -222,6 +222,7 @@ unsigned int dbs_update(struct cpufreq_policy
>> *policy)
>>                         max_load = load;
>>         }
>> 
>> +       idle_periods = 0;
>
> With this line added, conservative governor works well.
>
> Ondemand governor still can't work well.

Your original message said:

On 12-12-17, 02:46, Andy Tang wrote:
> I run into a question that conservative governor can't work while ondemand governor works well.

So, for my part of it, I never looked at ondemand.

> As I mentioned in earlier email, load varies from different sampling_rate.

Because that e-mail was html, a lot of people didn't get it, and it also is not
in the archives. Anyway:

On 2017.12.12 23:24 Andy wrote:

> I also found sampling_rate affects the CPU load dramatically.
>			sample rate
>			100000	10000	1000
> actual cpu load	1%		1%	1%
> cpu report load	1%		20%	40-100%
>
> So we can know that when sample_rate is higher than 10000us, cpufreq can not calculate the load correctly.

Can you tell us more details about how you did this test and how you know your data is correct?
In particular, what is your work/sleep frequency for your 1% load.
In my experience the work/sleep frequency has always been a very important consideration with these tests.
What is the tick rate of your kernel? Myself, I wouldn't expect a 1000 uSec sample period to work properly.

I used turbostat to verify my tests, and yes, on demand does start to misbehave if the sampling period
is short relative to the tick period (jiffy time), otherwise it was fine.

... Doug

RE: Ask for help on governor

[Andy Tang]

Hi,

> -----Original Message-----
> From: Doug Smythies [mailto:dsmythies@telus.net]
> Sent: Friday, December 15, 2017 9:31 AM
> To: Andy Tang <andy.tang@nxp.com>; 'Viresh Kumar'
> <viresh.kumar@linaro.org>; 'Stratos Karafotis' <stratosk@semaphore.gr>
> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; 'Rafael J. Wysocki'
> <rjw@rjwysocki.net>; 'Linux PM' <linux-pm@vger.kernel.org>
> Subject: RE: Ask for help on governor
> 
> On 2017.12.13 18:42 Andy Tang wrote:
> > On 2017.12.14 09:21 Doug wrote:
> >>
> >> diff --git a/drivers/cpufreq/cpufreq_governor.c
> >> b/drivers/cpufreq/cpufreq_governor.c
> >> index 58d4f4e..3493ca7 100644
> >> --- a/drivers/cpufreq/cpufreq_governor.c
> >> +++ b/drivers/cpufreq/cpufreq_governor.c
> >> @@ -222,6 +222,7 @@ unsigned int dbs_update(struct cpufreq_policy
> >> *policy)
> >>                         max_load = load;
> >>         }
> >>
> >> +       idle_periods = 0;
> >
> > With this line added, conservative governor works well.
> >
> > Ondemand governor still can't work well.
> 
> Your original message said:
> 
> On 12-12-17, 02:46, Andy Tang wrote:
> > I run into a question that conservative governor can't work while
> ondemand governor works well.
> 
> So, for my part of it, I never looked at ondemand.
Sorry for the misleading. Actually I observed two issues:
1. conservative governor can't increase cpu frequency even the load is high than 90%
2. CPU load can't be calculated correctly when sampling_rate is 1000 for both conservative and ondemand governor. At first I thought ondemand governor worked well. Actually I did not test it thoroughly. Ondemand governor can work only when cpu load is calculated correct.

> 
> > As I mentioned in earlier email, load varies from different sampling_rate.
> 
> Because that e-mail was html, a lot of people didn't get it, and it also is not in
> the archives. Anyway:
> 
> On 2017.12.12 23:24 Andy wrote:
> 
> > I also found sampling_rate affects the CPU load dramatically.
> >			sample rate
> >			100000	10000	1000
> > actual cpu load	1%		1%	1%
> > cpu report load	1%		20%	40-100%
> >
> > So we can know that when sample_rate is higher than 10000us, cpufreq
> can not calculate the load correctly.
> 
> Can you tell us more details about how you did this test and how you know
> your data is correct?
> In particular, what is your work/sleep frequency for your 1% load.
> In my experience the work/sleep frequency has always been a very
> important consideration with these tests.
> What is the tick rate of your kernel? Myself, I wouldn't expect a 1000 uSec
> sample period to work properly.
My tick rate is 250Hz.  And Yes, cpu load can't be calculated correctly.
We really should set sampling_rate a proper values as default if there are some sample rate don't work properly. We don't want to calculate a workable sampling_rate according to the tick rate manually.
 
That's odd too that you wouldn't expect a 1000 Usec sample period to work properly but set this value as default, even worse, you allow to set a lower value to it.

Is it better if we set a low limit of  sample period? 

Regards,
Andy
 
> 
> I used turbostat to verify my tests, and yes, on demand does start to
> misbehave if the sampling period is short relative to the tick period (jiffy
> time), otherwise it was fine.
> 
> ... Doug
> 

RE: Ask for help on governor

[Doug Smythies]

Hi Andy,

Could you please try the below reversion patch.
It is on top of kernel 4.15-rc3.

On my system, and for the intel_cpufrequ scaling driver,
the default sampling_rate goes back to 20000 (which works)
from 500 uSec (which doesn't), for both the conservative
and ondemand governors.

>From 2e914085991d02eb5e6a197aa75485f37742a535 Mon Sep 17 00:00:00 2001
From: Doug Smythies <dsmythies@telus.net>
Date: Thu, 14 Dec 2017 22:59:44 -0800
Subject: [PATCH] Revert "cpufreq: Use transition_delay_us for legacy governors as well"

This reverts commit aa7519af450d3c62a057aece24877c34562fa25a.

There were issues with the default sampling_rate becoming to
short for some versions of cpufreq conservative and ondemand
governors to operate properly. The idle periods and load
calculations broke down.
---
 drivers/cpufreq/cpufreq.c          | 26 --------------------------
 drivers/cpufreq/cpufreq_governor.c |  9 ++++++++-
 include/linux/cpufreq.h            |  1 -
 kernel/sched/cpufreq_schedutil.c   | 11 ++++++++++-
 4 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 41d148a..9489901 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -530,32 +530,6 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);

-unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
-{
-       unsigned int latency;
-
-       if (policy->transition_delay_us)
-               return policy->transition_delay_us;
-
-       latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
-       if (latency) {
-               /*
-                * For platforms that can change the frequency very fast (< 10
-                * us), the above formula gives a decent transition delay. But
-                * for platforms where transition_latency is in milliseconds, it
-                * ends up giving unrealistic values.
-                *
-                * Cap the default transition delay to 10 ms, which seems to be
-                * a reasonable amount of time after which we should reevaluate
-                * the frequency.
-                */
-               return min(latency * LATENCY_MULTIPLIER, (unsigned int)10000);
-       }
-
-       return LATENCY_MULTIPLIER;
-}
-EXPORT_SYMBOL_GPL(cpufreq_policy_transition_delay_us);
-
 /*********************************************************************
  *                          SYSFS INTERFACE                          *
  *********************************************************************/
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 58d4f4e..6302eb4 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -392,6 +392,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
        struct dbs_governor *gov = dbs_governor_of(policy);
        struct dbs_data *dbs_data;
        struct policy_dbs_info *policy_dbs;
+       unsigned int latency;
        int ret = 0;

        /* State should be equivalent to EXIT */
@@ -430,7 +431,13 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
        if (ret)
                goto free_policy_dbs_info;

-       dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
+       /* policy latency is in ns. Convert it to us first */
+       latency = policy->cpuinfo.transition_latency / 1000;
+       if (latency == 0)
+               latency = 1;
+
+       /* Bring kernel and HW constraints together */
+       dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;

        if (!have_governor_per_policy())
                gov->gdbs_data = dbs_data;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 065f3a8..75d5bd9 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -533,7 +533,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
                                   unsigned int relation);
 unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
                                         unsigned int target_freq);
-unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy);
 int cpufreq_register_governor(struct cpufreq_governor *governor);
 void cpufreq_unregister_governor(struct cpufreq_governor *governor);

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 2f52ec0..a61d7fa 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -583,7 +583,16 @@ static int sugov_init(struct cpufreq_policy *policy)
                goto stop_kthread;
        }

-       tunables->rate_limit_us = cpufreq_policy_transition_delay_us(policy);
+       if (policy->transition_delay_us) {
+               tunables->rate_limit_us = policy->transition_delay_us;
+       } else {
+               unsigned int lat;
+
+               tunables->rate_limit_us = LATENCY_MULTIPLIER;
+               lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
+               if (lat)
+                       tunables->rate_limit_us *= lat;
+       }

        policy->governor_data = sg_policy;
        sg_policy->tunables = tunables;
--
2.7.4

RE: Ask for help on governor

[Andy Tang]

Hi Doug,

> -----Original Message-----
> From: Doug Smythies [mailto:dsmythies@telus.net]
> Sent: Friday, December 15, 2017 3:38 PM
> To: Andy Tang <andy.tang@nxp.com>
> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; 'Rafael J. Wysocki'
> <rjw@rjwysocki.net>; 'Linux PM' <linux-pm@vger.kernel.org>; 'Viresh
> Kumar' <viresh.kumar@linaro.org>; 'Stratos Karafotis'
> <stratosk@semaphore.gr>
> Subject: RE: Ask for help on governor
> 
> Hi Andy,
> 
> Could you please try the below reversion patch.
> It is on top of kernel 4.15-rc3.
Tried, no any effects.

> 
> On my system, and for the intel_cpufrequ scaling driver, the default
> sampling_rate goes back to 20000 (which works) from 500 uSec (which
> doesn't), for both the conservative and ondemand governors.
If I set the sampling_rate to 20,000 (actually 10,000 is enough), cpufreq works too for both the conservative and ondemand governors.
But on my platforms, the default rate is 1000 which leads to cpu load calculating issue.

Regards,
Andy

> 
> From 2e914085991d02eb5e6a197aa75485f37742a535 Mon Sep 17 00:00:00
> 2001
> From: Doug Smythies <dsmythies@telus.net>
> Date: Thu, 14 Dec 2017 22:59:44 -0800
> Subject: [PATCH] Revert "cpufreq: Use transition_delay_us for legacy
> governors as well"
> 
> This reverts commit aa7519af450d3c62a057aece24877c34562fa25a.
> 
> There were issues with the default sampling_rate becoming to short for
> some versions of cpufreq conservative and ondemand governors to operate
> properly. The idle periods and load calculations broke down.
> ---
>  drivers/cpufreq/cpufreq.c          | 26 --------------------------
>  drivers/cpufreq/cpufreq_governor.c |  9 ++++++++-
>  include/linux/cpufreq.h            |  1 -
>  kernel/sched/cpufreq_schedutil.c   | 11 ++++++++++-
>  4 files changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index
> 41d148a..9489901 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -530,32 +530,6 @@ unsigned int cpufreq_driver_resolve_freq(struct
> cpufreq_policy *policy,  }
> EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
> 
> -unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy
> *policy) -{
> -       unsigned int latency;
> -
> -       if (policy->transition_delay_us)
> -               return policy->transition_delay_us;
> -
> -       latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> -       if (latency) {
> -               /*
> -                * For platforms that can change the frequency very fast (< 10
> -                * us), the above formula gives a decent transition delay. But
> -                * for platforms where transition_latency is in milliseconds, it
> -                * ends up giving unrealistic values.
> -                *
> -                * Cap the default transition delay to 10 ms, which seems to be
> -                * a reasonable amount of time after which we should reevaluate
> -                * the frequency.
> -                */
> -               return min(latency * LATENCY_MULTIPLIER, (unsigned int)10000);
> -       }
> -
> -       return LATENCY_MULTIPLIER;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_policy_transition_delay_us);
> -
> 
> /**********************************************************
> ***********
>   *                          SYSFS INTERFACE                          *
> 
> **********************************************************
> ***********/
> diff --git a/drivers/cpufreq/cpufreq_governor.c
> b/drivers/cpufreq/cpufreq_governor.c
> index 58d4f4e..6302eb4 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -392,6 +392,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy
> *policy)
>         struct dbs_governor *gov = dbs_governor_of(policy);
>         struct dbs_data *dbs_data;
>         struct policy_dbs_info *policy_dbs;
> +       unsigned int latency;
>         int ret = 0;
> 
>         /* State should be equivalent to EXIT */ @@ -430,7 +431,13 @@ int
> cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
>         if (ret)
>                 goto free_policy_dbs_info;
> 
> -       dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
> +       /* policy latency is in ns. Convert it to us first */
> +       latency = policy->cpuinfo.transition_latency / 1000;
> +       if (latency == 0)
> +               latency = 1;
> +
> +       /* Bring kernel and HW constraints together */
> +       dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;
> 
>         if (!have_governor_per_policy())
>                 gov->gdbs_data = dbs_data; diff --git a/include/linux/cpufreq.h
> b/include/linux/cpufreq.h index 065f3a8..75d5bd9 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -533,7 +533,6 @@ int __cpufreq_driver_target(struct cpufreq_policy
> *policy,
>                                    unsigned int relation);  unsigned int
> cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
>                                          unsigned int target_freq); -unsigned int
> cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy);  int
> cpufreq_register_governor(struct cpufreq_governor *governor);  void
> cpufreq_unregister_governor(struct cpufreq_governor *governor);
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index 2f52ec0..a61d7fa 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -583,7 +583,16 @@ static int sugov_init(struct cpufreq_policy *policy)
>                 goto stop_kthread;
>         }
> 
> -       tunables->rate_limit_us = cpufreq_policy_transition_delay_us(policy);
> +       if (policy->transition_delay_us) {
> +               tunables->rate_limit_us = policy->transition_delay_us;
> +       } else {
> +               unsigned int lat;
> +
> +               tunables->rate_limit_us = LATENCY_MULTIPLIER;
> +               lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> +               if (lat)
> +                       tunables->rate_limit_us *= lat;
> +       }
> 
>         policy->governor_data = sg_policy;
>         sg_policy->tunables = tunables;
> --
> 2.7.4
> 

Re: Ask for help on governor

[Rafael J. Wysocki]

On Fri, Dec 15, 2017 at 10:00 AM, Andy Tang <andy.tang@nxp.com> wrote:
> Hi Doug,
>
>> -----Original Message-----
>> From: Doug Smythies [mailto:dsmythies@telus.net]
>> Sent: Friday, December 15, 2017 3:38 PM
>> To: Andy Tang <andy.tang@nxp.com>
>> Cc: 'Rafael J. Wysocki' <rafael@kernel.org>; 'Rafael J. Wysocki'
>> <rjw@rjwysocki.net>; 'Linux PM' <linux-pm@vger.kernel.org>; 'Viresh
>> Kumar' <viresh.kumar@linaro.org>; 'Stratos Karafotis'
>> <stratosk@semaphore.gr>
>> Subject: RE: Ask for help on governor
>>
>> Hi Andy,
>>
>> Could you please try the below reversion patch.
>> It is on top of kernel 4.15-rc3.
> Tried, no any effects.

Does this mean that the revert helps or that it doesn't help?

>>
>> On my system, and for the intel_cpufrequ scaling driver, the default
>> sampling_rate goes back to 20000 (which works) from 500 uSec (which
>> doesn't), for both the conservative and ondemand governors.
>
> If I set the sampling_rate to 20,000 (actually 10,000 is enough), cpufreq works too for both the conservative and ondemand governors.
> But on my platforms, the default rate is 1000 which leads to cpu load calculating issue.
>

OK, that's good to know.

I'll look deeper into this later today or tomorrow.

Thanks,
Rafael

Re: Ask for help on governor

[Rafael J. Wysocki]

On Friday, December 15, 2017 8:37:38 AM CET Doug Smythies wrote:
> Hi Andy,
> 
> Could you please try the below reversion patch.
> It is on top of kernel 4.15-rc3.
> 
> On my system, and for the intel_cpufrequ scaling driver,
> the default sampling_rate goes back to 20000 (which works)
> from 500 uSec (which doesn't), for both the conservative
> and ondemand governors.

OK

So AFAICS the problem is that dbs_update() makes assumptions that are not
me, quite obviously, if the sampling interval is less than the tick period.

For example, the "time_elapsed > 2 * sampling_rate" condition in it may
very well trigger every time in that case and that obviously affects the
conservative governor.

What about the patch below, then?

---
 drivers/cpufreq/cpufreq_governor.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -430,7 +430,14 @@ int cpufreq_dbs_governor_init(struct cpu
 	if (ret)
 		goto free_policy_dbs_info;
 
+	/*
+	 * The sampling interval should not be greater than the transition
+	 * latency of the CPU, but it also cannot be less than the tick period
+	 * for dbs_update() to work correctly.
+	 */
 	dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
+	if (dbs_data->sampling_rate < TICK_NSEC * NSEC_PER_USEC)
+		dbs_data->sampling_rate = TICK_NSEC * NSEC_PER_USEC;
 
 	if (!have_governor_per_policy())
 		gov->gdbs_data = dbs_data;

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.15 07:54 Rafael J. Wysocki wrote:
> On Friday, December 15, 2017 8:37:38 AM Doug Smythies wrote:
>
>> Could you please try the below reversion patch.
>> It is on top of kernel 4.15-rc3.
>> 
>> On my system, and for the intel_cpufrequ scaling driver,
>> the default sampling_rate goes back to 20000 (which works)
>> from 500 uSec (which doesn't), for both the conservative
>> and ondemand governors.
>
> OK
> 
> So AFAICS the problem is that dbs_update() makes assumptions that are not
> me, quite obviously, if the sampling interval is less than the tick period.
>
> For example, the "time_elapsed > 2 * sampling_rate" condition in it may
> very well trigger every time in that case and that obviously affects the
> conservative governor.

It does trigger every time and that is why the conservative gov. sticks at
low frequency.

> What about the patch below, then?

I am testing now, from previous work, and already reported on this thread,
I already know that the conservative governor will be O.K..
It will take me awhile to thoroughly test the ondemand governor. At only 1.0
ticks at best it will be quite noisy and more sensitive to work/sleep frequencies
for periodic workflows.

By the way, I modified your proposed patch, which needed to divide by
NSEC_PER_USEC instead of multiply by.
i.e. I am testing this:

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 58d4f4e..1c2fd1d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -430,7 +430,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
        if (ret)
                goto free_policy_dbs_info;

+       /*
+        * The sampling interval should not be greater than the transition
+        * latency of the CPU, but it also cannot be less than the tick period
+        * for dbs_update() to work correctly.
+        */
        dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
+       if (dbs_data->sampling_rate < TICK_NSEC / NSEC_PER_USEC)
+               dbs_data->sampling_rate = TICK_NSEC / NSEC_PER_USEC;

        if (!have_governor_per_policy())
                gov->gdbs_data = dbs_data;

... Doug

Re: Ask for help on governor

[Rafael J. Wysocki]

On Fri, Dec 15, 2017 at 7:27 PM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2017.12.15 07:54 Rafael J. Wysocki wrote:
>> On Friday, December 15, 2017 8:37:38 AM Doug Smythies wrote:
>>
>>> Could you please try the below reversion patch.
>>> It is on top of kernel 4.15-rc3.
>>>
>>> On my system, and for the intel_cpufrequ scaling driver,
>>> the default sampling_rate goes back to 20000 (which works)
>>> from 500 uSec (which doesn't), for both the conservative
>>> and ondemand governors.
>>
>> OK
>>
>> So AFAICS the problem is that dbs_update() makes assumptions that are not
>> me, quite obviously, if the sampling interval is less than the tick period.
>>
>> For example, the "time_elapsed > 2 * sampling_rate" condition in it may
>> very well trigger every time in that case and that obviously affects the
>> conservative governor.
>
> It does trigger every time and that is why the conservative gov. sticks at
> low frequency.
>
>> What about the patch below, then?
>
> I am testing now, from previous work, and already reported on this thread,
> I already know that the conservative governor will be O.K..
> It will take me awhile to thoroughly test the ondemand governor. At only 1.0
> ticks at best it will be quite noisy and more sensitive to work/sleep frequencies
> for periodic workflows.

OK

Please also check a variant with 2 * TICK_NSEC / NSEC_PER_USEC on the
right-hand side.  If that's substantially less noisy, we can use it
instead.

The response latency of the governor obviously depends on that, but
the factor of 2 shouldn't really matter that much in that respect.

> By the way, I modified your proposed patch, which needed to divide by
> NSEC_PER_USEC instead of multiply by.
> i.e. I am testing this:
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 58d4f4e..1c2fd1d 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -430,7 +430,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
>         if (ret)
>                 goto free_policy_dbs_info;
>
> +       /*
> +        * The sampling interval should not be greater than the transition
> +        * latency of the CPU, but it also cannot be less than the tick period
> +        * for dbs_update() to work correctly.
> +        */
>         dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
> +       if (dbs_data->sampling_rate < TICK_NSEC / NSEC_PER_USEC)
> +               dbs_data->sampling_rate = TICK_NSEC / NSEC_PER_USEC;
>
>         if (!have_governor_per_policy())
>                 gov->gdbs_data = dbs_data;
>

Yeah, that's what I really meant. :-)

Using the multiplication instead of the division was a mistake (again,
quite obviously).

Thanks,
Rafael

[PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
governors as well) the sampling_rate field of struct dbs_data may be
less than the tick period which causes dbs_update() to produce
incorrect results, so make the code ensure that the value of that
field will always be sufficiently large.

Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
Reported-by: Andy Tang <andy.tang@nxp.com>
Reported-by: Doug Smythies <dsmythies@telus.net>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_governor.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,8 @@
 
 #include "cpufreq_governor.h"
 
+#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(2 * TICK_NSEC / NSEC_PER_USEC)
+
 static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
 
 static DEFINE_MUTEX(gov_dbs_data_mutex);
@@ -47,11 +49,15 @@ ssize_t store_sampling_rate(struct gov_a
 {
 	struct dbs_data *dbs_data = to_dbs_data(attr_set);
 	struct policy_dbs_info *policy_dbs;
+	unsigned int sampling_interval;
 	int ret;
-	ret = sscanf(buf, "%u", &dbs_data->sampling_rate);
-	if (ret != 1)
+
+	ret = sscanf(buf, "%u", &sampling_interval);
+	if (ret != 1 || sampling_interval < CPUFREQ_DBS_MIN_SAMPLING_INTERVAL)
 		return -EINVAL;
 
+	dbs_data->sampling_rate = sampling_interval;
+
 	/*
 	 * We are operating under dbs_data->mutex and so the list and its
 	 * entries can't be freed concurrently.
@@ -430,7 +436,14 @@ int cpufreq_dbs_governor_init(struct cpu
 	if (ret)
 		goto free_policy_dbs_info;
 
-	dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
+	/*
+	 * The sampling interval should not be less than the transition latency
+	 * of the CPU and it also cannot be too small for dbs_update() to work
+	 * correctly.
+	 */
+	dbs_data->sampling_rate = max_t(unsigned int,
+					CPUFREQ_DBS_MIN_SAMPLING_INTERVAL,
+					cpufreq_policy_transition_delay_us(policy));
 
 	if (!have_governor_per_policy())
 		gov->gdbs_data = dbs_data;

RE: [PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Andy Tang]

Tested this patch on my platform, both conservative and ondemand governor work fine.
The default sampling_rate was updated from 1000 to 8000.

Thanks,
Andy

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, December 18, 2017 9:16 AM
> To: Doug Smythies <dsmythies@telus.net>; Andy Tang
> <andy.tang@nxp.com>; 'Linux PM' <linux-pm@vger.kernel.org>
> Cc: 'Viresh Kumar' <viresh.kumar@linaro.org>; 'Stratos Karafotis'
> <stratosk@semaphore.gr>
> Subject: [PATCH] cpufreq: governor: Ensure sufficiently large sampling
> intervals
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
> governors as well) the sampling_rate field of struct dbs_data may be less
> than the tick period which causes dbs_update() to produce incorrect results,
> so make the code ensure that the value of that field will always be
> sufficiently large.
> 
> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors
> as well)
> Reported-by: Andy Tang <andy.tang@nxp.com>
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ==========================================================
> =========
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -22,6 +22,8 @@
> 
>  #include "cpufreq_governor.h"
> 
> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(2 * TICK_NSEC /
> NSEC_PER_USEC)
> +
>  static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
> 
>  static DEFINE_MUTEX(gov_dbs_data_mutex); @@ -47,11 +49,15 @@ ssize_t
> store_sampling_rate(struct gov_a  {
>  	struct dbs_data *dbs_data = to_dbs_data(attr_set);
>  	struct policy_dbs_info *policy_dbs;
> +	unsigned int sampling_interval;
>  	int ret;
> -	ret = sscanf(buf, "%u", &dbs_data->sampling_rate);
> -	if (ret != 1)
> +
> +	ret = sscanf(buf, "%u", &sampling_interval);
> +	if (ret != 1 || sampling_interval <
> CPUFREQ_DBS_MIN_SAMPLING_INTERVAL)
>  		return -EINVAL;
> 
> +	dbs_data->sampling_rate = sampling_interval;
> +
>  	/*
>  	 * We are operating under dbs_data->mutex and so the list and its
>  	 * entries can't be freed concurrently.
> @@ -430,7 +436,14 @@ int cpufreq_dbs_governor_init(struct cpu
>  	if (ret)
>  		goto free_policy_dbs_info;
> 
> -	dbs_data->sampling_rate =
> cpufreq_policy_transition_delay_us(policy);
> +	/*
> +	 * The sampling interval should not be less than the transition latency
> +	 * of the CPU and it also cannot be too small for dbs_update() to
> work
> +	 * correctly.
> +	 */
> +	dbs_data->sampling_rate = max_t(unsigned int,
> +
> 	CPUFREQ_DBS_MIN_SAMPLING_INTERVAL,
> +
> 	cpufreq_policy_transition_delay_us(policy));
> 
>  	if (!have_governor_per_policy())
>  		gov->gdbs_data = dbs_data;

Re: [PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Viresh Kumar]

On 18-12-17, 02:15, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
> governors as well) the sampling_rate field of struct dbs_data may be
> less than the tick period which causes dbs_update() to produce
> incorrect results, so make the code ensure that the value of that
> field will always be sufficiently large.
> 
> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
> Reported-by: Andy Tang <andy.tang@nxp.com>
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

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

-- 
viresh

RE: [PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Doug Smythies]

On 2017.12.17 17:16 Rafael J. Wysocki wrote:

> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
> governors as well) the sampling_rate field of struct dbs_data may be
> less than the tick period which causes dbs_update() to produce
> incorrect results, so make the code ensure that the value of that
> field will always be sufficiently large.
>
> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
> Reported-by: Andy Tang <andy.tang@nxp.com>
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/cpufreq_governor.c |   19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -22,6 +22,8 @@
> 
> #include "cpufreq_governor.h"
> 
> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(2 * TICK_NSEC / NSEC_PER_USEC)
> +

Left over from the other thread on Friday I was testing both the above and
the 1 TICK version:

+#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(TICK_NSEC / NSEC_PER_USEC)

I tested periodic workflows at around 35% average load with work/sleep
frequencies from 100 to 2100 hertz, for both 250 Hertz kernels
(4 millisecond TICK) and 1000 Hertz kernels (1 millisecond TICK)

The 1 TICK version does have a "nosier" response than the 2 TICK version,
but both seem to work fine. Neither are worse than the schedutil response
for the same test.

I'd be O.K. with either the 1 TICK or 2 TICK versions.

... Doug

Re: [PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Rafael J. Wysocki]

On Mon, Dec 18, 2017 at 5:11 PM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2017.12.17 17:16 Rafael J. Wysocki wrote:
>
>> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
>> governors as well) the sampling_rate field of struct dbs_data may be
>> less than the tick period which causes dbs_update() to produce
>> incorrect results, so make the code ensure that the value of that
>> field will always be sufficiently large.
>>
>> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
>> Reported-by: Andy Tang <andy.tang@nxp.com>
>> Reported-by: Doug Smythies <dsmythies@telus.net>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>> drivers/cpufreq/cpufreq_governor.c |   19 ++++++++++++++++---
>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
>> @@ -22,6 +22,8 @@
>>
>> #include "cpufreq_governor.h"
>>
>> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL    (2 * TICK_NSEC / NSEC_PER_USEC)
>> +
>
> Left over from the other thread on Friday I was testing both the above and
> the 1 TICK version:
>
> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL      (TICK_NSEC / NSEC_PER_USEC)
>
> I tested periodic workflows at around 35% average load with work/sleep
> frequencies from 100 to 2100 hertz, for both 250 Hertz kernels
> (4 millisecond TICK) and 1000 Hertz kernels (1 millisecond TICK)
>
> The 1 TICK version does have a "nosier" response than the 2 TICK version,
> but both seem to work fine. Neither are worse than the schedutil response
> for the same test.
>
> I'd be O.K. with either the 1 TICK or 2 TICK versions.

OK, thanks!

I prefer "less noisy", so I'll take the 2 TICK one (originally in this patch).

It can still be changed to 1 TICK later if there are complaints (even
though I'm not really expecting any).

Thanks,
Rafael

RE: Ask for help on governor

[Doug Smythies]

On 2017.12.12 22:18 Viresh Kumar wrote:
> On 12-12-17, 19:10, Doug Smythies wrote:

>> Thanks for the suggestion. It was so very close, the problem commit is the very
>> next one, aa7519af450d.
>> 
>> Bisect result:
>> 
>> aa7519af450d3c62a057aece24877c34562fa25a is the first bad commit
>> commit aa7519af450d3c62a057aece24877c34562fa25a
>> Author: Viresh Kumar <viresh.kumar@linaro.org>
>> Date:   Wed Jul 19 15:42:42 2017 +0530
>> 
>>     cpufreq: Use transition_delay_us for legacy governors as well
>> 
>>     The policy->transition_delay_us field is used only by the schedutil
>>     governor currently, and this field describes how fast the driver wants
>>     the cpufreq governor to change CPUs frequency. It should rather be a
>>     common thing across all governors, as it doesn't have any schedutil
>>     dependency here.
>> 
>>     Create a new helper cpufreq_policy_transition_delay_us() to get the
>>     transition delay across all governors.
>> 
>>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> What has changed for intel-pstate after this patch ?
>
> sampling_rate for conservative and ondemand is 500 us now, which was 20000 us
> earlier. So we reevaluate load very frequently now, but in case load isn't over
> 80% then we wouldn't increase the frequency.
>
> @Doug/Andy: Can you please try following:
>
> - Checkout 4.14 or mainline (the broken kernels).
> - Set governor to conservative.
> - Monitor /sys/devices/system/cpu/cpufreq/policy*/stats/total_trans (to check if
>  frequency is getting changed or not).

I do not have a stats directory. I am now using kernel 4.15-rc3

> - Run some dummy load, I did this:
>
>  perf bench sched messaging --pipe --thread --group 8
>
> This should result in frequency updates, so its not that we aren't changing
> frequency at all now with conservative.
>
> Then do this:
>
> echo 20000 > /sys/devices/system/cpu/cpufreq/conservative/sampling_rate
>
> and this should take you back to the original behavior which was there pre-4.14.

Yes, that works.
The breakpoint is <2000 (on my system, at least). 1999 doesn't work, but one can observe that it is trying.
2000 works, but seems to struggle a little.

... Doug

RE: Ask for help on governor

[Doug Smythies]

Hi Viresh,

In addition to what I wrote earlier:

I can also make the conservative governor work under the
intel_cpufreq scaling driver, with the default 500 uSec
sampling time, by increasing the
/sys/devices/system/cpu/cpufreq/conservative/freq_step
value from the default of 5 to 100.

In this case there is not a specific break point freq_step,
but rather a very gradual increase in CPU freq as a function
of freq_step for a 100% load on a CPU.

Now, I wonder if the two issues here might be related after all.

... Doug

[PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Viresh Kumar]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

[ Upstream commit 56026645e2b6f11ede34a5e6ab69d3eb56f9c8fc ]

After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
governors as well) the sampling_rate field of struct dbs_data may be
less than the tick period which causes dbs_update() to produce
incorrect results, so make the code ensure that the value of that
field will always be sufficiently large.

Cc: 4.14 <stable@vger.kernel.org> # 4.14
Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
Reported-by: Andy Tang <andy.tang@nxp.com>
Reported-by: Doug Smythies <dsmythies@telus.net>
Tested-by: Andy Tang <andy.tang@nxp.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 58d4f4e1ad6a..ca38229b045a 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,8 @@
 
 #include "cpufreq_governor.h"
 
+#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(2 * TICK_NSEC / NSEC_PER_USEC)
+
 static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
 
 static DEFINE_MUTEX(gov_dbs_data_mutex);
@@ -47,11 +49,15 @@ ssize_t store_sampling_rate(struct gov_attr_set *attr_set, const char *buf,
 {
 	struct dbs_data *dbs_data = to_dbs_data(attr_set);
 	struct policy_dbs_info *policy_dbs;
+	unsigned int sampling_interval;
 	int ret;
-	ret = sscanf(buf, "%u", &dbs_data->sampling_rate);
-	if (ret != 1)
+
+	ret = sscanf(buf, "%u", &sampling_interval);
+	if (ret != 1 || sampling_interval < CPUFREQ_DBS_MIN_SAMPLING_INTERVAL)
 		return -EINVAL;
 
+	dbs_data->sampling_rate = sampling_interval;
+
 	/*
 	 * We are operating under dbs_data->mutex and so the list and its
 	 * entries can't be freed concurrently.
@@ -430,7 +436,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_policy_dbs_info;
 
-	dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
+	/*
+	 * The sampling interval should not be less than the transition latency
+	 * of the CPU and it also cannot be too small for dbs_update() to work
+	 * correctly.
+	 */
+	dbs_data->sampling_rate = max_t(unsigned int,
+					CPUFREQ_DBS_MIN_SAMPLING_INTERVAL,
+					cpufreq_policy_transition_delay_us(policy));
 
 	if (!have_governor_per_policy())
 		gov->gdbs_data = dbs_data;
-- 
2.15.0.194.g9af6a3dea062

Re: [PATCH] cpufreq: governor: Ensure sufficiently large sampling intervals

[Rafael J. Wysocki]

On Monday, January 29, 2018 8:21:23 AM CET Viresh Kumar wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> [ Upstream commit 56026645e2b6f11ede34a5e6ab69d3eb56f9c8fc ]
> 
> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy
> governors as well) the sampling_rate field of struct dbs_data may be
> less than the tick period which causes dbs_update() to produce
> incorrect results, so make the code ensure that the value of that
> field will always be sufficiently large.
> 
> Cc: 4.14 <stable@vger.kernel.org> # 4.14
> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well)
> Reported-by: Andy Tang <andy.tang@nxp.com>
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Tested-by: Andy Tang <andy.tang@nxp.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_governor.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 58d4f4e1ad6a..ca38229b045a 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -22,6 +22,8 @@
>  
>  #include "cpufreq_governor.h"
>  
> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL	(2 * TICK_NSEC / NSEC_PER_USEC)
> +
>  static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
>  
>  static DEFINE_MUTEX(gov_dbs_data_mutex);
> @@ -47,11 +49,15 @@ ssize_t store_sampling_rate(struct gov_attr_set *attr_set, const char *buf,
>  {
>  	struct dbs_data *dbs_data = to_dbs_data(attr_set);
>  	struct policy_dbs_info *policy_dbs;
> +	unsigned int sampling_interval;
>  	int ret;
> -	ret = sscanf(buf, "%u", &dbs_data->sampling_rate);
> -	if (ret != 1)
> +
> +	ret = sscanf(buf, "%u", &sampling_interval);
> +	if (ret != 1 || sampling_interval < CPUFREQ_DBS_MIN_SAMPLING_INTERVAL)
>  		return -EINVAL;
>  
> +	dbs_data->sampling_rate = sampling_interval;
> +
>  	/*
>  	 * We are operating under dbs_data->mutex and so the list and its
>  	 * entries can't be freed concurrently.
> @@ -430,7 +436,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
>  	if (ret)
>  		goto free_policy_dbs_info;
>  
> -	dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
> +	/*
> +	 * The sampling interval should not be less than the transition latency
> +	 * of the CPU and it also cannot be too small for dbs_update() to work
> +	 * correctly.
> +	 */
> +	dbs_data->sampling_rate = max_t(unsigned int,
> +					CPUFREQ_DBS_MIN_SAMPLING_INTERVAL,
> +					cpufreq_policy_transition_delay_us(policy));
>  
>  	if (!have_governor_per_policy())
>  		gov->gdbs_data = dbs_data;
> 

Right, thanks for pushing this to -stable!