2016-11-06 09:19:34

by Stratos Karafotis

[permalink] [raw]
Subject: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

Conservative governor changes the CPU frequency in steps.
That means that if a CPU runs at max frequency, it will need several
sampling periods to return at min frequency when the workload
is finished.

If the timer that calculates the load and target frequency is deferred,
the governor might need even more time to decrease the frequency.

This may have impact to power consumption and after all conservative
should decrease the frequency if there is no workload every sampling
rate.

To resolve the above issue calculate the number of sampling periods
that the timer deferred. Considering that for each sampling period
conservative should drop the frequency by a freq_step because the
CPU was idle apply the proper subtraction to requested frequency.

Below, the kernel trace with and without this patch. First an
intensive workload is applied on a specific CPU. Then the workload
is removed and the CPU goes to idle.

WITHOUT
-------
<idle>-0 [007] dN.. 620.329153: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 620.350857: cpu_frequency: state=1700000 cpu_id=7
kworker/7:2-556 [007] .... 620.370856: cpu_frequency: state=1900000 cpu_id=7
kworker/7:2-556 [007] .... 620.390854: cpu_frequency: state=2100000 cpu_id=7
kworker/7:2-556 [007] .... 620.411853: cpu_frequency: state=2200000 cpu_id=7
kworker/7:2-556 [007] .... 620.432854: cpu_frequency: state=2400000 cpu_id=7
kworker/7:2-556 [007] .... 620.453854: cpu_frequency: state=2600000 cpu_id=7
kworker/7:2-556 [007] .... 620.494856: cpu_frequency: state=2900000 cpu_id=7
kworker/7:2-556 [007] .... 620.515856: cpu_frequency: state=3100000 cpu_id=7
kworker/7:2-556 [007] .... 620.536858: cpu_frequency: state=3300000 cpu_id=7
kworker/7:2-556 [007] .... 620.557857: cpu_frequency: state=3401000 cpu_id=7
<idle>-0 [007] d... 669.591363: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 669.591939: cpu_idle: state=4294967295 cpu_id=7
<idle>-0 [007] d... 669.591980: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] dN.. 669.591989: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 670.201224: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 670.221975: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 670.222016: cpu_frequency: state=3300000 cpu_id=7
<idle>-0 [007] d... 670.222026: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 670.234964: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 670.801251: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.236046: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 671.236073: cpu_frequency: state=3100000 cpu_id=7
<idle>-0 [007] d... 671.236112: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.393437: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 671.401277: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.404083: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 671.404111: cpu_frequency: state=2900000 cpu_id=7
<idle>-0 [007] d... 671.404125: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.404974: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 671.501180: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.995414: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 671.995459: cpu_frequency: state=2800000 cpu_id=7
<idle>-0 [007] d... 671.995469: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 671.996287: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 672.001305: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.078374: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 672.078410: cpu_frequency: state=2600000 cpu_id=7
<idle>-0 [007] d... 672.078419: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.158020: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 672.158040: cpu_frequency: state=2400000 cpu_id=7
<idle>-0 [007] d... 672.158044: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.160038: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 672.234557: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.237121: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 672.237174: cpu_frequency: state=2100000 cpu_id=7
<idle>-0 [007] d... 672.237186: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.237778: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 672.267902: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.269860: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 672.269906: cpu_frequency: state=1900000 cpu_id=7
<idle>-0 [007] d... 672.269914: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.271902: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 672.751342: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 672.823056: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-556 [007] .... 672.823095: cpu_frequency: state=1600000 cpu_id=7

WITH
----
<idle>-0 [007] dN.. 4380.928009: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-399 [007] .... 4380.949767: cpu_frequency: state=2000000 cpu_id=7
kworker/7:2-399 [007] .... 4380.969765: cpu_frequency: state=2200000 cpu_id=7
kworker/7:2-399 [007] .... 4381.009766: cpu_frequency: state=2500000 cpu_id=7
kworker/7:2-399 [007] .... 4381.029767: cpu_frequency: state=2600000 cpu_id=7
kworker/7:2-399 [007] .... 4381.049769: cpu_frequency: state=2800000 cpu_id=7
kworker/7:2-399 [007] .... 4381.069769: cpu_frequency: state=3000000 cpu_id=7
kworker/7:2-399 [007] .... 4381.089771: cpu_frequency: state=3100000 cpu_id=7
kworker/7:2-399 [007] .... 4381.109772: cpu_frequency: state=3400000 cpu_id=7
kworker/7:2-399 [007] .... 4381.129773: cpu_frequency: state=3401000 cpu_id=7
<idle>-0 [007] d... 4428.226159: cpu_idle: state=1 cpu_id=7
<idle>-0 [007] d... 4428.226176: cpu_idle: state=4294967295 cpu_id=7
<idle>-0 [007] d... 4428.226181: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4428.227177: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 4428.551640: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4428.649239: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-399 [007] .... 4428.649268: cpu_frequency: state=2800000 cpu_id=7
<idle>-0 [007] d... 4428.649278: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4428.689856: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 4428.799542: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4428.801683: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-399 [007] .... 4428.801748: cpu_frequency: state=1700000 cpu_id=7
<idle>-0 [007] d... 4428.801761: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4428.806545: cpu_idle: state=4294967295 cpu_id=7
...
<idle>-0 [007] d... 4429.051880: cpu_idle: state=4 cpu_id=7
<idle>-0 [007] d... 4429.086240: cpu_idle: state=4294967295 cpu_id=7
kworker/7:2-399 [007] .... 4429.086293: cpu_frequency: state=1600000 cpu_id=7

