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

Groups > linux.kernel > #1220431 > unrolled thread

Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()

Started by"Rafael J. Wysocki" <rjw@rjwysocki.net>
First post2015-09-08 02:50 +0200
Last post2015-09-09 21:50 +0200
Articles 5 — 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 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-08 02:50 +0200
    Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from  update_sampling_rate() Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-08 04:00 +0200
      Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-09 02:40 +0200
        Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from  update_sampling_rate() Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-09 04:40 +0200
          Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-09 21:50 +0200

#1220431 — Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-09-08 02:50 +0200
SubjectRe: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()
Message-ID<q6fgK-5ca-5@gated-at.bofh.it>
On Monday, July 27, 2015 05:58:08 PM Viresh Kumar wrote:
> The sampling rate is updated with a call to update_sampling_rate(), and
> we process CPUs one by one here. While the work is canceled on per-cpu
> basis, it is getting queued (by mistake) for all policy->cpus.
> 
> This would result in wasting cpu cycles for queuing works which are
> already queued and never canceled.
> 
> This patch changes this behavior to queue work only on the cpu, for
> which it was canceled earlier.
> 
> To do that, replace 'modify_all' parameter to gov_queue_work() with a
> mask of CPUs.

There really are two cases, either you pass a CPU or gov_queue_work() has to
walk policy->cpus.  Doing it the way you did hides that IMO.

I'd simply pass an int and use a special value to indicate that policy->cpus
is to be walked.

