[patch 0/4] Take care of cpufreq lockdep issues (take 2)

[patch 0/4] Take care of cpufreq lockdep issues (take 2)

[venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w]

Since recent chanegs to ondemand and conservative governor, there have been
multiple reports of lockdep issues in cpufreq. Patch series takes care of
these problems.

This is the next attempt following the one here, which was not a complete fix.
http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html

I am currently running some stress tests to make sure there are no issues with
these patches. But, wanted to send them out for review/comments/testing
before I head out for the long weekend.

If this patchset seems sane, the first patch in the patchset should also get
into 30.stable.

-- 

[patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w]

[-- Attachment #1: 0001-cpufreq-Eliminate-the-recent-lockdep-warnings-in-cp.patch --]
[-- Type: text/plain, Size: 6243 bytes --]

Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
much needed to properly cleanup ondemand timer, opened-up a can of worms
related to locking dependencies in cpufreq.

Patch here defines the need for dbs_mutex and cleans up its usage in
ondemand governor. This also resolves the lockdep warnings reported here

http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html

and few others..

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
 drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
 3 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6e2ec0b..c7fe16e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 #endif
 
-	unlock_policy_rwsem_write(cpu);
-
 	if (cpufreq_driver->target)
 		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
 
@@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(data);
 
+	unlock_policy_rwsem_write(cpu);
+
 	free_cpumask_var(data->related_cpus);
 	free_cpumask_var(data->cpus);
 	kfree(data);
diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 7fc58af..58889f2 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
 static unsigned int dbs_enable;	/* number of CPUs using this policy */
 
 /*
- * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take cpu_hotplug lock
- * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, then
- * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, because it
- * would deadlock with cancel_delayed_work_sync(), which is needed for proper
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
+ * different CPUs. It protects dbs_enable in governor start/stop. It also
+ * serializes governor limit_change with do_dbs_timer. We do not want
+ * do_dbs_timer to run when user is changing the governor or limits.
  */
 static DEFINE_MUTEX(dbs_mutex);
 
@@ -488,18 +483,17 @@ static void do_dbs_timer(struct work_struct *work)
 
 	delay -= jiffies % delay;
 
-	if (lock_policy_rwsem_write(cpu) < 0)
-		return;
+	mutex_lock(&dbs_mutex);
 
 	if (!dbs_info->enable) {
-		unlock_policy_rwsem_write(cpu);
+		mutex_unlock(&dbs_mutex);
 		return;
 	}
 
 	dbs_check_cpu(dbs_info);
 
 	queue_delayed_work_on(cpu, kconservative_wq, &dbs_info->work, delay);
-	unlock_policy_rwsem_write(cpu);
+	mutex_unlock(&dbs_mutex);
 }
 
 static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
@@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 					&dbs_cpufreq_notifier_block,
 					CPUFREQ_TRANSITION_NOTIFIER);
 		}
-		dbs_timer_init(this_dbs_info);
-
 		mutex_unlock(&dbs_mutex);
 
+		dbs_timer_init(this_dbs_info);
+
 		break;
 
 	case CPUFREQ_GOV_STOP:
-		mutex_lock(&dbs_mutex);
 		dbs_timer_exit(this_dbs_info);
+
+		mutex_lock(&dbs_mutex);
 		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
 		dbs_enable--;
 
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 1911d17..246ae14 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
 static unsigned int dbs_enable;	/* number of CPUs using this policy */
 
 /*
- * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take cpu_hotplug lock
- * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, then
- * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, because it
- * would deadlock with cancel_delayed_work_sync(), which is needed for proper
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
+ * different CPUs. It protects dbs_enable in governor start/stop. It also
+ * serializes governor limit_change with do_dbs_timer. We do not want
+ * do_dbs_timer to run when user is changing the governor or limits.
  */
 static DEFINE_MUTEX(dbs_mutex);
 
@@ -494,11 +489,10 @@ static void do_dbs_timer(struct work_struct *work)
 
 	delay -= jiffies % delay;
 
-	if (lock_policy_rwsem_write(cpu) < 0)
-		return;
+	mutex_lock(&dbs_mutex);
 
 	if (!dbs_info->enable) {
-		unlock_policy_rwsem_write(cpu);
+		mutex_unlock(&dbs_mutex);
 		return;
 	}
 
@@ -517,7 +511,7 @@ static void do_dbs_timer(struct work_struct *work)
 			dbs_info->freq_lo, CPUFREQ_RELATION_H);
 	}
 	queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work, delay);
-	unlock_policy_rwsem_write(cpu);
+	mutex_unlock(&dbs_mutex);
 }
 
 static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
@@ -598,14 +592,15 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 				max(min_sampling_rate,
 				    latency * LATENCY_MULTIPLIER);
 		}
-		dbs_timer_init(this_dbs_info);
-
 		mutex_unlock(&dbs_mutex);
+
+		dbs_timer_init(this_dbs_info);
 		break;
 
 	case CPUFREQ_GOV_STOP:
-		mutex_lock(&dbs_mutex);
 		dbs_timer_exit(this_dbs_info);
+
+		mutex_lock(&dbs_mutex);
 		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
 		dbs_enable--;
 		mutex_unlock(&dbs_mutex);
-- 
1.6.0.6

-- 

[patch 2/4] cpufreq: Mark policy_rwsem as going static in cpufreq.c wont be exported

[venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w]

