Re: [PATCH V3 1/5] cpufreq: ondemand: Drop unnecessary locks from update_sampling_rate()

[Viresh Kumar <viresh.kumar@linaro.org>]

On 28-10-15, 05:05, Rafael J. Wysocki wrote:
> On Tuesday, October 13, 2015 01:39:01 PM Viresh Kumar wrote:
> > 'timer_mutex' is required to sync work-handlers of policy->cpus.
> > update_sampling_rate() is just canceling the works and queuing them
> > again. This isn't protecting anything at all in update_sampling_rate()
> > and is not gonna be of any use.
> > 
> > Even if a work-handler is already running for a CPU,
> > cancel_delayed_work_sync() will wait for it to finish.
> > 
> > Drop these unnecessary locks.
> > 
> > Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> I'm queuing this up for 4.4, although I think that the changelog is not right.
> 
> While at it, what are the race conditions the lock is protecting against?

In cases where a single policy controls multiple CPUs, a timer is
queued for every cpu present in policy->cpus. When we reach the timer
handler (which can be on multiple CPUs together) on any CPU, we trace
CPU load for all policy->cpus and update the frequency accordingly.

The lock is for protecting multiple CPUs to do the same thing
together, as only its required to be done by a single CPU. Once any
CPUs handler has completed, it updates the last update time and drops
the mutex. At that point of time, other blocked handler (if any) check
the last update time and return early.

And then there are enough minute things that can go wrong if multiple
CPUs do the load evaluation and freq-update at the same time, apart
from it being an time wasting effort.

And so I still think that the commit log isn't that bad. The
timer_mutex lock isn't required in other parts of the governor, they
are just for synchronizing the work-handlers of CPUs belonging to the
same policy.

-- 
viresh