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);