Re: [PATCH] cpufreq: conservative: Drop cached requested_freq

[Viresh Kumar]

On 10-03-26, 21:17, Zhongqiu Han wrote:
> Thanks for the patch. The fix looks correct to me for the reported
> issue. I do have one question though - should we also consider the
> interaction with commit abb6627910a1 ("cpufreq: conservative: Fix
> next frequency selection")?

Thanks for pointing this, I missed this change.

> However, that change was subsequently reverted by commit abb6627910a1
> ("cpufreq: conservative: Fix next frequency selection"), which noted
> that using policy->cur directly broke the algorithm when freq_step is
> small relative to the distances between available frequencies. In that
> case, the governor may not be able to stay within a narrow range
> between two consecutive available frequencies and instead jumps through
> steps faster than intended.

Its the opposite I think. The governor stays within a range and never
goes to a higher or lower frequency. This is how I think this happens:
- Lets say frequencies are from 1GHz to 2GHz with a gap of 200 MHz.
- Lets say the frequency is 1 GHz now and the step size is 100 MHz.
- conservative governor will try to change the freq to 1.1 GHz and end
  up selecting 1 GHz only (due to CPUFREQ_RELATION_H, highest freq
  below 1.1 GHz).
- With my patch, we will keep resetting to 1 GHz (cur freq) and never
  change freq.
- With a recorded requested_freq, we will move to 1.1 GHz (actual 1
  GHz), but a subsequent call will go for 1.2 GHz.

Here is another idea, once everyone agrees I can send this formally:

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index e0e847764511..df01d33993d8 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -313,6 +313,17 @@ static void cs_start(struct cpufreq_policy *policy)
        dbs_info->requested_freq = policy->cur;
 }

+static void cs_limits(struct cpufreq_policy *policy)
+{
+       struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
+
+       /*
+        * The limits have changed, so may have the current frequency. Reset
+        * requested_freq to avoid any unintended outcomes due to the mismatch.
+        */
+       dbs_info->requested_freq = policy->cur;
+}
+
 static struct dbs_governor cs_governor = {
        .gov = CPUFREQ_DBS_GOVERNOR_INITIALIZER("conservative"),
        .kobj_type = { .default_groups = cs_groups },
@@ -322,6 +333,7 @@ static struct dbs_governor cs_governor = {
        .init = cs_init,
        .exit = cs_exit,
        .start = cs_start,
+       .limits = cs_limits,
 };

 #define CPU_FREQ_GOV_CONSERVATIVE      (cs_governor.gov)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 36eb7aee4bcd..acf101878733 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -563,6 +563,7 @@ EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_stop);

 void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
 {
+       struct dbs_governor *gov = dbs_governor_of(policy);
        struct policy_dbs_info *policy_dbs;

        /* Protect gov->gdbs_data against cpufreq_dbs_governor_exit() */
@@ -574,6 +575,8 @@ void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
        mutex_lock(&policy_dbs->update_mutex);
        cpufreq_policy_apply_limits(policy);
        gov_update_sample_delay(policy_dbs, 0);
+       if (gov->limits)
+               gov->limits(policy);
        mutex_unlock(&policy_dbs->update_mutex);

 out:
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 168c23fd7fca..1462d59277bd 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -138,6 +138,7 @@ struct dbs_governor {
        int (*init)(struct dbs_data *dbs_data);
        void (*exit)(struct dbs_data *dbs_data);
        void (*start)(struct cpufreq_policy *policy);
+       void (*limits)(struct cpufreq_policy *policy);
 };

 static inline struct dbs_governor *dbs_governor_of(struct cpufreq_policy *policy)


-- 
viresh