[-- Attachment #1: 0002-cpufreq-Mark-policy_rwsem-as-going-static-in-cpufre.patch --]
[-- Type: text/plain, Size: 1529 bytes --]

lock_policy_rwsem_* and unlock_policy_rwsem_* routines in cpufreq.c are
currently exported to drivers. Improper use of those locks can
result in deadlocks and it is better to keep the locks localized.
Two previous in-kernel users of these interfaces (ondemand and conservative),
do not use this interfaces any more. Schedule them for removal. 

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/feature-removal-schedule.txt |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index f8cd450..09e031c 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -458,3 +458,13 @@ Why:	Remove the old legacy 32bit machine check code. This has been
 	but the old version has been kept around for easier testing. Note this
 	doesn't impact the old P5 and WinChip machine check handlers.
 Who:	Andi Kleen <andi-Vw/NltI1exuRpAAqCnN02g@public.gmane.org>
+
+----------------------------
+
+What:	lock_policy_rwsem_* and unlock_policy_rwsem_* will not be
+	exported interface anymore.
+When:	2.6.33
+Why:	cpu_policy_rwsem has a new cleaner definition making it local to
+	cpufreq core and contained inside cpufreq.c. Other dependent
+	drivers should not use it in order to safely avoid lockdep issues.
+Who:	Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
-- 
1.6.0.6

-- 

[patch 3/4] cpufreq: Cleanup locking in ondemand governor

[venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w]

[-- Attachment #1: 0003-cpufreq-Cleanup-locking-in-ondemand-governor.patch --]
[-- Type: text/plain, Size: 6242 bytes --]

Redesign the locking inside ondemand driver. Make dbs_mutex handle all the
global state changes inside the driver and invent a new percpu mutex
to serialize percpu timer and frequency limit change.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/cpufreq/cpufreq_ondemand.c |   62 +++++++++++++++--------------------
 1 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 246ae14..d6ba142 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -70,8 +70,13 @@ struct cpu_dbs_info_s {
 	unsigned int freq_lo_jiffies;
 	unsigned int freq_hi_jiffies;
 	int cpu;
-	unsigned int enable:1,
-		sample_type:1;
+	unsigned int sample_type:1;
+	/*
+	 * percpu mutex that serializes governor limit change with
+	 * do_dbs_timer invocation. We do not want do_dbs_timer to run
+	 * when user is changing the governor or limits.
+	 */
+	struct mutex timer_mutex;
 };
 static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
 
@@ -79,9 +84,7 @@ static unsigned int dbs_enable;	/* number of CPUs using this policy */
 
 /*
  * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
- * different CPUs. It protects dbs_enable in governor start/stop. It also
- * serializes governor limit_change with do_dbs_timer. We do not want
- * do_dbs_timer to run when user is changing the governor or limits.
+ * different CPUs. It protects dbs_enable in governor start/stop.
  */
 static DEFINE_MUTEX(dbs_mutex);
 
@@ -187,13 +190,18 @@ static unsigned int powersave_bias_target(struct cpufreq_policy *policy,
 	return freq_hi;
 }
 
+static void ondemand_powersave_bias_init_cpu(int cpu)
+{
+	struct cpu_dbs_info_s *dbs_info = &per_cpu(cpu_dbs_info, cpu);
+	dbs_info->freq_table = cpufreq_frequency_get_table(cpu);
+	dbs_info->freq_lo = 0;
+}
+
 static void ondemand_powersave_bias_init(void)
 {
 	int i;
 	for_each_online_cpu(i) {
-		struct cpu_dbs_info_s *dbs_info = &per_cpu(cpu_dbs_info, i);
-		dbs_info->freq_table = cpufreq_frequency_get_table(i);
-		dbs_info->freq_lo = 0;
+		ondemand_powersave_bias_init_cpu(i);
 	}
 }
 
@@ -235,12 +243,10 @@ static ssize_t store_sampling_rate(struct cpufreq_policy *unused,
 	unsigned int input;
 	int ret;
 	ret = sscanf(buf, "%u", &input);
+	if (ret != 1)
+		return -EINVAL;
 
 	mutex_lock(&dbs_mutex);
-	if (ret != 1) {
-		mutex_unlock(&dbs_mutex);
-		return -EINVAL;
-	}
 	dbs_tuners_ins.sampling_rate = max(input, min_sampling_rate);
 	mutex_unlock(&dbs_mutex);
 
@@ -254,13 +260,12 @@ static ssize_t store_up_threshold(struct cpufreq_policy *unused,
 	int ret;
 	ret = sscanf(buf, "%u", &input);
 
-	mutex_lock(&dbs_mutex);
 	if (ret != 1 || input > MAX_FREQUENCY_UP_THRESHOLD ||
 			input < MIN_FREQUENCY_UP_THRESHOLD) {
-		mutex_unlock(&dbs_mutex);
 		return -EINVAL;
 	}
 
+	mutex_lock(&dbs_mutex);
 	dbs_tuners_ins.up_threshold = input;
 	mutex_unlock(&dbs_mutex);
 
@@ -358,9 +363,6 @@ static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
 	struct cpufreq_policy *policy;
 	unsigned int j;
 
-	if (!this_dbs_info->enable)
-		return;
-
 	this_dbs_info->freq_lo = 0;
 	policy = this_dbs_info->cur_policy;
 
@@ -488,13 +490,7 @@ static void do_dbs_timer(struct work_struct *work)
 	int delay = usecs_to_jiffies(dbs_tuners_ins.sampling_rate);
 
 	delay -= jiffies % delay;
-
-	mutex_lock(&dbs_mutex);
-
-	if (!dbs_info->enable) {
-		mutex_unlock(&dbs_mutex);
-		return;
-	}
+	mutex_lock(&dbs_info->timer_mutex);
 
 	/* Common NORMAL_SAMPLE setup */
 	dbs_info->sample_type = DBS_NORMAL_SAMPLE;
@@ -511,7 +507,7 @@ static void do_dbs_timer(struct work_struct *work)
 			dbs_info->freq_lo, CPUFREQ_RELATION_H);
 	}
 	queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work, delay);
-	mutex_unlock(&dbs_mutex);
+	mutex_unlock(&dbs_info->timer_mutex);
 }
 
 static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
@@ -520,8 +516,6 @@ static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
 	int delay = usecs_to_jiffies(dbs_tuners_ins.sampling_rate);
 	delay -= jiffies % delay;
 
-	dbs_info->enable = 1;
-	ondemand_powersave_bias_init();
 	dbs_info->sample_type = DBS_NORMAL_SAMPLE;
 	INIT_DELAYED_WORK_DEFERRABLE(&dbs_info->work, do_dbs_timer);
 	queue_delayed_work_on(dbs_info->cpu, kondemand_wq, &dbs_info->work,
@@ -530,7 +524,6 @@ static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
 
 static inline void dbs_timer_exit(struct cpu_dbs_info_s *dbs_info)
 {
-	dbs_info->enable = 0;
 	cancel_delayed_work_sync(&dbs_info->work);
 }
 
@@ -549,19 +542,15 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		if ((!cpu_online(cpu)) || (!policy->cur))
 			return -EINVAL;
 
-		if (this_dbs_info->enable) /* Already enabled */
-			break;
-
 		mutex_lock(&dbs_mutex);
-		dbs_enable++;
 
 		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);
 		if (rc) {
-			dbs_enable--;
 			mutex_unlock(&dbs_mutex);
 			return rc;
 		}
 
+		dbs_enable++;
 		for_each_cpu(j, policy->cpus) {
 			struct cpu_dbs_info_s *j_dbs_info;
 			j_dbs_info = &per_cpu(cpu_dbs_info, j);
@@ -575,6 +564,8 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 			}
 		}
 		this_dbs_info->cpu = cpu;
+		ondemand_powersave_bias_init_cpu(cpu);
+		mutex_init(&this_dbs_info->timer_mutex);
 		/*
 		 * Start the timerschedule work, when this governor
 		 * is used for first time
@@ -602,20 +593,21 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 
 		mutex_lock(&dbs_mutex);
 		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
+		mutex_destroy(&this_dbs_info->timer_mutex);
 		dbs_enable--;
 		mutex_unlock(&dbs_mutex);
 
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		mutex_lock(&dbs_mutex);
+		mutex_lock(&this_dbs_info->timer_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(this_dbs_info->cur_policy,
 				policy->max, CPUFREQ_RELATION_H);
 		else if (policy->min > this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(this_dbs_info->cur_policy,
 				policy->min, CPUFREQ_RELATION_L);
-		mutex_unlock(&dbs_mutex);
+		mutex_unlock(&this_dbs_info->timer_mutex);
 		break;
 	}
 	return 0;
-- 
1.6.0.6

-- 

[patch 4/4] cpufreq: Cleanup locking in conservative governor

[venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w]

[-- Attachment #1: 0004-cpufreq-Cleanup-locking-in-conservative-governor.patch --]
[-- Type: text/plain, Size: 4330 bytes --]

Redesign the locking inside conservative driver. Make dbs_mutex handle all the
global state changes inside the driver and invent a new percpu mutex
to serialize percpu timer and frequency limit change.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/cpufreq/cpufreq_conservative.c |   34 ++++++++++++-------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 58889f2..5749050 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -63,7 +63,12 @@ struct cpu_dbs_info_s {
 	unsigned int down_skip;
 	unsigned int requested_freq;
 	int cpu;
-	unsigned int enable:1;
+	/*
+	 * percpu mutex that serializes governor limit change with
+	 * do_dbs_timer invocation. We do not want do_dbs_timer to run
+	 * when user is changing the governor or limits.
+	 */
+	struct mutex timer_mutex;
 };
 static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
 
@@ -71,9 +76,7 @@ static unsigned int dbs_enable;	/* number of CPUs using this policy */
 
 /*
  * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
- * different CPUs. It protects dbs_enable in governor start/stop. It also
- * serializes governor limit_change with do_dbs_timer. We do not want
- * do_dbs_timer to run when user is changing the governor or limits.
+ * different CPUs. It protects dbs_enable in governor start/stop.
  */
 static DEFINE_MUTEX(dbs_mutex);
 
@@ -138,9 +141,6 @@ dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 
 	struct cpufreq_policy *policy;
 
-	if (!this_dbs_info->enable)
-		return 0;
-
 	policy = this_dbs_info->cur_policy;
 
 	/*
@@ -483,17 +483,12 @@ static void do_dbs_timer(struct work_struct *work)
 
 	delay -= jiffies % delay;
 
-	mutex_lock(&dbs_mutex);
-
-	if (!dbs_info->enable) {
-		mutex_unlock(&dbs_mutex);
-		return;
-	}
+	mutex_lock(&dbs_info->timer_mutex);
 
 	dbs_check_cpu(dbs_info);
 
 	queue_delayed_work_on(cpu, kconservative_wq, &dbs_info->work, delay);
-	mutex_unlock(&dbs_mutex);
+	mutex_unlock(&dbs_info->timer_mutex);
 }
 
 static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
@@ -502,7 +497,6 @@ static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
 	int delay = usecs_to_jiffies(dbs_tuners_ins.sampling_rate);
 	delay -= jiffies % delay;
 
-	dbs_info->enable = 1;
 	INIT_DELAYED_WORK_DEFERRABLE(&dbs_info->work, do_dbs_timer);
 	queue_delayed_work_on(dbs_info->cpu, kconservative_wq, &dbs_info->work,
 				delay);
@@ -510,7 +504,6 @@ static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
 
 static inline void dbs_timer_exit(struct cpu_dbs_info_s *dbs_info)
 {
-	dbs_info->enable = 0;
 	cancel_delayed_work_sync(&dbs_info->work);
 }
 
@@ -529,9 +522,6 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		if ((!cpu_online(cpu)) || (!policy->cur))
 			return -EINVAL;
 
-		if (this_dbs_info->enable) /* Already enabled */
-			break;
-
 		mutex_lock(&dbs_mutex);
 
 		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);
