| Message ID | 5893809.nB6uDLgMiy@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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;
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>
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;