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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
