Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Prarit Bhargava <prarit@redhat.com>]

On 11/10/2014 05:44 AM, Viresh Kumar wrote:
> On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
>> commit 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking
>> scheme for cpufreq.
>>
>> Simple tests such as rapidly switching the governor between ondemand and
>> performance or attempting to read policy values while a governor switch occurs
>> now fail with very NULL pointer warnings, sysfs namespace collisions, and
>> system hangs.  In short, the locking that policy->rwsem is supposed to provide
>> is currently broken.
>>
>> The identified commit attempts to resolve a lockdep warning by removing
>> a lock around a section of code which does a shutdown of the
>> existing policy.  The problem is that this is part of the _critical_ section of
>> code that switches the governors and must be protected by the lock; without
>> locking readers may access now NULL or stale data, and writes may collide with
>> each other.
>>
>> With the previous patch, which now returns -EBUSY during times of
>> contention the deadlock reported in
>> 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock
>> around CPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back
>> into this section of code is possible.
> 
> I still fail to understand why ? What will the _trylock() change ?

viresh, afaict read_trylock will return 0 when busy with write:

static inline int queue_read_trylock(struct qrwlock *lock)
{
        u32 cnts;

        cnts = atomic_read(&lock->cnts);
        if (likely(!(cnts & _QW_WMASK))) {

so the deadlock will not occur.  the show() is opened, write lock is taken, and
if the thread is rescheduled and takes read lock the trylock will return 0, and
the thread will return -EBUSY to userspace avoiding the deadlock.

P.
>