> Also the last parameter to ->gov_dbs_timer() was named
> 'modify_all' earlier, but its purpose was to decide if load has to be
> evaluated again or not. Lets rename that to load_eval.
> 
> Fixes: 031299b3be30 ("cpufreq: governors: Avoid unnecessary per cpu timer interrupts")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |  4 ++--
>  drivers/cpufreq/cpufreq_governor.c     | 30 ++++++++++--------------------
>  drivers/cpufreq/cpufreq_governor.h     |  4 ++--
>  drivers/cpufreq/cpufreq_ondemand.c     |  7 ++++---
>  4 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 18bfbc313e48..1aa3bd46cea3 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -116,11 +116,11 @@ static void cs_check_cpu(int cpu, unsigned int load)
>  }
>  
>  static unsigned int cs_dbs_timer(struct cpu_dbs_info *cdbs,
> -				 struct dbs_data *dbs_data, bool modify_all)
> +				 struct dbs_data *dbs_data, bool load_eval)
>  {
>  	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>  
> -	if (modify_all)
> +	if (load_eval)
>  		dbs_check_cpu(dbs_data, cdbs->shared->policy->cpu);
>  
>  	return delay_for_sampling_rate(cs_tuners->sampling_rate);
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 750626d8fb03..a890450711bb 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -167,7 +167,7 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>  }
>  
>  void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> -		unsigned int delay, bool all_cpus)
> +		    unsigned int delay, const struct cpumask *cpus)
>  {
>  	int i;
>  
> @@ -175,19 +175,8 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>  	if (!policy->governor_enabled)
>  		goto out_unlock;
>  
> -	if (!all_cpus) {
> -		/*
> -		 * Use raw_smp_processor_id() to avoid preemptible warnings.
> -		 * We know that this is only called with all_cpus == false from
> -		 * works that have been queued with *_work_on() functions and
> -		 * those works are canceled during CPU_DOWN_PREPARE so they
> -		 * can't possibly run on any other CPU.
> -		 */

This was a useful comment and it should be moved along the logic it was supposed
to explain and not just dropped.

> -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
> -	} else {
> -		for_each_cpu(i, policy->cpus)
> -			__gov_queue_work(i, dbs_data, delay);
> -	}
> +	for_each_cpu(i, cpus)
> +		__gov_queue_work(i, dbs_data, delay);
>  
>  out_unlock:
>  	mutex_unlock(&cpufreq_governor_lock);
> @@ -232,7 +221,8 @@ static void dbs_timer(struct work_struct *work)
>  	struct cpufreq_policy *policy = shared->policy;
>  	struct dbs_data *dbs_data = policy->governor_data;
>  	unsigned int sampling_rate, delay;
> -	bool modify_all = true;
> +	const struct cpumask *cpus;

I don't think this local variable is necessary.

> +	bool load_eval;
>  
>  	mutex_lock(&shared->timer_mutex);
>  
> @@ -246,11 +236,11 @@ static void dbs_timer(struct work_struct *work)
>  		sampling_rate = od_tuners->sampling_rate;
>  	}
>  
> -	if (!need_load_eval(cdbs->shared, sampling_rate))
> -		modify_all = false;
> +	load_eval = need_load_eval(cdbs->shared, sampling_rate);
> +	cpus = load_eval ? policy->cpus : cpumask_of(raw_smp_processor_id());
>  
> -	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all);
> -	gov_queue_work(dbs_data, policy, delay, modify_all);
> +	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, load_eval);
> +	gov_queue_work(dbs_data, policy, delay, cpus);
>  
>  	mutex_unlock(&shared->timer_mutex);
>  }
> @@ -474,7 +464,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
>  	}
>  
>  	gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate),
> -		       true);
> +		       policy->cpus);
>  	return 0;
>  }
>  
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index 5621bb03e874..52665a0624b2 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -211,7 +211,7 @@ struct common_dbs_data {
>  	void *(*get_cpu_dbs_info_s)(int cpu);
>  	unsigned int (*gov_dbs_timer)(struct cpu_dbs_info *cdbs,
>  				      struct dbs_data *dbs_data,
> -				      bool modify_all);
> +				      bool load_eval);
>  	void (*gov_check_cpu)(int cpu, unsigned int load);
>  	int (*init)(struct dbs_data *dbs_data, bool notify);
>  	void (*exit)(struct dbs_data *dbs_data, bool notify);
> @@ -273,7 +273,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
>  int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  		struct common_dbs_data *cdata, unsigned int event);
>  void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> -		unsigned int delay, bool all_cpus);
> +		    unsigned int delay, const struct cpumask *cpus);
>  void od_register_powersave_bias_handler(unsigned int (*f)
>  		(struct cpufreq_policy *, unsigned int, unsigned int),
>  		unsigned int powersave_bias);
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 1fa9088c84a8..2474c9c34022 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -192,7 +192,7 @@ static void od_check_cpu(int cpu, unsigned int load)
>  }
>  
>  static unsigned int od_dbs_timer(struct cpu_dbs_info *cdbs,
> -				 struct dbs_data *dbs_data, bool modify_all)
> +				 struct dbs_data *dbs_data, bool load_eval)
>  {
>  	struct cpufreq_policy *policy = cdbs->shared->policy;
>  	unsigned int cpu = policy->cpu;
> @@ -201,7 +201,7 @@ static unsigned int od_dbs_timer(struct cpu_dbs_info *cdbs,
>  	struct od_dbs_tuners *od_tuners = dbs_data->tuners;
>  	int delay = 0, sample_type = dbs_info->sample_type;
>  
> -	if (!modify_all)
> +	if (!load_eval)
>  		goto max_delay;
>  
>  	/* Common NORMAL_SAMPLE setup */
> @@ -284,7 +284,8 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
>  			mutex_lock(&dbs_info->cdbs.shared->timer_mutex);
>  
>  			gov_queue_work(dbs_data, policy,
> -				       usecs_to_jiffies(new_rate), true);
> +				       usecs_to_jiffies(new_rate),
> +				       cpumask_of(cpu));
>  
>  		}
>  		mutex_unlock(&dbs_info->cdbs.shared->timer_mutex);
> 

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]

#1220451 — Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-09-08 04:00 +0200
SubjectRe: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()
Message-ID<q6gmu-6J6-9@gated-at.bofh.it>
In reply to#1220431
On 08-09-15, 03:11, Rafael J. Wysocki wrote:
> There really are two cases, either you pass a CPU or gov_queue_work() has to
> walk policy->cpus.

Right (At least for now, we are doing just that.)

> Doing it the way you did hides that IMO.

Maybe. But I see it otherwise. Adding special meaning to a variable
(like int cpu == -1 being the special case to specify policy->cpus)
hides things morei, as we need to look at how it is decoded finally in
the routine gov_queue_work().

But if we send a mask instead, it is very clear by reading the callers
site, what we are trying to do.

> I'd simply pass an int and use a special value to indicate that policy->cpus
> is to be walked.

Like cpu == -1 thing? Or something else?

