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

[Viresh Kumar]

I am starting to follow cpufreq patches religiously now and so have to come
back to this old thread due to some crash we got :)

Its still not pushed upstream, so better to get it resolved before 3.9.

On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
<fabio.baltieri@xxxxxxxxxx> wrote:

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

>  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);

I couldn't understand the real need for this, as it should really give
back the same
pointer pointed out by: cdbs and hence no need of cpu in params too..

I may be wrong here :)

>
> -       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_local->work, delay);
>  }
>
>  static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
> @@ -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,33 @@ second_time:
>                 }
>                 mutex_unlock(&dbs_data->mutex);
>
> -               mutex_init(&cpu_cdbs->timer_mutex);
> -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> +                       for_each_cpu(j, policy->cpus) {
> +                               struct cpu_dbs_common_info *j_cdbs;
> +
> +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> +                               dbs_timer_init(dbs_data, j_cdbs,
> +                                              *sampling_rate, j);
> +                       }
> +               } else {
> +                       dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
> +               }

do you really need this else part? In case of uniprocessor systems also, the if
block should be enough. Isn't it?

>                 break;
>
>         case CPUFREQ_GOV_STOP:
>                 if (dbs_data->governor == GOV_CONSERVATIVE)
>                         cs_dbs_info->enable = 0;
>
> -               dbs_timer_exit(cpu_cdbs);
> +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> +                       for_each_cpu(j, policy->cpus) {
> +                               struct cpu_dbs_common_info *j_cdbs;
> +
> +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> +                               dbs_timer_exit(j_cdbs);
> +                       }
> +               } else {
> +                       dbs_timer_exit(cpu_cdbs);
> +               }

ditto.

>                 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);
>  }
>
> --
> 1.7.12.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/