@@ -555,6 +545,7 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		this_dbs_info->down_skip = 0;
 		this_dbs_info->requested_freq = policy->cur;
 
+		mutex_init(&this_dbs_info->timer_mutex);
 		dbs_enable++;
 		/*
 		 * Start the timerschedule work, when this governor
@@ -596,6 +587,7 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		mutex_lock(&dbs_mutex);
 		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
 		dbs_enable--;
+		mutex_destroy(&this_dbs_info->timer_mutex);
 
 		/*
 		 * Stop the timerschedule work, when this governor
@@ -611,7 +603,7 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		mutex_lock(&dbs_mutex);
+		mutex_lock(&this_dbs_info->timer_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(
 					this_dbs_info->cur_policy,
@@ -620,7 +612,7 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 			__cpufreq_driver_target(
 					this_dbs_info->cur_policy,
 					policy->min, CPUFREQ_RELATION_L);
-		mutex_unlock(&dbs_mutex);
+		mutex_unlock(&this_dbs_info->timer_mutex);
 
 		break;
 	}
-- 
1.6.0.6

-- 

Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Mathieu Desnoyers]

* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org (venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org) wrote:
> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
> much needed to properly cleanup ondemand timer, opened-up a can of worms
> related to locking dependencies in cpufreq.
> 
> Patch here defines the need for dbs_mutex and cleans up its usage in
> ondemand governor. This also resolves the lockdep warnings reported here
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
> 
> and few others..
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/cpufreq/cpufreq.c              |    4 ++--
>  drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
>  drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
>  3 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6e2ec0b..c7fe16e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
>  	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  #endif
>  
> -	unlock_policy_rwsem_write(cpu);
> -
>  	if (cpufreq_driver->target)
>  		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
>  
> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
>  	if (cpufreq_driver->exit)
>  		cpufreq_driver->exit(data);
>  
> +	unlock_policy_rwsem_write(cpu);
> +
>  	free_cpumask_var(data->related_cpus);
>  	free_cpumask_var(data->cpus);
>  	kfree(data);
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 7fc58af..58889f2 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
>  static unsigned int dbs_enable;	/* number of CPUs using this policy */
>  
>  /*
> - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> - * dbs_mutex. If any function that can potentially take cpu_hotplug lock
> - * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, then
> - * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
> - * is recursive for the same process. -Venki
> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, because it
> - * would deadlock with cancel_delayed_work_sync(), which is needed for proper
> - * raceless workqueue teardown.
> + * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
> + * different CPUs. It protects dbs_enable in governor start/stop. It also
> + * serializes governor limit_change with do_dbs_timer. We do not want
> + * do_dbs_timer to run when user is changing the governor or limits.
>   */
>  static DEFINE_MUTEX(dbs_mutex);
>  
> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct work_struct *work)
>  
>  	delay -= jiffies % delay;
>  
> -	if (lock_policy_rwsem_write(cpu) < 0)
> -		return;
> +	mutex_lock(&dbs_mutex);

OK, I now have absolutely no idea what the rwsem mutex is protecting
anymore.

You should probably describe the new world order not just in terms of
what the dbs_mutex is protecting, but also about what the rwsem is
doing. I'm worried that this rwsem is there to protect against more than
what is protected by the dbs_mutex local to the ondemand/conservative
governors.

See below,

>  
>  	if (!dbs_info->enable) {
> -		unlock_policy_rwsem_write(cpu);
> +		mutex_unlock(&dbs_mutex);
>  		return;
>  	}
>  
>  	dbs_check_cpu(dbs_info);
>  
>  	queue_delayed_work_on(cpu, kconservative_wq, &dbs_info->work, delay);
> -	unlock_policy_rwsem_write(cpu);
> +	mutex_unlock(&dbs_mutex);
>  }
>  
>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  					&dbs_cpufreq_notifier_block,
>  					CPUFREQ_TRANSITION_NOTIFIER);
>  		}
> -		dbs_timer_init(this_dbs_info);
> -
>  		mutex_unlock(&dbs_mutex);
>  
> +		dbs_timer_init(this_dbs_info);
> +
>  		break;
>  
>  	case CPUFREQ_GOV_STOP:
> -		mutex_lock(&dbs_mutex);
>  		dbs_timer_exit(this_dbs_info);

So now the only thing that seems to prevent the init and exit to race
with each other is the rwsem. But this does not seem to be described
anywhere.

Mathieu

> +
> +		mutex_lock(&dbs_mutex);
>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>  		dbs_enable--;
>  
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 1911d17..246ae14 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);
>  static unsigned int dbs_enable;	/* number of CPUs using this policy */
>  
>  /*
> - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> - * dbs_mutex. If any function that can potentially take cpu_hotplug lock
> - * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, then
> - * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
> - * is recursive for the same process. -Venki
> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, because it
> - * would deadlock with cancel_delayed_work_sync(), which is needed for proper
> - * raceless workqueue teardown.
> + * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
> + * different CPUs. It protects dbs_enable in governor start/stop. It also
> + * serializes governor limit_change with do_dbs_timer. We do not want
> + * do_dbs_timer to run when user is changing the governor or limits.
>   */
>  static DEFINE_MUTEX(dbs_mutex);
>  
> @@ -494,11 +489,10 @@ static void do_dbs_timer(struct work_struct *work)
>  
>  	delay -= jiffies % delay;
>  
> -	if (lock_policy_rwsem_write(cpu) < 0)
> -		return;
> +	mutex_lock(&dbs_mutex);
>  
>  	if (!dbs_info->enable) {
> -		unlock_policy_rwsem_write(cpu);
> +		mutex_unlock(&dbs_mutex);
>  		return;
>  	}
>  
> @@ -517,7 +511,7 @@ static void do_dbs_timer(struct work_struct *work)
>  			dbs_info->freq_lo, CPUFREQ_RELATION_H);
>  	}
>  	queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work, delay);
> -	unlock_policy_rwsem_write(cpu);
> +	mutex_unlock(&dbs_mutex);
>  }
>  
>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> @@ -598,14 +592,15 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  				max(min_sampling_rate,
>  				    latency * LATENCY_MULTIPLIER);
>  		}
> -		dbs_timer_init(this_dbs_info);
> -
>  		mutex_unlock(&dbs_mutex);
> +
> +		dbs_timer_init(this_dbs_info);
>  		break;
>  
>  	case CPUFREQ_GOV_STOP:
> -		mutex_lock(&dbs_mutex);
>  		dbs_timer_exit(this_dbs_info);
> +
> +		mutex_lock(&dbs_mutex);
>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>  		dbs_enable--;
>  		mutex_unlock(&dbs_mutex);
> -- 
> 1.6.0.6
> 
> -- 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