> > -	if (!all_cpus) {
> > -		/*
> > -		 * Use raw_smp_processor_id() to avoid preemptible warnings.
> > -		 * We know that this is only called with all_cpus == false from
> > -		 * works that have been queued with *_work_on() functions and
> > -		 * those works are canceled during CPU_DOWN_PREPARE so they
> > -		 * can't possibly run on any other CPU.
> > -		 */
> 
> This was a useful comment and it should be moved along the logic it was supposed
> to explain and not just dropped.

Sigh

> > -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
> > -	} else {
> > -		for_each_cpu(i, policy->cpus)
> > -			__gov_queue_work(i, dbs_data, delay);
> > -	}
> > +	for_each_cpu(i, cpus)
> > +		__gov_queue_work(i, dbs_data, delay);
> >  
> >  out_unlock:
> >  	mutex_unlock(&cpufreq_governor_lock);
> > @@ -232,7 +221,8 @@ static void dbs_timer(struct work_struct *work)
> >  	struct cpufreq_policy *policy = shared->policy;
> >  	struct dbs_data *dbs_data = policy->governor_data;
> >  	unsigned int sampling_rate, delay;
> > -	bool modify_all = true;
> > +	const struct cpumask *cpus;
> 
> I don't think this local variable is necessary.
> 
> > +	bool load_eval;
> >  
> >  	mutex_lock(&shared->timer_mutex);
> >  
> > @@ -246,11 +236,11 @@ static void dbs_timer(struct work_struct *work)
> >  		sampling_rate = od_tuners->sampling_rate;
> >  	}
> >  
> > -	if (!need_load_eval(cdbs->shared, sampling_rate))
> > -		modify_all = false;
> > +	load_eval = need_load_eval(cdbs->shared, sampling_rate);
> > +	cpus = load_eval ? policy->cpus : cpumask_of(raw_smp_processor_id());
> >  
> > -	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all);
> > -	gov_queue_work(dbs_data, policy, delay, modify_all);
> > +	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, load_eval);
> > +	gov_queue_work(dbs_data, policy, delay, cpus);

Avoiding that local variable would have made this a little longer, but
I can surely drop it :)

        gov_queue_work(dbs_data, policy, delay,
                       load_eval ? policy->cpus :
                                   cpumask_of(raw_smp_processor_id());

-- 
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]

#1221158

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-09-09 02:40 +0200
Message-ID<q6BAB-3Go-7@gated-at.bofh.it>
In reply to#1220451
On Tuesday, September 08, 2015 07:28:31 AM Viresh Kumar wrote:
> On 08-09-15, 03:11, Rafael J. Wysocki wrote:
> > There really are two cases, either you pass a CPU or gov_queue_work() has to
> > walk policy->cpus.
> 
> Right (At least for now, we are doing just that.)
> 
> > Doing it the way you did hides that IMO.
> 
> Maybe. But I see it otherwise. Adding special meaning to a variable
> (like int cpu == -1 being the special case to specify policy->cpus)
> hides things morei, as we need to look at how it is decoded finally in
> the routine gov_queue_work().

Oh well.

I've just realized that if you combined this patch with the [6/9],
you wouldn't need to make any changes to gov_queue_work() at all,
because that patch removes the case in point entirely.

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] | [next] | [standalone]

#1221190 — Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-09-09 04:40 +0200
SubjectRe: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate()
Message-ID<q6DsJ-6sy-3@gated-at.bofh.it>
In reply to#1221158
On 09-09-15, 03:06, Rafael J. Wysocki wrote:
> I've just realized that if you combined this patch with the [6/9],
> you wouldn't need to make any changes to gov_queue_work() at all,
> because that patch removes the case in point entirely.

Yeah, but then these are really two separate issues at hand and so I
solved them separately. Lemme know how you want to see that and I can
change :)

-- 
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]

#1221662

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-09-09 21:50 +0200
Message-ID<q6Txw-4jy-17@gated-at.bofh.it>
In reply to#1221190
On Wednesday, September 09, 2015 08:00:20 AM Viresh Kumar wrote:
> On 09-09-15, 03:06, Rafael J. Wysocki wrote:
> > I've just realized that if you combined this patch with the [6/9],
> > you wouldn't need to make any changes to gov_queue_work() at all,
> > because that patch removes the case in point entirely.
> 
> Yeah, but then these are really two separate issues at hand and so I
> solved them separately. Lemme know how you want to see that and I can
> change :)

I don't want to make artificial changes.

If you can address two problems in one go with one relatively simple patch,
why don't you do that?

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