Without the patch the CPU dropped to min frequency after 3.2s
With the patch applied the CPU dropped to min frequency after 0.86s

Signed-off-by: Stratos Karafotis <[email protected]>
---
drivers/cpufreq/cpufreq_conservative.c | 9 +++++++++
drivers/cpufreq/cpufreq_governor.c | 18 +++++++++++++-----
drivers/cpufreq/cpufreq_governor.h | 1 +
3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 1347589..07dac72 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -74,6 +74,15 @@ static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
if (cs_tuners->freq_step == 0)
goto out;

+ if (policy_dbs->deferred_periods < UINT_MAX) {
+ unsigned int freq_target = policy_dbs->deferred_periods *
+ get_freq_target(cs_tuners, policy);
+ if (requested_freq > freq_target)
+ requested_freq -= freq_target;
+ else
+ requested_freq = policy->min;
+ policy_dbs->deferred_periods = UINT_MAX;
+ }
/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 642dd0f..0d498a0 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -117,7 +117,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
struct policy_dbs_info *policy_dbs = policy->governor_data;
struct dbs_data *dbs_data = policy_dbs->dbs_data;
unsigned int ignore_nice = dbs_data->ignore_nice_load;
- unsigned int max_load = 0;
+ unsigned int max_load = 0, deferred_periods = UINT_MAX;
unsigned int sampling_rate, io_busy, j;