RE: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Mathieu Desnoyers [mailto:mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org] 
>Sent: Thursday, July 02, 2009 6:07 PM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Ingo 
>Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Thomas Renninger
>Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep 
>warnings in cpufreq
>
>* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org (venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org) wrote:
>> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
>> much needed to properly cleanup ondemand timer, opened-up a 
>can of worms
>> related to locking dependencies in cpufreq.
>> 
>> Patch here defines the need for dbs_mutex and cleans up its usage in
>> ondemand governor. This also resolves the lockdep warnings 
>reported here
>> 
>> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
>> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
>> 
>> and few others..
>> 
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/cpufreq/cpufreq.c              |    4 ++--
>>  drivers/cpufreq/cpufreq_conservative.c |   27 
>+++++++++++----------------
>>  drivers/cpufreq/cpufreq_ondemand.c     |   27 
>+++++++++++----------------
>>  3 files changed, 24 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 6e2ec0b..c7fe16e 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct 
>sys_device *sys_dev)
>>  	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>  #endif
>>  
>> -	unlock_policy_rwsem_write(cpu);
>> -
>>  	if (cpufreq_driver->target)
>>  		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
>>  
>> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct 
>sys_device *sys_dev)
>>  	if (cpufreq_driver->exit)
>>  		cpufreq_driver->exit(data);
>>  
>> +	unlock_policy_rwsem_write(cpu);
>> +
>>  	free_cpumask_var(data->related_cpus);
>>  	free_cpumask_var(data->cpus);
>>  	kfree(data);
>> diff --git a/drivers/cpufreq/cpufreq_conservative.c 
>b/drivers/cpufreq/cpufreq_conservative.c
>> index 7fc58af..58889f2 100644
>> --- a/drivers/cpufreq/cpufreq_conservative.c
>> +++ b/drivers/cpufreq/cpufreq_conservative.c
>> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct 
>cpu_dbs_info_s, cpu_dbs_info);
>>  static unsigned int dbs_enable;	/* number of CPUs using 
>this policy */
>>  
>>  /*
>> - * DEADLOCK ALERT! There is a ordering requirement between 
>cpu_hotplug
>> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
>> - * dbs_mutex. If any function that can potentially take 
>cpu_hotplug lock
>> - * (like __cpufreq_driver_target()) is being called with 
>dbs_mutex taken, then
>> - * cpu_hotplug lock should be taken before that. Note that 
>cpu_hotplug lock
>> - * is recursive for the same process. -Venki
>> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the 
>dbs_mutex, because it
>> - * would deadlock with cancel_delayed_work_sync(), which is 
>needed for proper
>> - * raceless workqueue teardown.
>> + * dbs_mutex protects data in dbs_tuners_ins from 
>concurrent changes on
>> + * different CPUs. It protects dbs_enable in governor 
>start/stop. It also
>> + * serializes governor limit_change with do_dbs_timer. We 
>do not want
>> + * do_dbs_timer to run when user is changing the governor or limits.
>>   */
>>  static DEFINE_MUTEX(dbs_mutex);
>>  
>> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct 
>work_struct *work)
>>  
>>  	delay -= jiffies % delay;
>>  
>> -	if (lock_policy_rwsem_write(cpu) < 0)
>> -		return;
>> +	mutex_lock(&dbs_mutex);
>
>OK, I now have absolutely no idea what the rwsem mutex is protecting
>anymore.
>
>You should probably describe the new world order not just in terms of
>what the dbs_mutex is protecting, but also about what the rwsem is
>doing. I'm worried that this rwsem is there to protect against 
>more than
>what is protected by the dbs_mutex local to the ondemand/conservative
>governors.
>
>See below,
>
>>  
>>  	if (!dbs_info->enable) {
>> -		unlock_policy_rwsem_write(cpu);
>> +		mutex_unlock(&dbs_mutex);
>>  		return;
>>  	}
>>  
>>  	dbs_check_cpu(dbs_info);
>>  
>>  	queue_delayed_work_on(cpu, kconservative_wq, 
>&dbs_info->work, delay);
>> -	unlock_policy_rwsem_write(cpu);
>> +	mutex_unlock(&dbs_mutex);
>>  }
>>  
>>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
>> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct 
>cpufreq_policy *policy,
>>  					&dbs_cpufreq_notifier_block,
>>  					CPUFREQ_TRANSITION_NOTIFIER);
>>  		}
>> -		dbs_timer_init(this_dbs_info);
>> -
>>  		mutex_unlock(&dbs_mutex);
>>  
>> +		dbs_timer_init(this_dbs_info);
>> +
>>  		break;
>>  
>>  	case CPUFREQ_GOV_STOP:
>> -		mutex_lock(&dbs_mutex);
>>  		dbs_timer_exit(this_dbs_info);
>
>So now the only thing that seems to prevent the init and exit to race
>with each other is the rwsem. But this does not seem to be described
>anywhere.

Mathieu,

Yes. rwsem in cpufreq core makes sure that START and STOP happen sequentially. There
Is no way for START and STOP for a CPU to happen at the same time as cpufreq core holds
per policy rwsem lock before making any change to the policy. I can add a comment to
that effect in cpufreq.c. This is a clean seperation across cpufreq core and governor,
as cpufreq core takes care of all the policy changes. With that, do you see any
Issues/races with this patchset?

Thanks,
Venki--
To unsubscribe from this list: send the line "unsubscribe kernel-testers" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] CPUFREQ: fix (utter) cpufreq_add_dev mess v1

[Mathieu Desnoyers]

(Can you give this patch a try and review my locking fixes ? I'm
uncomfortable modifying cpufreq locking before getting this fix correct
in the first place)

OK, I've tried to clean it up the best I could, but please test this with
concurrent cpu hotplug and cpufreq add/remove in loops. I'm sure we will make
other interesting findings.

This is step one of fixing the overall cpufreq locking dependency mess.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org>
CC: "Pallipadi, Venkatesh" <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
CC: Dave Jones <davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
CC: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
CC: "Rafael J.  Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
CC: Dave Young <hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: Pekka Enberg <penberg-bbCR+/B0CizivPeTLB3BmA@public.gmane.org>
CC: Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c |   65 ++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 25 deletions(-)

Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c	2009-07-02 20:47:19.000000000 -0400
+++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c	2009-07-02 21:52:56.000000000 -0400
@@ -763,6 +763,10 @@ static struct kobj_type ktype_cpufreq = 
  * cpufreq_add_dev - add a CPU device
  *
  * Adds the cpufreq interface for a CPU device.
+ *
+ * The Oracle says: try running cpufreq registration/unregistration concurrently
+ * with with cpu hotplugging and all hell will break loose. Tried to clean this
+ * mess up, but more thorough testing is needed. - Mathieu
  */
 static int cpufreq_add_dev(struct sys_device *sys_dev)
 {
@@ -806,15 +810,12 @@ static int cpufreq_add_dev(struct sys_de
 		goto nomem_out;
 	}
 	if (!alloc_cpumask_var(&policy->cpus, GFP_KERNEL)) {
-		kfree(policy);
 		ret = -ENOMEM;
-		goto nomem_out;
+		goto err_free_policy;
 	}
 	if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL)) {
-		free_cpumask_var(policy->cpus);
-		kfree(policy);
 		ret = -ENOMEM;
-		goto nomem_out;
+		goto err_free_cpumask;
 	}
 
 	policy->cpu = cpu;
