Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

["Rafael J. Wysocki" <rjw@sisk.pl>]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.