On Mon, 2013-09-16 at 22:38 +0530, Viresh Kumar wrote: > On 16 September 2013 21:57, Jon Medhurst (Tixy) <tixy@xxxxxxxxxx> wrote: > > If I take mainline code and just change the line above to: > > You meant this line by above line? > > unlock_policy_rwsem_write(cpu); Yes. > > up_write(&per_cpu(cpu_policy_rwsem, (per_cpu(cpufreq_cpu_data, > > cpu))->last_cpu)); > > then the big_little cpufreq driver works for me. > > That would be same as: up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu)); Yes. (I had cut'n'pasted from the unlock_policy_rwsem_ macro) > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >> index 43c24aa..c18bf7b 100644 > >> --- a/drivers/cpufreq/cpufreq.c > >> +++ b/drivers/cpufreq/cpufreq.c > >> @@ -952,9 +952,16 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) > >> if (cpu == policy->cpu) > >> return; > >> > >> + /* take direct locks as lock_policy_rwsem_write wouldn't work here */ > >> + down_write(&per_cpu(cpu_policy_rwsem, policy->cpu)); > >> + down_write(&per_cpu(cpu_policy_rwsem, cpu)); > >> + > >> policy->last_cpu = policy->cpu; > >> policy->cpu = cpu; > >> > >> + up_write(&per_cpu(cpu_policy_rwsem, cpu)); > >> + up_write(&per_cpu(cpu_policy_rwsem, policy->cpu)); > > > > You've just overwritten policy->cpu with cpu. > > Stupid enough :) > > > I tried using > > policy->last_cpu to fix that, but it still doesn't work for me (giving > > the lockdep warning I mentioned.) If I change the code to just lock the > > original policy->cpu lock only, then all is fine. > > Fixed my patch now.. find attached.. The commit log to that patch still mentions taking both locks. The code itself fixes the lockdep errors I had, so Tested-by: Jon Medhurst <tixy@xxxxxxxxxx> however, I still have concerns about the locking (below)... > It mentions why lock for last cpu is > enough here. Copied that here too.. > > + /* > + * Take direct locks as lock_policy_rwsem_write wouldn't work here. > + * Also lock for last cpu is enough here as contention will happen only > + * after policy->cpu is changed and after it is changed, other threads > + * will try to acquire lock for new cpu. And policy is already updated > + * by then. > + */ > > > Also, this locking is now just happening around policy->cpu update, > > whereas before this change, it was locked for the whole of > > update_policy_cpu, i.e. cpufreq_frequency_table_update_policy_cpu and > > the notifier callbacks. Is that change of lock coverage OK? > > Yeah, the rwsem is only required for updating policy's fields and so that > should be okay. But what about reads, like in cpufreq_frequency_table_update_policy_cpu which is called immediately after the unlocking writes? Should that not be covered by a reader lock? And after that call, policy is passed into blocking_notifier_call_chain, do those callbacks not want to look at policy fields? Or are they going to do there own locking? Or is update_policy_cpu itself meant to be called with a read lock held? (It doesn't appear to be as the semaphore 'activiy' field of the lock is zero). Or is there some other non-obvious synchronisation method which means the policy object can't change? This is the first time I've looked at this code, so feel free just to say 'it's complicated, just trust me, I'm the expert, I know what I'm doing'... -- Tixy -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