@@ -822,7 +823,8 @@ static int cpufreq_add_dev(struct sys_de
 
 	/* Initially set CPU itself as the policy_cpu */
 	per_cpu(policy_cpu, cpu) = cpu;
-	lock_policy_rwsem_write(cpu);
+	ret = (lock_policy_rwsem_write(cpu) < 0);
+	WARN_ON(ret);
 
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
@@ -835,7 +837,7 @@ static int cpufreq_add_dev(struct sys_de
 	ret = cpufreq_driver->init(policy);
 	if (ret) {
 		dprintk("initialization failed\n");
-		goto err_out;
+		goto err_unlock_policy;
 	}
 	policy->user_policy.min = policy->min;
 	policy->user_policy.max = policy->max;
@@ -860,15 +862,21 @@ static int cpufreq_add_dev(struct sys_de
 		/* Check for existing affected CPUs.
 		 * They may not be aware of it due to CPU Hotplug.
 		 */
-		managed_policy = cpufreq_cpu_get(j);		/* FIXME: Where is this released?  What about error paths? */
+		managed_policy = cpufreq_cpu_get(j);
 		if (unlikely(managed_policy)) {
 
 			/* Set proper policy_cpu */
 			unlock_policy_rwsem_write(cpu);
 			per_cpu(policy_cpu, cpu) = managed_policy->cpu;
 
-			if (lock_policy_rwsem_write(cpu) < 0)
-				goto err_out_driver_exit;
+			if (lock_policy_rwsem_write(cpu) < 0) {
+				/* Should not go through policy unlock path */
+				if (cpufreq_driver->exit)
+					cpufreq_driver->exit(policy);
+				ret = -EBUSY;
+				cpufreq_cpu_put(managed_policy);
+				goto err_free_cpumask;
+			}
 
 			spin_lock_irqsave(&cpufreq_driver_lock, flags);
 			cpumask_copy(managed_policy->cpus, policy->cpus);
@@ -879,12 +887,14 @@ static int cpufreq_add_dev(struct sys_de
 			ret = sysfs_create_link(&sys_dev->kobj,
 						&managed_policy->kobj,
 						"cpufreq");
-			if (ret)
-				goto err_out_driver_exit;
-
-			cpufreq_debug_enable_ratelimit();
-			ret = 0;
-			goto err_out_driver_exit; /* call driver->exit() */
+			if (!ret)
+				cpufreq_cpu_put(managed_policy);
+			/*
+			 * Success. We only needed to be added to the mask.
+			 * Call driver->exit() because only the cpu parent of
+			 * the kobj needed to call init().
+			 */
+			goto out_driver_exit; /* call driver->exit() */
 		}
 	}
 #endif
@@ -894,25 +904,25 @@ static int cpufreq_add_dev(struct sys_de
 	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, &sys_dev->kobj,
 				   "cpufreq");
 	if (ret)
-		goto err_out_driver_exit;
+		goto out_driver_exit;
 
 	/* set up files for this cpu device */
 	drv_attr = cpufreq_driver->attr;
 	while ((drv_attr) && (*drv_attr)) {
 		ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
 		if (ret)
-			goto err_out_driver_exit;
+			goto err_out_kobj_put;
 		drv_attr++;
 	}
 	if (cpufreq_driver->get) {
 		ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr);
 		if (ret)
-			goto err_out_driver_exit;
+			goto err_out_kobj_put;
 	}
 	if (cpufreq_driver->target) {
 		ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
 		if (ret)
-			goto err_out_driver_exit;
+			goto err_out_kobj_put;
 	}
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
@@ -930,12 +940,14 @@ static int cpufreq_add_dev(struct sys_de
 			continue;
 
 		dprintk("CPU %u already managed, adding link\n", j);
-		cpufreq_cpu_get(cpu);
+		managed_policy = cpufreq_cpu_get(cpu);
 		cpu_sys_dev = get_cpu_sysdev(j);
 		ret = sysfs_create_link(&cpu_sys_dev->kobj, &policy->kobj,
 					"cpufreq");
-		if (ret)
+		if (ret) {
+			cpufreq_cpu_put(managed_policy);
 			goto err_out_unregister;
+		}
 	}
 
 	policy->governor = NULL; /* to assure that the starting sequence is
@@ -967,17 +979,20 @@ err_out_unregister:
 		per_cpu(cpufreq_cpu_data, j) = NULL;
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+err_out_kobj_put:
 	kobject_put(&policy->kobj);
 	wait_for_completion(&policy->kobj_unregister);
 
-err_out_driver_exit:
+out_driver_exit:
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 
-err_out:
+err_unlock_policy:
 	unlock_policy_rwsem_write(cpu);
+err_free_cpumask:
+	free_cpumask_var(policy->cpus);
+err_free_policy:
 	kfree(policy);
-
 nomem_out:
 	module_put(cpufreq_driver->owner);
 module_out:

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Mathieu Desnoyers]

* Pallipadi, Venkatesh (venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org) wrote:
>  
> 
> >-----Original Message-----
> >From: Mathieu Desnoyers [mailto:mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org] 
> >Sent: Thursday, July 02, 2009 6:07 PM
> >To: Pallipadi, Venkatesh
> >Cc: Dave Jones; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> >cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Ingo 
> >Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Thomas Renninger
> >Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep 
> >warnings in cpufreq
> >
> >* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org (venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org) wrote:
> >> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
> >> much needed to properly cleanup ondemand timer, opened-up a 
> >can of worms
> >> related to locking dependencies in cpufreq.
> >> 
> >> Patch here defines the need for dbs_mutex and cleans up its usage in
> >> ondemand governor. This also resolves the lockdep warnings 
> >reported here
> >> 
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
> >> 
> >> and few others..
> >> 
> >> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/cpufreq/cpufreq.c              |    4 ++--
> >>  drivers/cpufreq/cpufreq_conservative.c |   27 
> >+++++++++++----------------
> >>  drivers/cpufreq/cpufreq_ondemand.c     |   27 
> >+++++++++++----------------
> >>  3 files changed, 24 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index 6e2ec0b..c7fe16e 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct 
> >sys_device *sys_dev)
> >>  	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> >>  #endif
> >>  
> >> -	unlock_policy_rwsem_write(cpu);
> >> -
> >>  	if (cpufreq_driver->target)
> >>  		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
> >>  
> >> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct 
> >sys_device *sys_dev)
> >>  	if (cpufreq_driver->exit)
> >>  		cpufreq_driver->exit(data);
> >>  
> >> +	unlock_policy_rwsem_write(cpu);
> >> +
> >>  	free_cpumask_var(data->related_cpus);
> >>  	free_cpumask_var(data->cpus);
> >>  	kfree(data);
> >> diff --git a/drivers/cpufreq/cpufreq_conservative.c 
> >b/drivers/cpufreq/cpufreq_conservative.c
> >> index 7fc58af..58889f2 100644
> >> --- a/drivers/cpufreq/cpufreq_conservative.c
> >> +++ b/drivers/cpufreq/cpufreq_conservative.c
> >> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct 
> >cpu_dbs_info_s, cpu_dbs_info);
> >>  static unsigned int dbs_enable;	/* number of CPUs using 
> >this policy */
> >>  
> >>  /*
> >> - * DEADLOCK ALERT! There is a ordering requirement between 
> >cpu_hotplug
> >> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> >> - * dbs_mutex. If any function that can potentially take 
> >cpu_hotplug lock
> >> - * (like __cpufreq_driver_target()) is being called with 
> >dbs_mutex taken, then
> >> - * cpu_hotplug lock should be taken before that. Note that 
> >cpu_hotplug lock
> >> - * is recursive for the same process. -Venki
> >> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the 
> >dbs_mutex, because it
> >> - * would deadlock with cancel_delayed_work_sync(), which is 
> >needed for proper
> >> - * raceless workqueue teardown.
> >> + * dbs_mutex protects data in dbs_tuners_ins from 
> >concurrent changes on
> >> + * different CPUs. It protects dbs_enable in governor 
> >start/stop. It also
> >> + * serializes governor limit_change with do_dbs_timer. We 
> >do not want
> >> + * do_dbs_timer to run when user is changing the governor or limits.
> >>   */
> >>  static DEFINE_MUTEX(dbs_mutex);
> >>  
> >> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct 
> >work_struct *work)
> >>  
> >>  	delay -= jiffies % delay;
> >>  
> >> -	if (lock_policy_rwsem_write(cpu) < 0)
> >> -		return;
> >> +	mutex_lock(&dbs_mutex);
> >
> >OK, I now have absolutely no idea what the rwsem mutex is protecting
> >anymore.
> >
> >You should probably describe the new world order not just in terms of
> >what the dbs_mutex is protecting, but also about what the rwsem is
> >doing. I'm worried that this rwsem is there to protect against 
> >more than
> >what is protected by the dbs_mutex local to the ondemand/conservative
> >governors.
> >
> >See below,
> >
> >>  
> >>  	if (!dbs_info->enable) {
> >> -		unlock_policy_rwsem_write(cpu);
> >> +		mutex_unlock(&dbs_mutex);
> >>  		return;
> >>  	}
> >>  
> >>  	dbs_check_cpu(dbs_info);
> >>  
> >>  	queue_delayed_work_on(cpu, kconservative_wq, 
> >&dbs_info->work, delay);
> >> -	unlock_policy_rwsem_write(cpu);
> >> +	mutex_unlock(&dbs_mutex);
> >>  }
> >>  
> >>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> >> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct 
> >cpufreq_policy *policy,
> >>  					&dbs_cpufreq_notifier_block,
> >>  					CPUFREQ_TRANSITION_NOTIFIER);
> >>  		}
> >> -		dbs_timer_init(this_dbs_info);
> >> -
> >>  		mutex_unlock(&dbs_mutex);
> >>  
> >> +		dbs_timer_init(this_dbs_info);
> >> +
> >>  		break;
> >>  
> >>  	case CPUFREQ_GOV_STOP:
> >> -		mutex_lock(&dbs_mutex);
> >>  		dbs_timer_exit(this_dbs_info);
> >
> >So now the only thing that seems to prevent the init and exit to race
> >with each other is the rwsem. But this does not seem to be described
> >anywhere.
> 
> Mathieu,
> 
> Yes. rwsem in cpufreq core makes sure that START and STOP happen sequentially. There
> Is no way for START and STOP for a CPU to happen at the same time as cpufreq core holds
> per policy rwsem lock before making any change to the policy. I can add a comment to
> that effect in cpufreq.c. This is a clean seperation across cpufreq core and governor,
> as cpufreq core takes care of all the policy changes. With that, do you see any
> Issues/races with this patchset?
> 

