diff mbox

[08/12] cpufreq: governor: synchronize work-handler with governor callbacks

Message ID c8e5037760d45b96e059bf6fd150e2e23205ff76.1434019473.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar June 11, 2015, 10:51 a.m. UTC 
Currently the work-handler's for all the CPUs are synchronized with one
another but not with cpufreq_governor_dbs(). Both work-handlers and
cpufreq_governor_dbs() enqueue work. And these not being in sync is an
open invitation to all kind of races to come in.

So, this patch replaces 'timer_mutex' with 'cdata->mutex' so that both
work-handler and callback are in sync.

Also in update_sampling_rate() we are dropping the mutex while
canceling delayed-work. There is no need to do that, keep lock active
for the entire length of routine as that also enqueues works.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c | 14 +++-----------
 drivers/cpufreq/cpufreq_governor.h |  6 ------
 drivers/cpufreq/cpufreq_ondemand.c | 13 ++++---------
 3 files changed, 7 insertions(+), 26 deletions(-)

Comments

Viresh Kumar June 15, 2015, 8:31 a.m. UTC | #1 
On 15-06-15, 13:53, Preeti U Murthy wrote:
> This patch is not convincing. What are the precise races between
> cpufreq_governor_dbs() and the work handlers ?

The most important problem was restarting of workqueues from the
handler, which can happen at the same time when the governor-callbacks
are in progress..

> It is true that the work handlers can get queued after a governor exit
> due to a race between different callers of cpufreq_governor_dbs() and
> they can dereference freed data structures. But that is about invalid
> interleaving of states which your next patch aims at fixing.
> 
> AFAICT, preventing invalid states from succeeding will avoid this race.
> I don't see the need for another lock here.

Another lock? I am taking exactly the same lock at all places and
actually removing the need of two separate locks.

> > 
> > So, this patch replaces 'timer_mutex' with 'cdata->mutex' so that both
> > work-handler and callback are in sync.
> > 
> > Also in update_sampling_rate() we are dropping the mutex while
> > canceling delayed-work. There is no need to do that, keep lock active
> 
> Why? What is the race that we are fixing by taking the lock here ?

This can also queue work on CPUs.
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 08bb67225b85..aa24aa9a9eb3 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -228,13 +228,12 @@  static void dbs_timer(struct work_struct *work)
 {
 	struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info,
 						 dwork.work);
-	struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs;
-	struct cpufreq_policy *policy = ccdbs->policy;
+	struct cpufreq_policy *policy = cdbs->ccdbs->policy;
 	struct dbs_data *dbs_data = policy->governor_data;
 	unsigned int sampling_rate, delay;
 	bool modify_all = true;
 
-	mutex_lock(&ccdbs->timer_mutex);
+	mutex_lock(&dbs_data->cdata->mutex);
 
 	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
 		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -252,7 +251,7 @@  static void dbs_timer(struct work_struct *work)
 	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all);
 	gov_queue_work(dbs_data, policy, delay, modify_all);
 
-	mutex_unlock(&ccdbs->timer_mutex);
+	mutex_unlock(&dbs_data->cdata->mutex);
 }
 
 static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -424,7 +423,6 @@  static int cpufreq_governor_start(struct cpufreq_policy *policy,
 	}
 
 	ccdbs->time_stamp = ktime_get();
-	mutex_init(&ccdbs->timer_mutex);
 
 	for_each_cpu(j, policy->cpus) {
 		struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
@@ -470,8 +468,6 @@  static void cpufreq_governor_stop(struct cpufreq_policy *policy,
 {
 	struct common_dbs_data *cdata = dbs_data->cdata;
 	unsigned int cpu = policy->cpu;
-	struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu);
-	struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs;
 
 	gov_cancel_work(dbs_data, policy);
 
@@ -481,8 +477,6 @@  static void cpufreq_governor_stop(struct cpufreq_policy *policy,
 
 		cs_dbs_info->enable = 0;
 	}
-
-	mutex_destroy(&ccdbs->timer_mutex);
 }
 
 static void cpufreq_governor_limits(struct cpufreq_policy *policy,
@@ -495,7 +489,6 @@  static void cpufreq_governor_limits(struct cpufreq_policy *policy,
 	if (!cdbs->ccdbs)
 		return;
 
-	mutex_lock(&cdbs->ccdbs->timer_mutex);
 	if (policy->max < cdbs->ccdbs->policy->cur)
 		__cpufreq_driver_target(cdbs->ccdbs->policy, policy->max,
 					CPUFREQ_RELATION_H);
@@ -503,7 +496,6 @@  static void cpufreq_governor_limits(struct cpufreq_policy *policy,
 		__cpufreq_driver_target(cdbs->ccdbs->policy, policy->min,
 					CPUFREQ_RELATION_L);
 	dbs_check_cpu(dbs_data, cpu);
-	mutex_unlock(&cdbs->ccdbs->timer_mutex);
 }
 
 int cpufreq_governor_dbs(struct cpufreq_policy *policy,
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 2125c299c602..2b2884f91fe7 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -131,12 +131,6 @@  static void *get_cpu_dbs_info_s(int cpu)				\
 /* Common to all CPUs of a policy */
 struct cpu_common_dbs_info {
 	struct cpufreq_policy *policy;
-	/*
-	 * percpu mutex that serializes governor limit change with dbs_timer
-	 * invocation. We do not want dbs_timer to run when user is changing
-	 * the governor or limits.
-	 */
-	struct mutex timer_mutex;
 	ktime_t time_stamp;
 };
 
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 11db20079fc6..87813830e169 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -249,6 +249,8 @@  static void update_sampling_rate(struct dbs_data *dbs_data,
 	struct od_dbs_tuners *od_tuners = dbs_data->tuners;
 	int cpu;
 
+	mutex_lock(&dbs_data->cdata->mutex);
+
 	od_tuners->sampling_rate = new_rate = max(new_rate,
 			dbs_data->min_sampling_rate);
 
@@ -267,28 +269,21 @@  static void update_sampling_rate(struct dbs_data *dbs_data,
 		dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
 		cpufreq_cpu_put(policy);
 
-		mutex_lock(&dbs_info->cdbs.ccdbs->timer_mutex);
-
-		if (!delayed_work_pending(&dbs_info->cdbs.dwork)) {
-			mutex_unlock(&dbs_info->cdbs.ccdbs->timer_mutex);
+		if (!delayed_work_pending(&dbs_info->cdbs.dwork))
 			continue;
-		}
 
 		next_sampling = jiffies + usecs_to_jiffies(new_rate);
 		appointed_at = dbs_info->cdbs.dwork.timer.expires;
 
 		if (time_before(next_sampling, appointed_at)) {
 
-			mutex_unlock(&dbs_info->cdbs.ccdbs->timer_mutex);
 			cancel_delayed_work_sync(&dbs_info->cdbs.dwork);
-			mutex_lock(&dbs_info->cdbs.ccdbs->timer_mutex);
-
 			gov_queue_work(dbs_data, policy,
 				       usecs_to_jiffies(new_rate), true);
 
 		}
-		mutex_unlock(&dbs_info->cdbs.ccdbs->timer_mutex);
 	}
+	mutex_unlock(&dbs_data->cdata->mutex);
 }
 
 static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,