| Message ID | 1359550803-18577-2-git-send-email-fabio.baltieri@linaro.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Hi Fabio, I know Rafael has asked you to send only the incremental patch, but i am interested in reviewing whole series again, as i haven't reviewed earlier stuff :) I believe you are required to send patches to patches@linaro.org too :) On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> 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. Its really a good point. I wondered earlier why does interactive governor has per-cpu timer. BTW, you can check how interactive governor is handling this requirement. That might improve these drivers too. > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > +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); I believe this routine should be rather present in cpufreq core code, as their might be other users of it. Its really not related to dbs or governors. My ideas about the name of routine then: - policy_is_shared() - or something better you have :) > @@ -217,6 +227,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); That doesn't look the right place for doing it. With this change we will initialize mutex and work for all cpus whenever cpufreq_governor_dbs(GOV_START) is called. We really want to do it just before second_time: label, which will do it only when this is called for the very first time or cpu. That's what i thought initially :) Then i looked at my own work and realized something else :).. Now, because we stop/start governors on every cpu movement, this routine is called only once. What you can do: - Create a single for_each_cpu() in GOV_START path - Get rid of dbs_data->enable routine as we don't need to store the number of calls for GOV_START. > } > > /* > @@ -275,15 +289,16 @@ second_time: > } > mutex_unlock(&dbs_data->mutex); > > - mutex_init(&cpu_cdbs->timer_mutex); > - dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate); > + for_each_cpu(j, policy->cpus) > + dbs_timer_init(dbs_data, j, *sampling_rate); Keep this too in the same for_each_cpu() loop and hence get to older param types of dbs_timer_init(), i.e. don't pass cpu as we already have j_cdbs in our loop. > break; > > case CPUFREQ_GOV_STOP: > if (dbs_data->governor == GOV_CONSERVATIVE) > cs_dbs_info->enable = 0; > > - dbs_timer_exit(cpu_cdbs); > + for_each_cpu(j, policy->cpus) > + dbs_timer_exit(dbs_data, j); > > mutex_lock(&dbs_data->mutex); > mutex_destroy(&cpu_cdbs->timer_mutex); -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Viresh, On Wed, Jan 30, 2013 at 08:32:38PM +0530, Viresh Kumar wrote: > Hi Fabio, > > I know Rafael has asked you to send only the incremental patch, but i > am interested in reviewing whole series again, as i haven't reviewed > earlier stuff :) Sure, take your chance. [internal note] > I believe you are required to send patches to patches@linaro.org too :) I already have patches@linaro.org in BCC, all my patches are there. [/internal note] > On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> 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. > > Its really a good point. I wondered earlier why does interactive governor > has per-cpu timer. BTW, you can check how interactive governor is handling > this requirement. That might improve these drivers too. > > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > > +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); > > I believe this routine should be rather present in cpufreq core code, > as their might > be other users of it. Its really not related to dbs or governors. > > My ideas about the name of routine then: > - policy_is_shared() > - or something better you have :) So you are suggesting to rethink this function to be related to policy rather than dbs... this may as well become an inline in cpufreq.h, as: static inline bool policy_is_shared(struct cpufreq_policy *policy) { return cpumask_weight(policy->cpus) > 1; } I'm not sure about the name through, I like mentioning sw coordination in it because that's the comment in the declaration: cpumask_var_t cpus; /* CPUs requiring sw coordination */ cpumask_var_t related_cpus; /* CPUs with any coordination */ And those two are already confusing as a starting point. Anyway, this sounds fine to me. If you think this is useful I can send a patch, or feel free to include it in your patches if you plan to do further cleanup work on this code. /me tries to also keep that ->cpu field in mind. > > @@ -217,6 +227,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); > > That doesn't look the right place for doing it. With this change we > will initialize > mutex and work for all cpus whenever cpufreq_governor_dbs(GOV_START) is > called. We really want to do it just before second_time: label, which will do > it only when this is called for the very first time or cpu. > > That's what i thought initially :) > > Then i looked at my own work and realized something else :).. Now, because > we stop/start governors on every cpu movement, this routine is called only once. > > What you can do: > - Create a single for_each_cpu() in GOV_START path > - Get rid of dbs_data->enable routine as we don't need to store the > number of calls > for GOV_START. > > > } > > > > /* > > @@ -275,15 +289,16 @@ second_time: > > } > > mutex_unlock(&dbs_data->mutex); > > > > - mutex_init(&cpu_cdbs->timer_mutex); > > - dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate); > > + for_each_cpu(j, policy->cpus) > > + dbs_timer_init(dbs_data, j, *sampling_rate); > > Keep this too in the same for_each_cpu() loop and hence get to older > param types of dbs_timer_init(), i.e. don't pass cpu as we already have > j_cdbs in our loop. Ok, I can try to implement this if you help testing the patch on your bL system. :-) Fabio > > break; > > > > case CPUFREQ_GOV_STOP: > > if (dbs_data->governor == GOV_CONSERVATIVE) > > cs_dbs_info->enable = 0; > > > > - dbs_timer_exit(cpu_cdbs); > > + for_each_cpu(j, policy->cpus) > > + dbs_timer_exit(dbs_data, j); > > > > mutex_lock(&dbs_data->mutex); > > mutex_destroy(&cpu_cdbs->timer_mutex);
On 30 January 2013 21:53, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > On Wed, Jan 30, 2013 at 08:32:38PM +0530, Viresh Kumar wrote: >> I believe this routine should be rather present in cpufreq core code, >> as their might >> be other users of it. Its really not related to dbs or governors. >> >> My ideas about the name of routine then: >> - policy_is_shared() >> - or something better you have :) > > So you are suggesting to rethink this function to be related to policy > rather than dbs... this may as well become an inline in cpufreq.h, as: > > static inline bool policy_is_shared(struct cpufreq_policy *policy) > { > return cpumask_weight(policy->cpus) > 1; > } Perfect. > I'm not sure about the name through, I like mentioning sw coordination in it > because that's the comment in the declaration: > > cpumask_var_t cpus; /* CPUs requiring sw coordination */ > cpumask_var_t related_cpus; /* CPUs with any coordination */ > > And those two are already confusing as a starting point. I will fix these comments with a patch of mine. > Anyway, this sounds fine to me. If you think this is useful I can send > a patch, or feel free to include it in your patches if you plan to do > further cleanup work on this code. > > /me tries to also keep that ->cpu field in mind. You can make it part of your patchsets v8. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 30, 2013 at 10:21:03PM +0530, Viresh Kumar wrote: > > I'm not sure about the name through, I like mentioning sw coordination in it > > because that's the comment in the declaration: > > > > cpumask_var_t cpus; /* CPUs requiring sw coordination */ > > cpumask_var_t related_cpus; /* CPUs with any coordination */ > > > > And those two are already confusing as a starting point. > > I will fix these comments with a patch of mine. Great! > > > Anyway, this sounds fine to me. If you think this is useful I can send > > a patch, or feel free to include it in your patches if you plan to do > > further cleanup work on this code. > > > > /me tries to also keep that ->cpu field in mind. > > You can make it part of your patchsets v8. I'm not sending a v8 as Rafael already asked for incremental, but I'll send a patch with that soon. Thanks, Fabio
On Wednesday, January 30, 2013 05:57:39 PM Fabio Baltieri wrote: > On Wed, Jan 30, 2013 at 10:21:03PM +0530, Viresh Kumar wrote: > > > I'm not sure about the name through, I like mentioning sw coordination in it > > > because that's the comment in the declaration: > > > > > > cpumask_var_t cpus; /* CPUs requiring sw coordination */ > > > cpumask_var_t related_cpus; /* CPUs with any coordination */ > > > > > > And those two are already confusing as a starting point. > > > > I will fix these comments with a patch of mine. > > Great! > > > > > > Anyway, this sounds fine to me. If you think this is useful I can send > > > a patch, or feel free to include it in your patches if you plan to do > > > further cleanup work on this code. > > > > > > /me tries to also keep that ->cpu field in mind. > > > > You can make it part of your patchsets v8. > > I'm not sending a v8 as Rafael already asked for incremental, but I'll > send a patch with that soon. Yes, please. 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..8393d6e 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -161,17 +161,27 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) } EXPORT_SYMBOL_GPL(dbs_check_cpu); -static inline void dbs_timer_init(struct dbs_data *dbs_data, - struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate) +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, int cpu, + unsigned int sampling_rate) { int delay = delay_for_sampling_rate(sampling_rate); + struct cpu_dbs_common_info *cdbs = dbs_data->get_cpu_cdbs(cpu); - 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->work, delay); } -static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs) +static inline void dbs_timer_exit(struct dbs_data *dbs_data, int cpu) { + struct cpu_dbs_common_info *cdbs = dbs_data->get_cpu_cdbs(cpu); + cancel_delayed_work_sync(&cdbs->work); } @@ -217,6 +227,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 +289,16 @@ second_time: } mutex_unlock(&dbs_data->mutex); - mutex_init(&cpu_cdbs->timer_mutex); - dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate); + for_each_cpu(j, policy->cpus) + dbs_timer_init(dbs_data, j, *sampling_rate); break; case CPUFREQ_GOV_STOP: if (dbs_data->governor == GOV_CONSERVATIVE) cs_dbs_info->enable = 0; - dbs_timer_exit(cpu_cdbs); + for_each_cpu(j, policy->cpus) + dbs_timer_exit(dbs_data, j); 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 7731f7c..93bb56d 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -243,7 +243,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); }