I'll code a less intrusive patchset that should hopefully fix the
problem tonight with less possible side-effects. I'll need help for
testing though.

Matheu

> Thanks,
> Venki

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

[Ingo Molnar]

* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> Since recent chanegs to ondemand and conservative governor, there 
> have been multiple reports of lockdep issues in cpufreq. Patch 
> series takes care of these problems.
> 
> This is the next attempt following the one here, which was not a 
> complete fix. 
> http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html
> 
> I am currently running some stress tests to make sure there are no 
> issues with these patches. But, wanted to send them out for 
> review/comments/testing before I head out for the long weekend.
> 
> If this patchset seems sane, the first patch in the patchset 
> should also get into 30.stable.

Btw., FYI, because my test-systems were frequently triggering those 
bugs, i kept testing the following series from you and Mathieu in 
-tip:

 ecf8b04: cpufreq: Define dbs_mutex purpose and cleanup its usage conservative gov
 b08c597: cpufreq: Define dbs_mutex purpose and cleanup its usage
 0807e30: cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call (second call site)

So that fix-series, while probably not complete (given that you sent 
a v2 series), worked well in practice and gets my:

 Tested-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>

Is the delta between this (tested) series and your v2 version 
significant? If not it might make sense to shape it as a delta patch 
to the v1 series, if that looks clean enough - to preserve testing 
results.

	Ingo

Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Thomas Renninger]

On Friday 03 July 2009 02:08:30 venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
> much needed to properly cleanup ondemand timer, opened-up a can of worms
> related to locking dependencies in cpufreq.
> 
> Patch here defines the need for dbs_mutex and cleans up its usage in
> ondemand governor. This also resolves the lockdep warnings reported here
I think I get these changes and now dbs_mutex is needed...
Making sure governor() is always called with rwsem held (hope that is the
case now) is a good idea. Unfortunately it requires the dbs_mutex in
do_dbs_timer and it will be hard to ever remove it.

I still do not see the need of "dbs_mutex protects data in dbs_tuners_ins
from concurrent changes", though. If someone enlightens me, that would
be appreciated.

Thanks,

     Thomas

> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
> 
> and few others..
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/cpufreq/cpufreq.c              |    4 ++--
>  drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
>  drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
>  3 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6e2ec0b..c7fe16e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct sys_device 
*sys_dev)
>  	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  #endif
>  
> -	unlock_policy_rwsem_write(cpu);
> -
>  	if (cpufreq_driver->target)
>  		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
>  
> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct sys_device 
*sys_dev)
>  	if (cpufreq_driver->exit)
>  		cpufreq_driver->exit(data);
>  
> +	unlock_policy_rwsem_write(cpu);
> +
>  	free_cpumask_var(data->related_cpus);
>  	free_cpumask_var(data->cpus);
>  	kfree(data);
> diff --git a/drivers/cpufreq/cpufreq_conservative.c 
b/drivers/cpufreq/cpufreq_conservative.c
> index 7fc58af..58889f2 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, 
cpu_dbs_info);
>  static unsigned int dbs_enable;	/* number of CPUs using this policy */
>  
>  /*
> - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> - * dbs_mutex. If any function that can potentially take cpu_hotplug lock
> - * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, 
then
> - * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
> - * is recursive for the same process. -Venki
> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, 
because it
> - * would deadlock with cancel_delayed_work_sync(), which is needed for 
proper
> - * raceless workqueue teardown.
> + * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
> + * different CPUs. It protects dbs_enable in governor start/stop. It also
> + * serializes governor limit_change with do_dbs_timer. We do not want
> + * do_dbs_timer to run when user is changing the governor or limits.
>   */
>  static DEFINE_MUTEX(dbs_mutex);
>  
> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct work_struct *work)
>  
>  	delay -= jiffies % delay;
>  
> -	if (lock_policy_rwsem_write(cpu) < 0)
> -		return;
> +	mutex_lock(&dbs_mutex);
>  
>  	if (!dbs_info->enable) {
> -		unlock_policy_rwsem_write(cpu);
> +		mutex_unlock(&dbs_mutex);
>  		return;
>  	}
>  
>  	dbs_check_cpu(dbs_info);
>  
>  	queue_delayed_work_on(cpu, kconservative_wq, &dbs_info->work, delay);
> -	unlock_policy_rwsem_write(cpu);
> +	mutex_unlock(&dbs_mutex);
>  }
>  
>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct cpufreq_policy 
*policy,
>  					&dbs_cpufreq_notifier_block,
>  					CPUFREQ_TRANSITION_NOTIFIER);
>  		}
> -		dbs_timer_init(this_dbs_info);
> -
>  		mutex_unlock(&dbs_mutex);
>  
> +		dbs_timer_init(this_dbs_info);
> +
>  		break;
>  
>  	case CPUFREQ_GOV_STOP:
> -		mutex_lock(&dbs_mutex);
>  		dbs_timer_exit(this_dbs_info);
> +
> +		mutex_lock(&dbs_mutex);
>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>  		dbs_enable--;
>  
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
> index 1911d17..246ae14 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, 
cpu_dbs_info);
>  static unsigned int dbs_enable;	/* number of CPUs using this policy */
>  
>  /*
> - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> - * dbs_mutex. If any function that can potentially take cpu_hotplug lock
> - * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, 
then
> - * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
> - * is recursive for the same process. -Venki
> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, 
because it
> - * would deadlock with cancel_delayed_work_sync(), which is needed for 
proper
> - * raceless workqueue teardown.
> + * dbs_mutex protects data in dbs_tuners_ins from concurrent changes on
> + * different CPUs. It protects dbs_enable in governor start/stop. It also
> + * serializes governor limit_change with do_dbs_timer. We do not want
> + * do_dbs_timer to run when user is changing the governor or limits.
>   */
>  static DEFINE_MUTEX(dbs_mutex);
>  
> @@ -494,11 +489,10 @@ static void do_dbs_timer(struct work_struct *work)
>  
>  	delay -= jiffies % delay;
>  
> -	if (lock_policy_rwsem_write(cpu) < 0)
> -		return;
> +	mutex_lock(&dbs_mutex);
>  
>  	if (!dbs_info->enable) {
> -		unlock_policy_rwsem_write(cpu);
> +		mutex_unlock(&dbs_mutex);
>  		return;
>  	}
>  
> @@ -517,7 +511,7 @@ static void do_dbs_timer(struct work_struct *work)
>  			dbs_info->freq_lo, CPUFREQ_RELATION_H);
>  	}
>  	queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work, delay);
> -	unlock_policy_rwsem_write(cpu);
> +	mutex_unlock(&dbs_mutex);
>  }
>  
>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> @@ -598,14 +592,15 @@ static int cpufreq_governor_dbs(struct cpufreq_policy 
*policy,
>  				max(min_sampling_rate,
>  				    latency * LATENCY_MULTIPLIER);
>  		}
> -		dbs_timer_init(this_dbs_info);
> -
>  		mutex_unlock(&dbs_mutex);
> +
> +		dbs_timer_init(this_dbs_info);
>  		break;
>  
>  	case CPUFREQ_GOV_STOP:
> -		mutex_lock(&dbs_mutex);
>  		dbs_timer_exit(this_dbs_info);
> +
> +		mutex_lock(&dbs_mutex);
>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>  		dbs_enable--;
>  		mutex_unlock(&dbs_mutex);
> -- 
> 1.6.0.6
> 
> -- 
> 
> 

Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

[Mathieu Desnoyers]