/*
@@ -163,8 +163,13 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
* calls, so the previous load value can be used then.
*/
load = j_cdbs->prev_load;
- } else if (unlikely(time_elapsed > 2 * sampling_rate &&
- j_cdbs->prev_load)) {
+ } else if (unlikely(time_elapsed > 2 * sampling_rate)) {
+ unsigned int periods = time_elapsed / sampling_rate;
+
+ if (periods < deferred_periods)
+ deferred_periods = periods;
+
+ if (j_cdbs->prev_load) {
/*
* If the CPU had gone completely idle and a task has
* just woken up on this CPU now, it would be unfair to
@@ -189,8 +194,9 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
* 'time_elapsed' (as compared to the sampling rate)
* indicates this scenario.
*/
- load = j_cdbs->prev_load;
- j_cdbs->prev_load = 0;
+ load = j_cdbs->prev_load;
+ j_cdbs->prev_load = 0;
+ }
} else {
if (time_elapsed >= idle_time) {
load = 100 * (time_elapsed - idle_time) / time_elapsed;
@@ -218,6 +224,8 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
if (load > max_load)
max_load = load;
}
+ policy_dbs->deferred_periods = deferred_periods;
+
return max_load;
}
EXPORT_SYMBOL_GPL(dbs_update);
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index ef1037e..48efeb5 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -97,6 +97,7 @@ struct policy_dbs_info {
struct list_head list;
/* Multiplier for increasing sample delay temporarily. */
unsigned int rate_mult;
+ unsigned int deferred_periods;
/* Status indicators */
bool is_shared; /* This object is used by multiple CPUs */
bool work_in_progress; /* Work is being queued up or in progress */
--
2.7.4


2016-11-07 06:12:40

by Viresh Kumar

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

For the record, I have never got the original mail with this subject.

On 06-11-16, 11:19, Stratos Karafotis wrote:
> Conservative governor changes the CPU frequency in steps.
> That means that if a CPU runs at max frequency, it will need several
> sampling periods to return at min frequency when the workload
> is finished.
>
> If the timer that calculates the load and target frequency is deferred,
> the governor might need even more time to decrease the frequency.
>
> This may have impact to power consumption and after all conservative
> should decrease the frequency if there is no workload every sampling
> rate.
>
> To resolve the above issue calculate the number of sampling periods
> that the timer deferred. Considering that for each sampling period
> conservative should drop the frequency by a freq_step because the
> CPU was idle apply the proper subtraction to requested frequency.
>
> Below, the kernel trace with and without this patch. First an
> intensive workload is applied on a specific CPU. Then the workload
> is removed and the CPU goes to idle.
>
> WITHOUT
> -------
> <idle>-0 [007] dN.. 620.329153: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 620.350857: cpu_frequency: state=1700000 cpu_id=7
> kworker/7:2-556 [007] .... 620.370856: cpu_frequency: state=1900000 cpu_id=7
> kworker/7:2-556 [007] .... 620.390854: cpu_frequency: state=2100000 cpu_id=7
> kworker/7:2-556 [007] .... 620.411853: cpu_frequency: state=2200000 cpu_id=7
> kworker/7:2-556 [007] .... 620.432854: cpu_frequency: state=2400000 cpu_id=7
> kworker/7:2-556 [007] .... 620.453854: cpu_frequency: state=2600000 cpu_id=7
> kworker/7:2-556 [007] .... 620.494856: cpu_frequency: state=2900000 cpu_id=7
> kworker/7:2-556 [007] .... 620.515856: cpu_frequency: state=3100000 cpu_id=7
> kworker/7:2-556 [007] .... 620.536858: cpu_frequency: state=3300000 cpu_id=7
> kworker/7:2-556 [007] .... 620.557857: cpu_frequency: state=3401000 cpu_id=7
> <idle>-0 [007] d... 669.591363: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 669.591939: cpu_idle: state=4294967295 cpu_id=7
> <idle>-0 [007] d... 669.591980: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] dN.. 669.591989: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 670.201224: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 670.221975: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 670.222016: cpu_frequency: state=3300000 cpu_id=7
> <idle>-0 [007] d... 670.222026: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 670.234964: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 670.801251: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.236046: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 671.236073: cpu_frequency: state=3100000 cpu_id=7
> <idle>-0 [007] d... 671.236112: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.393437: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 671.401277: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.404083: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 671.404111: cpu_frequency: state=2900000 cpu_id=7
> <idle>-0 [007] d... 671.404125: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.404974: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 671.501180: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.995414: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 671.995459: cpu_frequency: state=2800000 cpu_id=7
> <idle>-0 [007] d... 671.995469: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 671.996287: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 672.001305: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.078374: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 672.078410: cpu_frequency: state=2600000 cpu_id=7
> <idle>-0 [007] d... 672.078419: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.158020: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 672.158040: cpu_frequency: state=2400000 cpu_id=7
> <idle>-0 [007] d... 672.158044: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.160038: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 672.234557: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.237121: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 672.237174: cpu_frequency: state=2100000 cpu_id=7
> <idle>-0 [007] d... 672.237186: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.237778: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 672.267902: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.269860: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 672.269906: cpu_frequency: state=1900000 cpu_id=7
> <idle>-0 [007] d... 672.269914: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.271902: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 672.751342: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 672.823056: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-556 [007] .... 672.823095: cpu_frequency: state=1600000 cpu_id=7
>
> WITH
> ----
> <idle>-0 [007] dN.. 4380.928009: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-399 [007] .... 4380.949767: cpu_frequency: state=2000000 cpu_id=7
> kworker/7:2-399 [007] .... 4380.969765: cpu_frequency: state=2200000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.009766: cpu_frequency: state=2500000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.029767: cpu_frequency: state=2600000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.049769: cpu_frequency: state=2800000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.069769: cpu_frequency: state=3000000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.089771: cpu_frequency: state=3100000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.109772: cpu_frequency: state=3400000 cpu_id=7
> kworker/7:2-399 [007] .... 4381.129773: cpu_frequency: state=3401000 cpu_id=7
> <idle>-0 [007] d... 4428.226159: cpu_idle: state=1 cpu_id=7
> <idle>-0 [007] d... 4428.226176: cpu_idle: state=4294967295 cpu_id=7
> <idle>-0 [007] d... 4428.226181: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4428.227177: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 4428.551640: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4428.649239: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-399 [007] .... 4428.649268: cpu_frequency: state=2800000 cpu_id=7
> <idle>-0 [007] d... 4428.649278: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4428.689856: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 4428.799542: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4428.801683: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-399 [007] .... 4428.801748: cpu_frequency: state=1700000 cpu_id=7
> <idle>-0 [007] d... 4428.801761: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4428.806545: cpu_idle: state=4294967295 cpu_id=7
> ...
> <idle>-0 [007] d... 4429.051880: cpu_idle: state=4 cpu_id=7
> <idle>-0 [007] d... 4429.086240: cpu_idle: state=4294967295 cpu_id=7
> kworker/7:2-399 [007] .... 4429.086293: cpu_frequency: state=1600000 cpu_id=7
>
> Without the patch the CPU dropped to min frequency after 3.2s
> With the patch applied the CPU dropped to min frequency after 0.86s
>
> Signed-off-by: Stratos Karafotis <[email protected]>
> ---
> drivers/cpufreq/cpufreq_conservative.c | 9 +++++++++
> drivers/cpufreq/cpufreq_governor.c | 18 +++++++++++++-----
> drivers/cpufreq/cpufreq_governor.h | 1 +
> 3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 1347589..07dac72 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -74,6 +74,15 @@ static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
> if (cs_tuners->freq_step == 0)
> goto out;
>
> + if (policy_dbs->deferred_periods < UINT_MAX) {

Perhaps this all should be done only if we are going to decrease the frequency,
i.e. somewhere down than what you are proposing.

> + unsigned int freq_target = policy_dbs->deferred_periods *
> + get_freq_target(cs_tuners, policy);
> + if (requested_freq > freq_target)
> + requested_freq -= freq_target;
> + else
> + requested_freq = policy->min;
> + policy_dbs->deferred_periods = UINT_MAX;
> + }
> /*
> * If requested_freq is out of range, it is likely that the limits
> * changed in the meantime, so fall back to current frequency in that
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 642dd0f..0d498a0 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -117,7 +117,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> struct policy_dbs_info *policy_dbs = policy->governor_data;
> struct dbs_data *dbs_data = policy_dbs->dbs_data;
> unsigned int ignore_nice = dbs_data->ignore_nice_load;
> - unsigned int max_load = 0;
> + unsigned int max_load = 0, deferred_periods = UINT_MAX;
> unsigned int sampling_rate, io_busy, j;
>
> /*
> @@ -163,8 +163,13 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> * calls, so the previous load value can be used then.
> */
> load = j_cdbs->prev_load;
> - } else if (unlikely(time_elapsed > 2 * sampling_rate &&
> - j_cdbs->prev_load)) {
> + } else if (unlikely(time_elapsed > 2 * sampling_rate)) {
> + unsigned int periods = time_elapsed / sampling_rate;
> +
> + if (periods < deferred_periods)
> + deferred_periods = periods;
> +
> + if (j_cdbs->prev_load) {
> /*

You forgot to shift the below comment by a tab. Maybe just position the above
'if' statement after the comment.

> * If the CPU had gone completely idle and a task has
> * just woken up on this CPU now, it would be unfair to
> @@ -189,8 +194,9 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> * 'time_elapsed' (as compared to the sampling rate)
> * indicates this scenario.
> */
> - load = j_cdbs->prev_load;
> - j_cdbs->prev_load = 0;
> + load = j_cdbs->prev_load;
> + j_cdbs->prev_load = 0;
> + }
> } else {
> if (time_elapsed >= idle_time) {
> load = 100 * (time_elapsed - idle_time) / time_elapsed;
> @@ -218,6 +224,8 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> if (load > max_load)
> max_load = load;
> }
> + policy_dbs->deferred_periods = deferred_periods;
> +
> return max_load;
> }
> EXPORT_SYMBOL_GPL(dbs_update);
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ef1037e..48efeb5 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -97,6 +97,7 @@ struct policy_dbs_info {
> struct list_head list;
> /* Multiplier for increasing sample delay temporarily. */
> unsigned int rate_mult;
> + unsigned int deferred_periods;
> /* Status indicators */
> bool is_shared; /* This object is used by multiple CPUs */
> bool work_in_progress; /* Work is being queued up or in progress */
> --
> 2.7.4

--
viresh

2016-11-07 17:28:06

by Stratos Karafotis

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

Hi,

Thanks for reviewing.


On 07/11/2016 08:12 πμ, Viresh Kumar wrote:
> For the record, I have never got the original mail with this subject.

I'm sorry for inconvenience. It seems that I had an issue on my mail
server.

> On 06-11-16, 11:19, Stratos Karafotis wrote:
>> Conservative governor changes the CPU frequency in steps.
>> That means that if a CPU runs at max frequency, it will need several
>> sampling periods to return at min frequency when the workload
>> is finished.
>>
>> If the timer that calculates the load and target frequency is deferred,
>> the governor might need even more time to decrease the frequency.
>>
>> This may have impact to power consumption and after all conservative
>> should decrease the frequency if there is no workload every sampling
>> rate.
>>
>> To resolve the above issue calculate the number of sampling periods
>> that the timer deferred. Considering that for each sampling period
>> conservative should drop the frequency by a freq_step because the
>> CPU was idle apply the proper subtraction to requested frequency.
>>
>> Below, the kernel trace with and without this patch. First an
>> intensive workload is applied on a specific CPU. Then the workload
>> is removed and the CPU goes to idle.
>>
>> WITHOUT
>> -------
>> <idle>-0 [007] dN.. 620.329153: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 620.350857: cpu_frequency: state=1700000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.370856: cpu_frequency: state=1900000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.390854: cpu_frequency: state=2100000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.411853: cpu_frequency: state=2200000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.432854: cpu_frequency: state=2400000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.453854: cpu_frequency: state=2600000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.494856: cpu_frequency: state=2900000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.515856: cpu_frequency: state=3100000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.536858: cpu_frequency: state=3300000 cpu_id=7
>> kworker/7:2-556 [007] .... 620.557857: cpu_frequency: state=3401000 cpu_id=7
>> <idle>-0 [007] d... 669.591363: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 669.591939: cpu_idle: state=4294967295 cpu_id=7
>> <idle>-0 [007] d... 669.591980: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] dN.. 669.591989: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 670.201224: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 670.221975: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 670.222016: cpu_frequency: state=3300000 cpu_id=7
>> <idle>-0 [007] d... 670.222026: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 670.234964: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 670.801251: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.236046: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 671.236073: cpu_frequency: state=3100000 cpu_id=7
>> <idle>-0 [007] d... 671.236112: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.393437: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 671.401277: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.404083: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 671.404111: cpu_frequency: state=2900000 cpu_id=7
>> <idle>-0 [007] d... 671.404125: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.404974: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 671.501180: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.995414: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 671.995459: cpu_frequency: state=2800000 cpu_id=7
>> <idle>-0 [007] d... 671.995469: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 671.996287: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 672.001305: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.078374: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 672.078410: cpu_frequency: state=2600000 cpu_id=7
>> <idle>-0 [007] d... 672.078419: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.158020: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 672.158040: cpu_frequency: state=2400000 cpu_id=7
>> <idle>-0 [007] d... 672.158044: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.160038: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 672.234557: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.237121: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 672.237174: cpu_frequency: state=2100000 cpu_id=7
>> <idle>-0 [007] d... 672.237186: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.237778: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 672.267902: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.269860: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 672.269906: cpu_frequency: state=1900000 cpu_id=7
>> <idle>-0 [007] d... 672.269914: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.271902: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 672.751342: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 672.823056: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-556 [007] .... 672.823095: cpu_frequency: state=1600000 cpu_id=7
>>
>> WITH
>> ----
>> <idle>-0 [007] dN.. 4380.928009: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-399 [007] .... 4380.949767: cpu_frequency: state=2000000 cpu_id=7
>> kworker/7:2-399 [007] .... 4380.969765: cpu_frequency: state=2200000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.009766: cpu_frequency: state=2500000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.029767: cpu_frequency: state=2600000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.049769: cpu_frequency: state=2800000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.069769: cpu_frequency: state=3000000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.089771: cpu_frequency: state=3100000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.109772: cpu_frequency: state=3400000 cpu_id=7
>> kworker/7:2-399 [007] .... 4381.129773: cpu_frequency: state=3401000 cpu_id=7
>> <idle>-0 [007] d... 4428.226159: cpu_idle: state=1 cpu_id=7
>> <idle>-0 [007] d... 4428.226176: cpu_idle: state=4294967295 cpu_id=7
>> <idle>-0 [007] d... 4428.226181: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4428.227177: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 4428.551640: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4428.649239: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-399 [007] .... 4428.649268: cpu_frequency: state=2800000 cpu_id=7
>> <idle>-0 [007] d... 4428.649278: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4428.689856: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 4428.799542: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4428.801683: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-399 [007] .... 4428.801748: cpu_frequency: state=1700000 cpu_id=7
>> <idle>-0 [007] d... 4428.801761: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4428.806545: cpu_idle: state=4294967295 cpu_id=7
>> ...
>> <idle>-0 [007] d... 4429.051880: cpu_idle: state=4 cpu_id=7
>> <idle>-0 [007] d... 4429.086240: cpu_idle: state=4294967295 cpu_id=7
>> kworker/7:2-399 [007] .... 4429.086293: cpu_frequency: state=1600000 cpu_id=7
>>
>> Without the patch the CPU dropped to min frequency after 3.2s
>> With the patch applied the CPU dropped to min frequency after 0.86s
>>
>> Signed-off-by: Stratos Karafotis <[email protected]>
>> ---
>> drivers/cpufreq/cpufreq_conservative.c | 9 +++++++++
>> drivers/cpufreq/cpufreq_governor.c | 18 +++++++++++++-----
>> drivers/cpufreq/cpufreq_governor.h | 1 +
>> 3 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
>> index 1347589..07dac72 100644
>> --- a/drivers/cpufreq/cpufreq_conservative.c
>> +++ b/drivers/cpufreq/cpufreq_conservative.c
>> @@ -74,6 +74,15 @@ static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
>> if (cs_tuners->freq_step == 0)
>> goto out;
>>
>> + if (policy_dbs->deferred_periods < UINT_MAX) {
>
> Perhaps this all should be done only if we are going to decrease the frequency,
> i.e. somewhere down than what you are proposing.

Yes, it could be done only when we decrease frequency. But I thought that maybe
this is against conservative governor principle.

I initially observed this issue on a Snapdragon 808 using conservative on the
big cluster (A57). The CPU seemed to remain in high frequencies for
long time (even 10 seconds) before it returns to min.

So, most probably the load after the deferred period is completely unrelated to
the previous one. If we apply this heuristic only when the frequency will be
decreased (and having in mind that we copy the load value from the previous
period), IMHO I'm afraid that the conservative will be still more aggressive even
from ondemand governor.

>> + unsigned int freq_target = policy_dbs->deferred_periods *
>> + get_freq_target(cs_tuners, policy);
>> + if (requested_freq > freq_target)
>> + requested_freq -= freq_target;
>> + else
>> + requested_freq = policy->min;
>> + policy_dbs->deferred_periods = UINT_MAX;
>> + }
>> /*
>> * If requested_freq is out of range, it is likely that the limits
>> * changed in the meantime, so fall back to current frequency in that
>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>> index 642dd0f..0d498a0 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -117,7 +117,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
>> struct policy_dbs_info *policy_dbs = policy->governor_data;
>> struct dbs_data *dbs_data = policy_dbs->dbs_data;
>> unsigned int ignore_nice = dbs_data->ignore_nice_load;
>> - unsigned int max_load = 0;
>> + unsigned int max_load = 0, deferred_periods = UINT_MAX;
>> unsigned int sampling_rate, io_busy, j;
>>
>> /*
>> @@ -163,8 +163,13 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
>> * calls, so the previous load value can be used then.
>> */
>> load = j_cdbs->prev_load;
>> - } else if (unlikely(time_elapsed > 2 * sampling_rate &&
>> - j_cdbs->prev_load)) {
>> + } else if (unlikely(time_elapsed > 2 * sampling_rate)) {
>> + unsigned int periods = time_elapsed / sampling_rate;
>> +
>> + if (periods < deferred_periods)
>> + deferred_periods = periods;
>> +
>> + if (j_cdbs->prev_load) {
>> /*
>
> You forgot to shift the below comment by a tab. Maybe just position the above
> 'if' statement after the comment.

OK, I will place the 'if' statement after the comment, because the shifting
will cause the block to exceed the 80 columns limit.


Regards,
Stratos

2016-11-08 04:04:35

by Viresh Kumar

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

On 07-11-16, 19:27, Stratos Karafotis wrote:
> Yes, it could be done only when we decrease frequency. But I thought that maybe
> this is against conservative governor principle.
>
> I initially observed this issue on a Snapdragon 808 using conservative on the
> big cluster (A57). The CPU seemed to remain in high frequencies for
> long time (even 10 seconds) before it returns to min.
>
> So, most probably the load after the deferred period is completely unrelated to
> the previous one. If we apply this heuristic only when the frequency will be
> decreased (and having in mind that we copy the load value from the previous
> period), IMHO I'm afraid that the conservative will be still more aggressive even
> from ondemand governor.

The deferred period here is actually the time for which the CPU was idle and not
doing anything.

And I am not sure why we should be worrying about increasing the frequency steps
for the period for which the CPU was idle.

--
viresh

2016-11-08 08:32:32

by Viresh Kumar

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

On 8 November 2016 at 12:49, Stratos Karafotis <[email protected]> wrote:
> I think we shouldn't. That's why the patch first decreases the frequency
> by n freq steps (where n the number of deferred periods).
> Then the normal processing takes place.

The problem that I see is that the new algorithm will reduce the
frequency even if we are
on a ramp up phase.

For example consider this case:

- We have a special load running, that runs in bursts. i.e. runs for
some time, lets the CPU idle
then and then again runs.

- To run the load properly, we need to ramp up the frequency

- But the new algorithm can make the frequency stagnant in this case.
i.e. because of the idle
period you may want to decrease the frequency by delta A and then the
regular algorithm may
want to increase it by same delta A.

That's why I was asking to adopt this only in the ramp down path.

--
viresh

2016-11-08 19:25:23

by Stratos Karafotis

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

On 08/11/2016 10:32 πμ, Viresh Kumar wrote:
> On 8 November 2016 at 12:49, Stratos Karafotis <[email protected]> wrote:
>> I think we shouldn't. That's why the patch first decreases the frequency
>> by n freq steps (where n the number of deferred periods).
>> Then the normal processing takes place.
>
> The problem that I see is that the new algorithm will reduce the
> frequency even if we are
> on a ramp up phase.
>
> For example consider this case:
>
> - We have a special load running, that runs in bursts. i.e. runs for
> some time, lets the CPU idle
> then and then again runs.
>
> - To run the load properly, we need to ramp up the frequency
>
> - But the new algorithm can make the frequency stagnant in this case.
> i.e. because of the idle
> period you may want to decrease the frequency by delta A and then the
> regular algorithm may
> want to increase it by same delta A.
>
> That's why I was asking to adopt this only in the ramp down path.
>

But this is the supposed behaviour of conservative governor. We want
the CPU to increase the frequency in steps. The patch just resets
the frequency to a lower frequency in case of idle.

For argument's sake, let's assume that the governor timer is never
deferred and runs every sampling period even on completely idle CPU.
And let's assume, for example, a burst load that runs every 100ms
for 20ms. The default sampling rate is also 20ms.
What would conservative do in case of that burst load? It would
increase the frequency by one freq step after 20ms and then it would
decrease the frequency 4 times by one frequency step. Most probably
on the next burst load, the CPU will run on min frequency.

I agree that maybe this is not ideal for performance but maybe this is
how we want conservative governor to work (lazily increase and decrease
frequency).


Regards,
Stratos

2016-11-09 05:55:43

by Viresh Kumar

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

On 08-11-16, 21:25, Stratos Karafotis wrote:
> But this is the supposed behaviour of conservative governor. We want
> the CPU to increase the frequency in steps. The patch just resets
> the frequency to a lower frequency in case of idle.
>
> For argument's sake, let's assume that the governor timer is never
> deferred and runs every sampling period even on completely idle CPU.

There are no timers now :)

> And let's assume, for example, a burst load that runs every 100ms
> for 20ms. The default sampling rate is also 20ms.
> What would conservative do in case of that burst load? It would
> increase the frequency by one freq step after 20ms and then it would
> decrease the frequency 4 times by one frequency step. Most probably
> on the next burst load, the CPU will run on min frequency.
>
> I agree that maybe this is not ideal for performance but maybe this is
> how we want conservative governor to work (lazily increase and decrease
> frequency).

Idle periods are already accounted for while calculating system load by legacy
governors.

And the more and more I think about this, I am inclined towards your patch.
Maybe in a bit different form and commit log.

If we see how the governors were written initially, there were no deferred
timers. And so even if CPUs were idle, we will wake up to adjust the step.

Even if we want to make the behavior similar to that, then also we should
account of missed sampling periods both while decreasing or increasing
frequencies.

@Rafael: What do you think ?

--
viresh

2016-11-09 18:27:38

by Stratos Karafotis

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred



On 09/11/2016 07:55 πμ, Viresh Kumar wrote:
> On 08-11-16, 21:25, Stratos Karafotis wrote:
>> But this is the supposed behaviour of conservative governor. We want
>> the CPU to increase the frequency in steps. The patch just resets
>> the frequency to a lower frequency in case of idle.
>>
>> For argument's sake, let's assume that the governor timer is never
>> deferred and runs every sampling period even on completely idle CPU.
>
> There are no timers now :)
>
>> And let's assume, for example, a burst load that runs every 100ms
>> for 20ms. The default sampling rate is also 20ms.
>> What would conservative do in case of that burst load? It would
>> increase the frequency by one freq step after 20ms and then it would
>> decrease the frequency 4 times by one frequency step. Most probably
>> on the next burst load, the CPU will run on min frequency.
>>
>> I agree that maybe this is not ideal for performance but maybe this is
>> how we want conservative governor to work (lazily increase and decrease
>> frequency).
>
> Idle periods are already accounted for while calculating system load by legacy
> governors.
>
> And the more and more I think about this, I am inclined towards your patch.
> Maybe in a bit different form and commit log.
>
> If we see how the governors were written initially, there were no deferred
> timers. And so even if CPUs were idle, we will wake up to adjust the step.
>
> Even if we want to make the behavior similar to that, then also we should
> account of missed sampling periods both while decreasing or increasing
> frequencies.

I have already tested a patch that tries to account the missed sampling
periods in load calculation and I will submit it, but I thought that
conservative should drop the frequency after the timer (or the update)
is deferred. This is the reason I first submitted this patch.


Regards,
Stratos

2016-11-10 00:13:52

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred

On Wed, Nov 9, 2016 at 6:55 AM, Viresh Kumar <[email protected]> wrote:
> On 08-11-16, 21:25, Stratos Karafotis wrote:
>> But this is the supposed behaviour of conservative governor. We want
>> the CPU to increase the frequency in steps. The patch just resets
>> the frequency to a lower frequency in case of idle.
>>
>> For argument's sake, let's assume that the governor timer is never
>> deferred and runs every sampling period even on completely idle CPU.
>
> There are no timers now :)
>
>> And let's assume, for example, a burst load that runs every 100ms
>> for 20ms. The default sampling rate is also 20ms.
>> What would conservative do in case of that burst load? It would
>> increase the frequency by one freq step after 20ms and then it would
>> decrease the frequency 4 times by one frequency step. Most probably
>> on the next burst load, the CPU will run on min frequency.
>>
>> I agree that maybe this is not ideal for performance but maybe this is
>> how we want conservative governor to work (lazily increase and decrease
>> frequency).
>
> Idle periods are already accounted for while calculating system load by legacy
> governors.
>
> And the more and more I think about this, I am inclined towards your patch.
> Maybe in a bit different form and commit log.
>
> If we see how the governors were written initially, there were no deferred
> timers. And so even if CPUs were idle, we will wake up to adjust the step.
>
> Even if we want to make the behavior similar to that, then also we should
> account of missed sampling periods both while decreasing or increasing
> frequencies.
>
> @Rafael: What do you think ?

It looks like the issue with the conservative governor is real, but
I'm a bit concerned about adding things to use by one particular
governor only to cpufreq_governor.c.

Apart from the timer-related terminology that is not applicable any
more, of course.

Thanks,
Rafael

2016-11-10 15:48:47

by Stratos Karafotis

[permalink] [raw]
Subject: Re: [Resend][PATCH] cpufreq: conservative: Decrease frequency faster when the timer deferred



On 10/11/2016 02:13 πμ, Rafael J. Wysocki wrote:
> On Wed, Nov 9, 2016 at 6:55 AM, Viresh Kumar <[email protected]> wrote:
>> On 08-11-16, 21:25, Stratos Karafotis wrote:
>>> But this is the supposed behaviour of conservative governor. We want
>>> the CPU to increase the frequency in steps. The patch just resets
>>> the frequency to a lower frequency in case of idle.
>>>
>>> For argument's sake, let's assume that the governor timer is never
>>> deferred and runs every sampling period even on completely idle CPU.
>>
>> There are no timers now :)
>>
>>> And let's assume, for example, a burst load that runs every 100ms
>>> for 20ms. The default sampling rate is also 20ms.
>>> What would conservative do in case of that burst load? It would
>>> increase the frequency by one freq step after 20ms and then it would
>>> decrease the frequency 4 times by one frequency step. Most probably
>>> on the next burst load, the CPU will run on min frequency.
>>>
>>> I agree that maybe this is not ideal for performance but maybe this is
>>> how we want conservative governor to work (lazily increase and decrease
>>> frequency).
>>
>> Idle periods are already accounted for while calculating system load by legacy
>> governors.
>>
>> And the more and more I think about this, I am inclined towards your patch.
>> Maybe in a bit different form and commit log.
>>
>> If we see how the governors were written initially, there were no deferred
>> timers. And so even if CPUs were idle, we will wake up to adjust the step.
>>
>> Even if we want to make the behavior similar to that, then also we should
>> account of missed sampling periods both while decreasing or increasing
>> frequencies.
>>
>> @Rafael: What do you think ?
>
> It looks like the issue with the conservative governor is real, but
> I'm a bit concerned about adding things to use by one particular
> governor only to cpufreq_governor.c.

I think the code is minimum and I didn't find a way to do this
calculation in cpufreq_conservative.c. We also use code in
cpufreq_governor.c that it's only specific to ondemand (io_busy).

If you can give me a hint about how to implement this logic in
cpufreq_conservative I would appreciate it.

> Apart from the timer-related terminology that is not applicable any
> more, of course.

I will correct the terminology if the logic is accepted.


Regards,
Stratos