| Message ID | 4f7aef4f032e082a5093b91d244647f339ef6558.1437999691.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Monday, July 27, 2015 05:58:09 PM Viresh Kumar wrote: > __gov_queue_work() isn't required anymore and can be merged with > gov_queue_work(). Do it. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Quite frankly I don't see the point. I'd even remove the inline from its definition and let the compiler decide what to do with it. > --- > drivers/cpufreq/cpufreq_governor.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > index a890450711bb..3ddc27764e10 100644 > --- a/drivers/cpufreq/cpufreq_governor.c > +++ b/drivers/cpufreq/cpufreq_governor.c > @@ -158,25 +158,20 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) > } > EXPORT_SYMBOL_GPL(dbs_check_cpu); > > -static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, > - unsigned int delay) > -{ > - struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); > - > - mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); > -} > - > void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, > unsigned int delay, const struct cpumask *cpus) > { > - int i; > + struct cpu_dbs_info *cdbs; > + int cpu; > > mutex_lock(&cpufreq_governor_lock); > if (!policy->governor_enabled) > goto out_unlock; > > - for_each_cpu(i, cpus) > - __gov_queue_work(i, dbs_data, delay); > + for_each_cpu(cpu, cpus) { > + cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); > + mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); > + } > > out_unlock: > mutex_unlock(&cpufreq_governor_lock); > Thanks, Rafael -- 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 08-09-15, 03:15, Rafael J. Wysocki wrote: > On Monday, July 27, 2015 05:58:09 PM Viresh Kumar wrote: > > __gov_queue_work() isn't required anymore and can be merged with > > gov_queue_work(). Do it. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Quite frankly I don't see the point. But isn't that just an unnecessary wrapper ? > I'd even remove the inline from its definition and let the compiler decide > what to do with it. What if the compiler decides to link it? Why add a function call for (almost) no use?
On Tuesday, September 08, 2015 07:30:44 AM Viresh Kumar wrote: > On 08-09-15, 03:15, Rafael J. Wysocki wrote: > > On Monday, July 27, 2015 05:58:09 PM Viresh Kumar wrote: > > > __gov_queue_work() isn't required anymore and can be merged with > > > gov_queue_work(). Do it. > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > Quite frankly I don't see the point. > > But isn't that just an unnecessary wrapper ? It isn't a wrapper, just a separation of code executed in each step of the loop. There's nothing wrong with having a separate function for that in principle. I wouldn't make a fuss about that if that was new code even, so I don't see why we should change it. > > I'd even remove the inline from its definition and let the compiler decide > > what to do with it. > > What if the compiler decides to link it? Why add a function call for > (almost) no use? If the compiler does that, let it do it. :-) If you think that you can outsmart the compiler people by doing such optimizations at this level manually, you're likely wrong. Serious man-hours go into making that stuff work as well as it can in compilers. Thanks, Rafael -- 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
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index a890450711bb..3ddc27764e10 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -158,25 +158,20 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) } EXPORT_SYMBOL_GPL(dbs_check_cpu); -static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, - unsigned int delay) -{ - struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); - - mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); -} - void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, unsigned int delay, const struct cpumask *cpus) { - int i; + struct cpu_dbs_info *cdbs; + int cpu; mutex_lock(&cpufreq_governor_lock); if (!policy->governor_enabled) goto out_unlock; - for_each_cpu(i, cpus) - __gov_queue_work(i, dbs_data, delay); + for_each_cpu(cpu, cpus) { + cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); + mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); + } out_unlock: mutex_unlock(&cpufreq_governor_lock);
__gov_queue_work() isn't required anymore and can be merged with gov_queue_work(). Do it. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq_governor.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)