* Ingo Molnar (mingo-X9Un+BFzKDI@public.gmane.org) wrote:
> 
> * venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Since recent chanegs to ondemand and conservative governor, there 
> > have been multiple reports of lockdep issues in cpufreq. Patch 
> > series takes care of these problems.
> > 
> > This is the next attempt following the one here, which was not a 
> > complete fix. 
> > http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html
> > 
> > I am currently running some stress tests to make sure there are no 
> > issues with these patches. But, wanted to send them out for 
> > review/comments/testing before I head out for the long weekend.
> > 
> > If this patchset seems sane, the first patch in the patchset 
> > should also get into 30.stable.
> 
> Btw., FYI, because my test-systems were frequently triggering those 
> bugs, i kept testing the following series from you and Mathieu in 
> -tip:
> 
>  ecf8b04: cpufreq: Define dbs_mutex purpose and cleanup its usage conservative gov
>  b08c597: cpufreq: Define dbs_mutex purpose and cleanup its usage
>  0807e30: cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call (second call site)
> 
> So that fix-series, while probably not complete (given that you sent 
> a v2 series), worked well in practice and gets my:
> 
>  Tested-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
> 
> Is the delta between this (tested) series and your v2 version 
> significant? If not it might make sense to shape it as a delta patch 
> to the v1 series, if that looks clean enough - to preserve testing 
> results.

The delta is very significant. The purpose of each lock changes quite a
bit. I'm preparing a patch serie that should just fix the problem
without significant locking semantic modification.

(not that I have time to do this, but I end up spending more time
looking at the proposed solutions than doing it..) ;)

Mathieu

> 
> 	Ingo
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

RE: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Thomas Renninger [mailto:trenn-l3A5Bk7waGM@public.gmane.org] 
>Sent: Friday, July 03, 2009 4:41 AM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Ingo 
>Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu Desnoyers
>Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep 
>warnings in cpufreq
>
>On Friday 03 July 2009 02:08:30 venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
>> Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very
>> much needed to properly cleanup ondemand timer, opened-up a 
>can of worms
>> related to locking dependencies in cpufreq.
>> 
>> Patch here defines the need for dbs_mutex and cleans up its usage in
>> ondemand governor. This also resolves the lockdep warnings 
>reported here
>I think I get these changes and now dbs_mutex is needed...
>Making sure governor() is always called with rwsem held (hope 
>that is the
>case now) is a good idea. Unfortunately it requires the dbs_mutex in
>do_dbs_timer and it will be hard to ever remove it.

Yes. Rwsem held for any policy changes from cpufreq core makes this fix
clean. I did not like the earlier state where rwsem was helt for START
and not for STOP calls etc.

Patch 3 and 4 removes dbs_mutex from do_dbs_timer.

>I still do not see the need of "dbs_mutex protects data in 
>dbs_tuners_ins
>from concurrent changes", though. If someone enlightens me, that would
>be appreciated.

OK. Consider these two happening in parallel.
echo 0 > /sys/devices/system/cpu/cpu0/cpufreq/ondemand/ignore_nice
echo 1 > /sys/devices/system/cpu/cpu4/cpufreq/ondemand/ignore_nice

As they are coming from different cpu, rwsem wont protect us and 
without the dbs_mutex, end state after this can will be unpredictable.
prev_cpu_idle and prev_cpu_nice can end up with wrong values where
only one of them is set etc. That will affect the ondemand algorithm.

Thanks,
Venki

>> http://lkml.indiana.edu/hypermail/linux/kernel/0906.1/01925.html
>> http://lkml.indiana.edu/hypermail/linux/kernel/0907.0/00820.html
>> 
>> and few others..
>> 
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/cpufreq/cpufreq.c              |    4 ++--
>>  drivers/cpufreq/cpufreq_conservative.c |   27 
>+++++++++++----------------
>>  drivers/cpufreq/cpufreq_ondemand.c     |   27 
>+++++++++++----------------
>>  3 files changed, 24 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 6e2ec0b..c7fe16e 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct 
>sys_device 
>*sys_dev)
>>  	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>  #endif
>>  
>> -	unlock_policy_rwsem_write(cpu);
>> -
>>  	if (cpufreq_driver->target)
>>  		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
>>  
>> @@ -1088,6 +1086,8 @@ static int __cpufreq_remove_dev(struct 
>sys_device 
>*sys_dev)
>>  	if (cpufreq_driver->exit)
>>  		cpufreq_driver->exit(data);
>>  
>> +	unlock_policy_rwsem_write(cpu);
>> +
>>  	free_cpumask_var(data->related_cpus);
>>  	free_cpumask_var(data->cpus);
>>  	kfree(data);
>> diff --git a/drivers/cpufreq/cpufreq_conservative.c 
>b/drivers/cpufreq/cpufreq_conservative.c
>> index 7fc58af..58889f2 100644
>> --- a/drivers/cpufreq/cpufreq_conservative.c
>> +++ b/drivers/cpufreq/cpufreq_conservative.c
>> @@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, 
>cpu_dbs_info);
>>  static unsigned int dbs_enable;	/* number of CPUs using 
>this policy */
>>  
>>  /*
>> - * DEADLOCK ALERT! There is a ordering requirement between 
>cpu_hotplug
>> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
>> - * dbs_mutex. If any function that can potentially take 
>cpu_hotplug lock
>> - * (like __cpufreq_driver_target()) is being called with 
>dbs_mutex taken, 
>then
>> - * cpu_hotplug lock should be taken before that. Note that 
>cpu_hotplug lock
>> - * is recursive for the same process. -Venki
>> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the 
>dbs_mutex, 
>because it
>> - * would deadlock with cancel_delayed_work_sync(), which is 
>needed for 
>proper
>> - * raceless workqueue teardown.
>> + * dbs_mutex protects data in dbs_tuners_ins from 
>concurrent changes on
>> + * different CPUs. It protects dbs_enable in governor 
>start/stop. It also
>> + * serializes governor limit_change with do_dbs_timer. We 
>do not want
>> + * do_dbs_timer to run when user is changing the governor or limits.
>>   */
>>  static DEFINE_MUTEX(dbs_mutex);
>>  
>> @@ -488,18 +483,17 @@ static void do_dbs_timer(struct 
>work_struct *work)
>>  
>>  	delay -= jiffies % delay;
>>  
>> -	if (lock_policy_rwsem_write(cpu) < 0)
>> -		return;
>> +	mutex_lock(&dbs_mutex);
>>  
>>  	if (!dbs_info->enable) {
>> -		unlock_policy_rwsem_write(cpu);
>> +		mutex_unlock(&dbs_mutex);
>>  		return;
>>  	}
>>  
>>  	dbs_check_cpu(dbs_info);
>>  
>>  	queue_delayed_work_on(cpu, kconservative_wq, 
>&dbs_info->work, delay);
>> -	unlock_policy_rwsem_write(cpu);
>> +	mutex_unlock(&dbs_mutex);
>>  }
>>  
>>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
>> @@ -590,15 +584,16 @@ static int cpufreq_governor_dbs(struct 
>cpufreq_policy 
>*policy,
>>  					&dbs_cpufreq_notifier_block,
>>  					CPUFREQ_TRANSITION_NOTIFIER);
>>  		}
>> -		dbs_timer_init(this_dbs_info);
>> -
>>  		mutex_unlock(&dbs_mutex);
>>  
>> +		dbs_timer_init(this_dbs_info);
>> +
>>  		break;
>>  
>>  	case CPUFREQ_GOV_STOP:
>> -		mutex_lock(&dbs_mutex);
>>  		dbs_timer_exit(this_dbs_info);
>> +
>> +		mutex_lock(&dbs_mutex);
>>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>>  		dbs_enable--;
>>  
>> diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
>b/drivers/cpufreq/cpufreq_ondemand.c
>> index 1911d17..246ae14 100644
>> --- a/drivers/cpufreq/cpufreq_ondemand.c
>> +++ b/drivers/cpufreq/cpufreq_ondemand.c
>> @@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s, 
>cpu_dbs_info);
>>  static unsigned int dbs_enable;	/* number of CPUs using 
>this policy */
>>  
>>  /*
>> - * DEADLOCK ALERT! There is a ordering requirement between 
>cpu_hotplug
>> - * lock and dbs_mutex. cpu_hotplug lock should always be held before
>> - * dbs_mutex. If any function that can potentially take 
>cpu_hotplug lock
>> - * (like __cpufreq_driver_target()) is being called with 
>dbs_mutex taken, 
>then
>> - * cpu_hotplug lock should be taken before that. Note that 
>cpu_hotplug lock
>> - * is recursive for the same process. -Venki
>> - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the 
>dbs_mutex, 
>because it
>> - * would deadlock with cancel_delayed_work_sync(), which is 
>needed for 
>proper
>> - * raceless workqueue teardown.
>> + * dbs_mutex protects data in dbs_tuners_ins from 
>concurrent changes on
>> + * different CPUs. It protects dbs_enable in governor 
>start/stop. It also
>> + * serializes governor limit_change with do_dbs_timer. We 
>do not want
>> + * do_dbs_timer to run when user is changing the governor or limits.
>>   */
>>  static DEFINE_MUTEX(dbs_mutex);
>>  
>> @@ -494,11 +489,10 @@ static void do_dbs_timer(struct 
>work_struct *work)
>>  
>>  	delay -= jiffies % delay;
>>  
>> -	if (lock_policy_rwsem_write(cpu) < 0)
>> -		return;
>> +	mutex_lock(&dbs_mutex);
>>  
>>  	if (!dbs_info->enable) {
>> -		unlock_policy_rwsem_write(cpu);
>> +		mutex_unlock(&dbs_mutex);
>>  		return;
>>  	}
>>  
>> @@ -517,7 +511,7 @@ static void do_dbs_timer(struct 
>work_struct *work)
>>  			dbs_info->freq_lo, CPUFREQ_RELATION_H);
>>  	}
>>  	queue_delayed_work_on(cpu, kondemand_wq, 
>&dbs_info->work, delay);
>> -	unlock_policy_rwsem_write(cpu);
>> +	mutex_unlock(&dbs_mutex);
>>  }
>>  
>>  static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
>> @@ -598,14 +592,15 @@ static int cpufreq_governor_dbs(struct 
>cpufreq_policy 
>*policy,
>>  				max(min_sampling_rate,
>>  				    latency * LATENCY_MULTIPLIER);
>>  		}
>> -		dbs_timer_init(this_dbs_info);
>> -
>>  		mutex_unlock(&dbs_mutex);
>> +
>> +		dbs_timer_init(this_dbs_info);
>>  		break;
>>  
>>  	case CPUFREQ_GOV_STOP:
>> -		mutex_lock(&dbs_mutex);
>>  		dbs_timer_exit(this_dbs_info);
>> +
>> +		mutex_lock(&dbs_mutex);
>>  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
>>  		dbs_enable--;
>>  		mutex_unlock(&dbs_mutex);
>> -- 
>> 1.6.0.6
>> 
>> -- 
>> 
>> 
>
>
>--
To unsubscribe from this list: send the line "unsubscribe kernel-testers" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo-X9Un+BFzKDI@public.gmane.org] 
>Sent: Thursday, July 02, 2009 11:54 PM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu 
>Desnoyers; Thomas Renninger
>Subject: Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)
>
>
>* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
>> Since recent chanegs to ondemand and conservative governor, there 
>> have been multiple reports of lockdep issues in cpufreq. Patch 
>> series takes care of these problems.
>> 
>> This is the next attempt following the one here, which was not a 
>> complete fix. 
>> http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html
>> 
>> I am currently running some stress tests to make sure there are no 
>> issues with these patches. But, wanted to send them out for 
>> review/comments/testing before I head out for the long weekend.
>> 
>> If this patchset seems sane, the first patch in the patchset 
>> should also get into 30.stable.
>
>Btw., FYI, because my test-systems were frequently triggering those 
>bugs, i kept testing the following series from you and Mathieu in 
>-tip:
>
> ecf8b04: cpufreq: Define dbs_mutex purpose and cleanup its 
>usage conservative gov
> b08c597: cpufreq: Define dbs_mutex purpose and cleanup its usage
> 0807e30: cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP 
>call (second call site)
>
>So that fix-series, while probably not complete (given that you sent 
>a v2 series), worked well in practice and gets my:
>
> Tested-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
>
>Is the delta between this (tested) series and your v2 version 
>significant? If not it might make sense to shape it as a delta patch 
>to the v1 series, if that looks clean enough - to preserve testing 
>results.
>

