Messages in this thread | | | Date | Tue, 8 Sep 2015 07:28:31 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH V2 3/9] cpufreq: ondemand: only queue canceled works from update_sampling_rate() |
| |
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
| |