On 28-10-15, 08:46, Rafael J. Wysocki wrote:
On Wednesday, October 28, 2015 12:13:17 PM Viresh Kumar wrote:
Actually yeah, but then the fourth patch of this series uses the timer_mutex to fix a long standing problem (which was fixed by hacking the code earlier). And so we need to take the lock for the entire dbs_timer() routine.
Well, there is another reason why the lock is taken for the complete dbs_timer() routine. There are two parts of that routine: - Checking if load evaluation is required or not + updating the last-update time. - The second is the load evaluation + freq change thing.
Lock around the first check makes sure that timer handlers of other CPUs don't do load evaluation in parallel and that they don't do it before the sampling period.
Lock around the second part makes sure there is only one thread which is doing load evaluation + freq update. The other thread being cpufreq_governor_limits(). And so the same lock taken across that part as well.
I don't actually think that that patch is correct and even if it is, we'll only need to do that *after* that patch, so at least it would be fair to say a word about it in the changelog, wouldn't it?
Hmm, If you agree about the above reasoning, then we may not require an update to the changelog, otherwise I will mention that in the changelog of this patch.
Yeah, we are calling dbs_check_cpu(dbs_data, cpu) from that path, which will reevaluate the load.
Which means that we should take the lock around dbs_check_cpu() everywhere in a consistent way.
We already do this from everywhere.
Which in turn means that the lock actually does more than you said.
What I described towards the top is probably a better answer to the earlier query.
My point is basically that we seem to have a vague idea about what the lock is used for, while we need to know exactly why we need it.
I am totally with you on this, we have surely screwed up on locking for a long time in cpufreq. And we should know exactly why we want to change it now.