Thanks for testing. That earlier version even though it took care
of lockdep complaints, did not address all the race conditions properly.
The delta is significant as I had to change the approach compared
to first patchset. So, diff will not be very clean.

Thanks,
Venki--
To unsubscribe from this list: send the line "unsubscribe kernel-testers" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

[Ingo Molnar]

* Pallipadi, Venkatesh <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

>  
> 
> >-----Original Message-----
> >From: Ingo Molnar [mailto:mingo-X9Un+BFzKDI@public.gmane.org] 
> >Sent: Thursday, July 02, 2009 11:54 PM
> >To: Pallipadi, Venkatesh
> >Cc: Dave Jones; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> >cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> >Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu 
> >Desnoyers; Thomas Renninger
> >Subject: Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)
> >
> >
> >* venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> Since recent chanegs to ondemand and conservative governor, there 
> >> have been multiple reports of lockdep issues in cpufreq. Patch 
> >> series takes care of these problems.
> >> 
> >> This is the next attempt following the one here, which was not a 
> >> complete fix. 
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html
> >> 
> >> I am currently running some stress tests to make sure there are no 
> >> issues with these patches. But, wanted to send them out for 
> >> review/comments/testing before I head out for the long weekend.
> >> 
> >> If this patchset seems sane, the first patch in the patchset 
> >> should also get into 30.stable.
> >
> >Btw., FYI, because my test-systems were frequently triggering those 
> >bugs, i kept testing the following series from you and Mathieu in 
> >-tip:
> >
> > ecf8b04: cpufreq: Define dbs_mutex purpose and cleanup its 
> >usage conservative gov
> > b08c597: cpufreq: Define dbs_mutex purpose and cleanup its usage
> > 0807e30: cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP 
> >call (second call site)
> >
> >So that fix-series, while probably not complete (given that you sent 
> >a v2 series), worked well in practice and gets my:
> >
> > Tested-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
> >
> >Is the delta between this (tested) series and your v2 version 
> >significant? If not it might make sense to shape it as a delta patch 
> >to the v1 series, if that looks clean enough - to preserve testing 
> >results.
> >
> 
> Thanks for testing. That earlier version even though it took care 
> of lockdep complaints, did not address all the race conditions 
> properly. The delta is significant as I had to change the approach 
> compared to first patchset. So, diff will not be very clean.

Fair enough - these cases are when it makes sense to do a clean 
rebase.

	Ingo

Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

[Thomas Renninger]

On Friday 03 July 2009 16:28:43 Pallipadi, Venkatesh wrote:
> 
...
> >I still do not see the need of "dbs_mutex protects data in 
> >dbs_tuners_ins
> >from concurrent changes", though. If someone enlightens me, that would
> >be appreciated.
> 
> OK. Consider these two happening in parallel.
> echo 0 > /sys/devices/system/cpu/cpu0/cpufreq/ondemand/ignore_nice
> echo 1 > /sys/devices/system/cpu/cpu4/cpufreq/ondemand/ignore_nice
Hm, I just consider parallel configuration, especially with different
values as a userspace bug anyway.

> As they are coming from different cpu, rwsem wont protect us and 
> without the dbs_mutex, end state after this can will be unpredictable.
> prev_cpu_idle and prev_cpu_nice can end up with wrong values where
> only one of them is set etc. That will affect the ondemand algorithm.
For one sample in this case.
But I see that it should be made 100%
bulletproof and even userspace is doing wrong things already you want to
have a defined state. A separate mutex, uncoupled from .governor() would make 
things easier, but I wait until it's clear what cleanups are going into which 
kernel and will suggest another cleanup to only allow
global dbs_tuners on top for .31 or further future.

Thanks,

    Thomas

Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

[Pallipadi, Venkatesh]

On Thu, 2009-07-02 at 17:08 -0700, Pallipadi, Venkatesh wrote:
> Since recent chanegs to ondemand and conservative governor, there have been
> multiple reports of lockdep issues in cpufreq. Patch series takes care of
> these problems.
> 
> This is the next attempt following the one here, which was not a complete fix.
> http://lkml.indiana.edu/hypermail/linux/kernel/0906.3/01073.html
> 
> I am currently running some stress tests to make sure there are no issues with
> these patches. But, wanted to send them out for review/comments/testing
> before I head out for the long weekend.
> 
> If this patchset seems sane, the first patch in the patchset should also get
> into 30.stable.
> 

Just an update. I ran some stress test on few of my test machines over
the long weekend and havent seen any issues with this patchset.

Thanks,
Venki