| Message ID | 1353947996-26723-2-git-send-email-fabio.baltieri@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Monday, November 26, 2012 05:39:52 PM Fabio Baltieri wrote: > From: Rickard Andersson <rickard.andersson@stericsson.com> > > This patch fixes a bug that occurred when we had load on a secondary CPU > and the primary CPU was sleeping. Only one sampling timer was spawned > and it was spawned as a deferred timer on the primary CPU, so when a > secondary CPU had a change in load this was not detected by the cpufreq > governor (both ondemand and conservative). > > This patch make sure that deferred timers are run on all CPUs in the > case of software controlled CPUs that run on the same frequency. > > Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com> > Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org> > --- > drivers/cpufreq/cpufreq_conservative.c | 3 +- > drivers/cpufreq/cpufreq_governor.c | 52 ++++++++++++++++++++++++++++++---- > drivers/cpufreq/cpufreq_governor.h | 1 + > drivers/cpufreq/cpufreq_ondemand.c | 3 +- > 4 files changed, 51 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c > index 64ef737..b9d7f14 100644 > --- a/drivers/cpufreq/cpufreq_conservative.c > +++ b/drivers/cpufreq/cpufreq_conservative.c > @@ -122,7 +122,8 @@ static void cs_dbs_timer(struct work_struct *work) > > dbs_check_cpu(&cs_dbs_data, cpu); > > - schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay); > + schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work, > + delay); > mutex_unlock(&dbs_info->cdbs.timer_mutex); > } > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > index 6c5f1d3..a00f02d 100644 > --- a/drivers/cpufreq/cpufreq_governor.c > +++ b/drivers/cpufreq/cpufreq_governor.c > @@ -161,13 +161,31 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) > } > EXPORT_SYMBOL_GPL(dbs_check_cpu); > > +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs) > +{ > + struct cpufreq_policy *policy = cdbs->cur_policy; > + > + return cpumask_weight(policy->cpus) > 1; > +} > +EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus); > + > static inline void dbs_timer_init(struct dbs_data *dbs_data, > - struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate) > + struct cpu_dbs_common_info *cdbs, > + unsigned int sampling_rate, > + int cpu) > { > int delay = delay_for_sampling_rate(sampling_rate); > + struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu); > + struct od_cpu_dbs_info_s *od_dbs_info; > + > + cancel_delayed_work_sync(&cdbs_local->work); > + > + if (dbs_data->governor == GOV_ONDEMAND) { > + od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu); > + od_dbs_info->sample_type = OD_NORMAL_SAMPLE; > + } The patch looks good in general except for the special case above. Why exactly is it necessary? Rafael
Hello Rafael, On Tue, Nov 27, 2012 at 11:05:52PM +0100, Rafael J. Wysocki wrote: > > static inline void dbs_timer_init(struct dbs_data *dbs_data, > > - struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate) > > + struct cpu_dbs_common_info *cdbs, > > + unsigned int sampling_rate, > > + int cpu) > > { > > int delay = delay_for_sampling_rate(sampling_rate); > > + struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu); > > + struct od_cpu_dbs_info_s *od_dbs_info; > > + > > + cancel_delayed_work_sync(&cdbs_local->work); > > + > > + if (dbs_data->governor == GOV_ONDEMAND) { > > + od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu); > > + od_dbs_info->sample_type = OD_NORMAL_SAMPLE; > > + } > > The patch looks good in general except for the special case above. > > Why exactly is it necessary? Now that you point it out... it's not! It was part of ondemand init and moved in cpufreq_governor_dbs, I forgot to take it out the way. Also, I think that cancel_delayed_work_sync can be removed too. Should I send an updated version as soon as I get an ack for the other patches in the series or do you want me to wait until 3.8-rc1? Thanks, Fabio
On Wednesday, November 28, 2012 11:51:20 AM Fabio Baltieri wrote: > Hello Rafael, Hi, > On Tue, Nov 27, 2012 at 11:05:52PM +0100, Rafael J. Wysocki wrote: > > > static inline void dbs_timer_init(struct dbs_data *dbs_data, > > > - struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate) > > > + struct cpu_dbs_common_info *cdbs, > > > + unsigned int sampling_rate, > > > + int cpu) > > > { > > > int delay = delay_for_sampling_rate(sampling_rate); > > > + struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu); > > > + struct od_cpu_dbs_info_s *od_dbs_info; > > > + > > > + cancel_delayed_work_sync(&cdbs_local->work); > > > + > > > + if (dbs_data->governor == GOV_ONDEMAND) { > > > + od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu); > > > + od_dbs_info->sample_type = OD_NORMAL_SAMPLE; > > > + } > > > > The patch looks good in general except for the special case above. > > > > Why exactly is it necessary? > > Now that you point it out... it's not! It was part of ondemand init and > moved in cpufreq_governor_dbs, I forgot to take it out the way. > > Also, I think that cancel_delayed_work_sync can be removed too. > > Should I send an updated version as soon as I get an ack for the other > patches in the series or do you want me to wait until 3.8-rc1? Well, if it's not very urgent, I'd prefer it to wait a bit longer, get some more testing and so on. Thanks, Rafael
diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c index 64ef737..b9d7f14 100644 --- a/drivers/cpufreq/cpufreq_conservative.c +++ b/drivers/cpufreq/cpufreq_conservative.c @@ -122,7 +122,8 @@ static void cs_dbs_timer(struct work_struct *work) dbs_check_cpu(&cs_dbs_data, cpu); - schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay); + schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work, + delay); mutex_unlock(&dbs_info->cdbs.timer_mutex); } diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 6c5f1d3..a00f02d 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -161,13 +161,31 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) } EXPORT_SYMBOL_GPL(dbs_check_cpu); +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs) +{ + struct cpufreq_policy *policy = cdbs->cur_policy; + + return cpumask_weight(policy->cpus) > 1; +} +EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus); + static inline void dbs_timer_init(struct dbs_data *dbs_data, - struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate) + struct cpu_dbs_common_info *cdbs, + unsigned int sampling_rate, + int cpu) { int delay = delay_for_sampling_rate(sampling_rate); + struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu); + struct od_cpu_dbs_info_s *od_dbs_info; + + cancel_delayed_work_sync(&cdbs_local->work); + + if (dbs_data->governor == GOV_ONDEMAND) { + od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu); + od_dbs_info->sample_type = OD_NORMAL_SAMPLE; + } - INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer); - schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay); + schedule_delayed_work_on(cpu, &cdbs_local->work, delay); } static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs) @@ -217,6 +235,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data, if (ignore_nice) j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE]; + + mutex_init(&j_cdbs->timer_mutex); + INIT_DEFERRABLE_WORK(&j_cdbs->work, + dbs_data->gov_dbs_timer); } /* @@ -275,15 +297,33 @@ second_time: } mutex_unlock(&dbs_data->mutex); - mutex_init(&cpu_cdbs->timer_mutex); - dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate); + if (dbs_sw_coordinated_cpus(cpu_cdbs)) { + for_each_cpu(j, policy->cpus) { + struct cpu_dbs_common_info *j_cdbs; + + j_cdbs = dbs_data->get_cpu_cdbs(j); + dbs_timer_init(dbs_data, j_cdbs, + *sampling_rate, j); + } + } else { + dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu); + } break; case CPUFREQ_GOV_STOP: if (dbs_data->governor == GOV_CONSERVATIVE) cs_dbs_info->enable = 0; - dbs_timer_exit(cpu_cdbs); + if (dbs_sw_coordinated_cpus(cpu_cdbs)) { + for_each_cpu(j, policy->cpus) { + struct cpu_dbs_common_info *j_cdbs; + + j_cdbs = dbs_data->get_cpu_cdbs(j); + dbs_timer_exit(j_cdbs); + } + } else { + dbs_timer_exit(cpu_cdbs); + } mutex_lock(&dbs_data->mutex); mutex_destroy(&cpu_cdbs->timer_mutex); diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h index f661654..5bf6fb8 100644 --- a/drivers/cpufreq/cpufreq_governor.h +++ b/drivers/cpufreq/cpufreq_governor.h @@ -171,6 +171,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate) u64 get_cpu_idle_time(unsigned int cpu, u64 *wall); void dbs_check_cpu(struct dbs_data *dbs_data, int cpu); +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs); int cpufreq_governor_dbs(struct dbs_data *dbs_data, struct cpufreq_policy *policy, unsigned int event); #endif /* _CPUFREQ_GOVERNER_H */ diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index cca3e9f..fe6e47c 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -239,7 +239,8 @@ static void od_dbs_timer(struct work_struct *work) } } - schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay); + schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work, + delay); mutex_unlock(&dbs_info->cdbs.timer_mutex); }