Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220432 > unrolled thread

Re: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work()

Started by"Rafael J. Wysocki" <rjw@rjwysocki.net>
First post2015-09-08 02:50 +0200
Last post2015-09-09 02:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-08 02:50 +0200
    Re: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work() Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-08 04:10 +0200
      Re: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-09 02:40 +0200

#1220432 — Re: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work()

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-09-08 02:50 +0200
SubjectRe: [PATCH V2 4/9] cpufreq: governor: Drop __gov_queue_work()
Message-ID<q6fgK-5ca-3@gated-at.bofh.it>
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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220452

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-09-08 04:10 +0200
Message-ID<q6gw9-7aV-1@gated-at.bofh.it>
In reply to#1220432
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?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221155

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-09-09 02:40 +0200
Message-ID<q6BAB-3Go-1@gated-at.bofh.it>
In reply to#1220452
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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web