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

[Viresh Kumar]

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