Message-ID: <56B38DC8.8060606@codeaurora.org>
Date:	Thu, 04 Feb 2016 09:43:36 -0800
From:	Saravana Kannan <skannan@...eaurora.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	"Rafael J. Wysocki" <rafael@...nel.org>,
	Shilpa Bhat <shilpabhatppc@...il.com>,
	Juri Lelli <juri.lelli@....com>,
	Rafael Wysocki <rjw@...ysocki.net>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Michael Turquette <mturquette@...libre.com>,
	Steve Muckle <steve.muckle@...aro.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Morten Rasmussen <morten.rasmussen@....com>,
	dietmar.eggemann@....com,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups

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@...aro.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.

-Saravana

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

Powered by blists - more mailing lists