Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups

[Saravana Kannan <skannan@codeaurora.org>]

On 02/04/2016 09:43 AM, Saravana Kannan wrote:
> On 02/04/2016 03:09 AM, Viresh Kumar wrote:
>> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
>>> This is exactly right.  We've avoided one deadlock only to trip into
>>> another one.
>>>
>>> This happens because update_sampling_rate() acquires
>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
>>> cpufreq_governor_dbs().
>>>
>>> Worse yet, a deadlock can still happen without (the new)
>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
>>> update_sampling_rate() runs in parallel with
>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
>>> the race.
>>>
>>> It looks like we need to drop the governor mutex before putting the
>>> kobject in cpufreq_governor_exit().
>>
>> I have tried to explore all possible ways of fixing this, and every
>> other way looked to be racy in some way.
>>
>> Does anyone else have a better idea (untested):
>>
>> -------------------------8<-------------------------
>>
>> Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a
>> separate
>>   work
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   drivers/cpufreq/cpufreq_governor.h |  2 ++
>>   drivers/cpufreq/cpufreq_ondemand.c | 39
>> +++++++++++++++++++++++++++++---------
>>   2 files changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_governor.h
>> b/drivers/cpufreq/cpufreq_governor.h
>> index 7bed63e14e7d..97e604356b20 100644
>> --- a/drivers/cpufreq/cpufreq_governor.h
>> +++ b/drivers/cpufreq/cpufreq_governor.h
>> @@ -141,6 +141,8 @@ struct od_dbs_tuners {
>>       unsigned int powersave_bias;
>>       unsigned int io_is_busy;
>>       unsigned int min_sampling_rate;
>> +    struct work_struct work;
>> +    struct dbs_data *dbs_data;
>>   };
>>
>>   struct cs_dbs_tuners {
>> diff --git a/drivers/cpufreq/cpufreq_ondemand.c
>> b/drivers/cpufreq/cpufreq_ondemand.c
>> index 82ed490f7de0..93ad7a226aee 100644
>> --- a/drivers/cpufreq/cpufreq_ondemand.c
>> +++ b/drivers/cpufreq/cpufreq_ondemand.c
>> @@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
>>    * reducing the sampling rate, we need to make the new value effective
>>    * immediately.
>>    */
>> -static void update_sampling_rate(struct dbs_data *dbs_data,
>> -        unsigned int new_rate)
>> +static void update_sampling_rate(struct work_struct *work)
>>   {
>> -    struct od_dbs_tuners *od_tuners = dbs_data->tuners;
>> +    struct od_dbs_tuners *od_tuners = container_of(work, struct
>> +                               od_dbs_tuners, work);
>> +    unsigned int new_rate = od_tuners->sampling_rate;
>> +    struct dbs_data *dbs_data = od_tuners->dbs_data;
>>       struct cpumask cpumask;
>>       int cpu;
>>
>> -    od_tuners->sampling_rate = new_rate = max(new_rate,
>> -            od_tuners->min_sampling_rate);
>> -
>>       /*
>>        * Lock governor so that governor start/stop can't execute in
>> parallel.
>> +     *
>> +     * We can't do a regular mutex_lock() here, as that may deadlock
>> against
>> +     * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
>> +     * governor, which might have already taken od_dbs_cdata.mutex
>> and is
>> +     * waiting for this work to finish.
>>        */
>> -    mutex_lock(&od_dbs_cdata.mutex);
>> +    if (!mutex_trylock(&od_dbs_cdata.mutex)) {
>> +        queue_work(system_wq, &od_tuners->work);
>> +        return;
>> +    }
>>
>>       cpumask_copy(&cpumask, cpu_online_mask);
>>
>> @@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data
>> *dbs_data,
>>   static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const
>> char *buf,
>>           size_t count)
>>   {
>> +    struct od_dbs_tuners *od_tuners = dbs_data->tuners;
>>       unsigned int input;
>>       int ret;
>>       ret = sscanf(buf, "%u", &input);
>>       if (ret != 1)
>>           return -EINVAL;
>>
>> -    update_sampling_rate(dbs_data, input);
>> +    od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
>> +
>> +    /*
>> +     * update_sampling_rate() requires to hold od_dbs_cdata.mutex,
>> but we
>> +     * can't take that from this thread, otherwise it results in ABBA
>> +     * lockdep between s_active and od_dbs_cdata.mutex locks.
>> +     */
>> +    queue_work(system_wq, &od_tuners->work);
>> +
>>       return count;
>>   }
>>
>> @@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool
>> notify)
>>       tuners->ignore_nice_load = 0;
>>       tuners->powersave_bias = default_powersave_bias;
>>       tuners->io_is_busy = should_io_be_busy();
>> +    INIT_WORK(&tuners->work, update_sampling_rate);
>> +    tuners->dbs_data = dbs_data;
>>
>>       dbs_data->tuners = tuners;
>>       return 0;
>> @@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data,
>> bool notify)
>>
>>   static void od_exit(struct dbs_data *dbs_data, bool notify)
>>   {
>> -    kfree(dbs_data->tuners);
>> +    struct od_dbs_tuners *tuners = dbs_data->tuners;
>> +
>> +    cancel_work_sync(&tuners->work);
>> +    kfree(tuners);
>>   }
>>
>>   define_get_cpu_dbs_routines(od_cpu_dbs_info);
>>
>
> No no no no! Let's not open up this can of worms of queuing up the work
> to handle a write to a sysfs file. It *MIGHT* work for this specific
> tunable (I haven't bothered to analyze), but this makes it impossible to
> return a useful/proper error value.

Sent too soon. Not only that, but it can also cause the writes to the 
sysfs files to get processed in a different order and I don't know what 
other issues/races THAT will open up.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project