Skip to contentSkip to conversation

Take care of cpufreq lockdep issues (take 2)

PATCH In review [email protected] 4 participants 17 messages Jul 3, 2009 – Jul 6, 2009

Subsystems (2)
Documentation CPU frequency scaling framework
lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover Take care of cpufreq lockdep issues (take 2)

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.

author
#1 lore raw

PATCH 1/4 cpufreq: Eliminate the recent lockdep warnings in cpufreq

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
 drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +2 -2
index 6e2ec0b..c7fe16e 100644
@@ -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);
drivers/cpufreq/cpufreq_conservative.c +11 -16
index 7fc58af..58889f2 100644
@@ -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--;
drivers/cpufreq/cpufreq_ondemand.c +11 -16
index 1911d17..246ae14 100644
@@ -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);
Signature · 3 lines
1.6.0.6

--
#2 lore raw

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

Quoted 66 lines · * [email protected]…

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
 drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +2 -2
index 6e2ec0b..c7fe16e 100644
@@ -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);
drivers/cpufreq/cpufreq_conservative.c +4 -9
index 7fc58af..58889f2 100644
@@ -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);

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work) · line 486

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -2
@@ -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,

Quoted 20 lines
drivers/cpufreq/cpufreq_conservative.c +2 -3
 	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);

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 594

Quoted 10 lines
drivers/cpufreq/cpufreq_conservative.c +2 -2
-
 		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

Quoted 76 lines
drivers/cpufreq/cpufreq_conservative.c +2 -0
+
+		mutex_lock(&dbs_mutex);
 		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
 		dbs_enable--;
drivers/cpufreq/cpufreq_ondemand.c +11 -16
index 1911d17..246ae14 100644
@@ -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);
Signature · 3 lines
1.6.0.6

--
Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#3 lore raw

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

Quoted 146 lines

-----Original Message-----
From: Mathieu Desnoyers [mailto:[email protected]]
Sent: Thursday, July 02, 2009 6:07 PM
To: Pallipadi, Venkatesh
Cc: Dave Jones; [email protected]; [email protected]; [email protected]; Ingo Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Thomas Renninger
Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

Quoted 2 lines · * [email protected] ([email protected]) wrote:

Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very much needed to properly cleanup ondemand timer, opened-up a

can of worms

Quoted 4 lines

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

Quoted 10 lines

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27

+++++++++++----------------

Quoted 1 line
 drivers/cpufreq/cpufreq_ondemand.c     |   27

+++++++++++----------------

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 7 lines
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +0 -0
index 6e2ec0b..c7fe16e 100644
@@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct

sys_device *sys_dev)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 9 lines
drivers/cpufreq/cpufreq.c +0 -2
 	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)

Review

Quoted 9 lines
drivers/cpufreq/cpufreq.c +2 -0
 	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);
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_conservative.c

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct

Quoted 4 lines

index 7fc58af..58889f2 100644

drivers/cpufreq/cpufreq_conservative.c +0 -0
@@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct

cpu_dbs_info_s, cpu_dbs_info);

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 70

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -0
 static unsigned int dbs_enable;	/* number of CPUs using

this policy */

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 72

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -1
 /*
- * DEADLOCK ALERT! There is a ordering requirement between

cpu_hotplug

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 74

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 75

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * (like __cpufreq_driver_target()) is being called with

dbs_mutex taken, then

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 76

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * cpu_hotplug lock should be taken before that. Note that

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 78

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the

dbs_mutex, because it

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 79

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is

needed for proper

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 72

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from

concurrent changes on

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 73

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * different CPUs. It protects dbs_enable in governor

start/stop. It also

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 74

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * serializes governor limit_change with do_dbs_timer. We

do not want

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct

Quoted 5 lines
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * 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)

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work) · line 486

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -2
 	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,

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work)

Quoted 10 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
 	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);

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
-	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,

Quoted 11 lines
drivers/cpufreq/cpufreq_conservative.c +2 -2
 					&dbs_cpufreq_notifier_block,
 					CPUFREQ_TRANSITION_NOTIFIER);
 		}
-		dbs_timer_init(this_dbs_info);
-
 		mutex_unlock(&dbs_mutex);
 
+		dbs_timer_init(this_dbs_info);
+
 		break;
Quoted 7 lines

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 594

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -1
 	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

#4 lore raw

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

Quoted 154 lines · * Pallipadi, Venkatesh ([email protected]…
Quoted 146 lines

-----Original Message-----
From: Mathieu Desnoyers [mailto:mathieu.desnoyers-scC8bbJcJLCw5LPnMra/[email protected]]
Sent: Thursday, July 02, 2009 6:07 PM
To: Pallipadi, Venkatesh
Cc: Dave Jones; [email protected]; [email protected]; [email protected]; Ingo Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Thomas Renninger
Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep
warnings in cpufreq

Quoted 2 lines · * [email protected]…

Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very much needed to properly cleanup ondemand timer, opened-up a

can of worms

Quoted 4 lines

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

Quoted 10 lines

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27

+++++++++++----------------

Quoted 1 line
 drivers/cpufreq/cpufreq_ondemand.c     |   27

+++++++++++----------------

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 7 lines
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +0 -0
index 6e2ec0b..c7fe16e 100644
@@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct

sys_device *sys_dev)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 9 lines
drivers/cpufreq/cpufreq.c +0 -2
 	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)

Review

Quoted 9 lines
drivers/cpufreq/cpufreq.c +2 -0
 	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);
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_conservative.c

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct

Quoted 4 lines

index 7fc58af..58889f2 100644

drivers/cpufreq/cpufreq_conservative.c +0 -0
@@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct

cpu_dbs_info_s, cpu_dbs_info);

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 70

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -0
 static unsigned int dbs_enable;	/* number of CPUs using

this policy */

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 72

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -1
 /*
- * DEADLOCK ALERT! There is a ordering requirement between

cpu_hotplug

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 74

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 75

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * (like __cpufreq_driver_target()) is being called with

dbs_mutex taken, then

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 76

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * cpu_hotplug lock should be taken before that. Note that

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 78

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the

dbs_mutex, because it

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 79

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is

needed for proper

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 72

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from

concurrent changes on

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 73

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * different CPUs. It protects dbs_enable in governor

start/stop. It also

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct · line 74

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * serializes governor limit_change with do_dbs_timer. We

do not want

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct

Quoted 5 lines
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * 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)

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work) · line 486

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -2
 	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,

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work)

Quoted 10 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
 	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);

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
-	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,

Quoted 11 lines
drivers/cpufreq/cpufreq_conservative.c +2 -2
 					&dbs_cpufreq_notifier_block,
 					CPUFREQ_TRANSITION_NOTIFIER);
 		}
-		dbs_timer_init(this_dbs_info);
-
 		mutex_unlock(&dbs_mutex);
 
+		dbs_timer_init(this_dbs_info);
+
 		break;
Quoted 7 lines

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 594

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -1
 	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.

Quoted 9 lines

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

Quoted 2 lines

Thanks,
Venki

Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
#5 lore raw

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

In reply to [email protected]
Quoted 6 lines · On Friday 03 July 2009 02:08:30 venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@publ…

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

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct sys_device

Quoted 17 lines

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27 +++++++++++----------------
 drivers/cpufreq/cpufreq_ondemand.c     |   27 +++++++++++----------------
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +0 -0
index 6e2ec0b..c7fe16e 100644
@@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct sys_device

*sys_dev)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct sys_device

Quoted 9 lines
drivers/cpufreq/cpufreq.c +0 -2
 	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)

Review

Quoted 9 lines
drivers/cpufreq/cpufreq.c +2 -0
 	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);
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_conservative.c

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s,

Quoted 4 lines

index 7fc58af..58889f2 100644

drivers/cpufreq/cpufreq_conservative.c +0 -0
@@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,

cpu_dbs_info);

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);

Quoted 7 lines
drivers/cpufreq/cpufreq_conservative.c +0 -4
 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

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, cpu_dbs_info);

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -3
- * 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

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 80

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is needed for

proper

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct cpufreq_policy

Quoted 31 lines
drivers/cpufreq/cpufreq_conservative.c +7 -5
- * 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,

Review

Quoted 20 lines
drivers/cpufreq/cpufreq_conservative.c +4 -3
 					&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--;
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_ondemand.c

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s,

Quoted 4 lines

index 1911d17..246ae14 100644

drivers/cpufreq/cpufreq_ondemand.c +0 -0
@@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,

cpu_dbs_info);

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 84

Quoted 7 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -4
 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

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 87

Quoted 3 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -3
- * 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

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 88

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is needed for

proper

drivers/cpufreq/cpufreq_ondemand.c static int cpufreq_governor_dbs(struct cpufreq_policy

Quoted 32 lines
drivers/cpufreq/cpufreq_ondemand.c +7 -5
- * 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,

Quoted 24 lines
drivers/cpufreq/cpufreq_ondemand.c +4 -3
 				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);
Signature · 3 lines
1.6.0.6

--
author
#6 lore raw

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

Quoted 18 lines

-----Original Message-----
From: Thomas Renninger [mailto:[email protected]]
Sent: Friday, July 03, 2009 4:41 AM
To: Pallipadi, Venkatesh
Cc: Dave Jones; [email protected]; [email protected]; [email protected]; Ingo Molnar; Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu Desnoyers
Subject: Re: [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq

Quoted 2 lines · On Friday 03 July 2009 02:08:30 [email protected] wrote:

Commit b14893a62c73af0eca414cfed505b8c09efc613c although it was very much needed to properly cleanup ondemand timer, opened-up a

can of worms

Quoted 2 lines

related to locking dependencies in cpufreq.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 220 lines
Quoted 9 lines

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 <[email protected]>
---
 drivers/cpufreq/cpufreq.c              |    4 ++--
 drivers/cpufreq/cpufreq_conservative.c |   27

+++++++++++----------------

Quoted 1 line
 drivers/cpufreq/cpufreq_ondemand.c     |   27

+++++++++++----------------

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 7 lines
 3 files changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +0 -0
index 6e2ec0b..c7fe16e 100644
@@ -1070,8 +1070,6 @@ static int __cpufreq_remove_dev(struct

sys_device
*sys_dev)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct

Quoted 9 lines
drivers/cpufreq/cpufreq.c +0 -2
 	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)

Review

Quoted 9 lines
drivers/cpufreq/cpufreq.c +2 -0
 	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);
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_conservative.c

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s,

Quoted 4 lines

index 7fc58af..58889f2 100644

drivers/cpufreq/cpufreq_conservative.c +0 -0
@@ -70,15 +70,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,

cpu_dbs_info);

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 70

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -0
 static unsigned int dbs_enable;	/* number of CPUs using

this policy */

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 72

Quoted 3 lines
drivers/cpufreq/cpufreq_conservative.c +0 -1
 /*
- * DEADLOCK ALERT! There is a ordering requirement between

cpu_hotplug

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 74

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 75

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * (like __cpufreq_driver_target()) is being called with

dbs_mutex taken,
then

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 76

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * cpu_hotplug lock should be taken before that. Note that

cpu_hotplug lock

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 78

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +0 -2
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the

dbs_mutex,
because it

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 79

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is

needed for
proper

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 72

Quoted 2 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from

concurrent changes on

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 73

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * different CPUs. It protects dbs_enable in governor

start/stop. It also

drivers/cpufreq/cpufreq_conservative.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 74

Quoted 1 line
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * serializes governor limit_change with do_dbs_timer. We

do not want

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct

Quoted 5 lines
drivers/cpufreq/cpufreq_conservative.c +1 -0
+ * 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)

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -2
 	delay -= jiffies % delay;
 
-	if (lock_policy_rwsem_write(cpu) < 0)
-		return;
+	mutex_lock(&dbs_mutex);

drivers/cpufreq/cpufreq_conservative.c static void do_dbs_timer(struct work_struct *work)

Quoted 10 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
 
 	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);

drivers/cpufreq/cpufreq_conservative.c static int cpufreq_governor_dbs(struct

Quoted 6 lines
drivers/cpufreq/cpufreq_conservative.c +1 -1
-	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,

Review

Quoted 20 lines
drivers/cpufreq/cpufreq_conservative.c +4 -3
 					&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--;
(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq_ondemand.c

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s,

Quoted 4 lines

index 1911d17..246ae14 100644

drivers/cpufreq/cpufreq_ondemand.c +0 -0
@@ -78,15 +78,10 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,

cpu_dbs_info);

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 78

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +0 -0
 static unsigned int dbs_enable;	/* number of CPUs using

this policy */

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 80

Quoted 3 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -1
 /*
- * DEADLOCK ALERT! There is a ordering requirement between

cpu_hotplug

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 82

Quoted 2 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -2
- * lock and dbs_mutex. cpu_hotplug lock should always be held before
- * dbs_mutex. If any function that can potentially take

cpu_hotplug lock

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 83

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +0 -1
- * (like __cpufreq_driver_target()) is being called with

dbs_mutex taken,
then

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 84

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +0 -1
- * cpu_hotplug lock should be taken before that. Note that

cpu_hotplug lock

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 86

Quoted 2 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -2
- * is recursive for the same process. -Venki
- * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the

dbs_mutex,
because it

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 87

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +0 -1
- * would deadlock with cancel_delayed_work_sync(), which is

needed for
proper

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 80

Quoted 2 lines
drivers/cpufreq/cpufreq_ondemand.c +1 -1
- * raceless workqueue teardown.
+ * dbs_mutex protects data in dbs_tuners_ins from

concurrent changes on

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 81

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +1 -0
+ * different CPUs. It protects dbs_enable in governor

start/stop. It also

drivers/cpufreq/cpufreq_ondemand.c static DEFINE_PER_CPU(struct cpu_dbs_info_s, · line 82

Quoted 1 line
drivers/cpufreq/cpufreq_ondemand.c +1 -0
+ * serializes governor limit_change with do_dbs_timer. We

do not want

drivers/cpufreq/cpufreq_ondemand.c static void do_dbs_timer(struct

Quoted 5 lines
drivers/cpufreq/cpufreq_ondemand.c +1 -0
+ * 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)

drivers/cpufreq/cpufreq_ondemand.c static void do_dbs_timer(struct

Quoted 14 lines
drivers/cpufreq/cpufreq_ondemand.c +2 -3
 	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)

drivers/cpufreq/cpufreq_ondemand.c static void do_dbs_timer(struct work_struct *work)

Quoted 3 lines
drivers/cpufreq/cpufreq_ondemand.c +0 -0
 			dbs_info->freq_lo, CPUFREQ_RELATION_H);
 	}
 	queue_delayed_work_on(cpu, kondemand_wq,

&dbs_info->work, delay);

drivers/cpufreq/cpufreq_ondemand.c static int cpufreq_governor_dbs(struct

Quoted 6 lines
drivers/cpufreq/cpufreq_ondemand.c +1 -1
-	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,

Quoted 24 lines
drivers/cpufreq/cpufreq_ondemand.c +4 -3
 				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);
Signature · 3 lines
1.6.0.6

--
#7 lore raw

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

Quoted 1 line · Venkatesh

...

Quoted 8 lines
Quoted 4 lines

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.

Quoted 4 lines

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

author
#8 lore raw

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

In reply to [email protected]

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 <[email protected]>
---
 Documentation/feature-removal-schedule.txt |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
Documentation/feature-removal-schedule.txt +10 -0
index f8cd450..09e031c 100644
@@ -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/[email protected]>
+
+----------------------------
+
+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 <[email protected]>
Signature · 3 lines
1.6.0.6

--
author
#9 lore raw

PATCH 3/4 cpufreq: Cleanup locking in ondemand governor

In reply to [email protected]

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 <[email protected]>
---
 drivers/cpufreq/cpufreq_ondemand.c |   62 +++++++++++++++--------------------
 1 files changed, 27 insertions(+), 35 deletions(-)
drivers/cpufreq/cpufreq_ondemand.c +27 -35
index 246ae14..d6ba142 100644
@@ -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;
Signature · 3 lines
1.6.0.6

--
author
#10 lore raw

PATCH 4/4 cpufreq: Cleanup locking in conservative governor

In reply to [email protected]

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 <[email protected]>
---
 drivers/cpufreq/cpufreq_conservative.c |   34 ++++++++++++-------------------
 1 files changed, 13 insertions(+), 21 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +13 -21
index 58889f2..5749050 100644
@@ -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;
 	}
Signature · 3 lines
1.6.0.6

--
#11 lore raw

PATCH 1/1 CPUFREQ: fix (utter) cpufreq_add_dev mess v1

In reply to [email protected]

(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/[email protected]>
CC "Pallipadi, Venkatesh" <[email protected]>
CC Dave Jones <[email protected]>,
CC Ingo Molnar <[email protected]>
CC "Rafael J. Wysocki" <[email protected]>
CC Dave Young <[email protected]>
CC Pekka Enberg <penberg-bbCR+/[email protected]>
CC Thomas Renninger <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   65 ++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 25 deletions(-)
drivers/cpufreq/cpufreq.c +40 -25
Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c
===================================================================
@@ -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:
Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
Tested-by on the v1 series
#12 lore raw
In reply to [email protected]
Quoted 8 lines · * [email protected]…

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

Quoted 6 lines

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 <[email protected]>

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

#13 lore raw
Quoted 29 lines · * Ingo Molnar ([email protected]) wrote:
Quoted 8 lines · * [email protected]…

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

Quoted 6 lines

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:

Quoted 6 lines
Tested-by Ingo Molnar <[email protected]>

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

Quoted 3 lines

Ingo

Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
author
#14 lore raw
In reply to Ingo Molnar
Quoted 43 lines

-----Original Message-----
From: Ingo Molnar [mailto:[email protected]]
Sent: Thursday, July 02, 2009 11:54 PM
To: Pallipadi, Venkatesh
Cc: Dave Jones; [email protected]; [email protected]; [email protected]; Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu Desnoyers; Thomas Renninger
Subject: Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

Quoted 8 lines · * [email protected] <[email protected]> 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

Quoted 6 lines

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:

Quoted 7 lines
Tested-by Ingo Molnar <[email protected]>

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

#15 lore raw
Quoted 51 lines · Pallipadi, Venkatesh
Quoted 43 lines

-----Original Message-----
From: Ingo Molnar [mailto:[email protected]]
Sent: Thursday, July 02, 2009 11:54 PM
To: Pallipadi, Venkatesh
Cc: Dave Jones; [email protected]; [email protected]; [email protected]; Rafael J. Wysocki; Dave Young; Pekka Enberg; Mathieu
Desnoyers; Thomas Renninger
Subject: Re: [patch 0/4] Take care of cpufreq lockdep issues (take 2)

Quoted 8 lines · * [email protected]…

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

Quoted 6 lines

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:

Quoted 7 lines
Tested-by Ingo Molnar <[email protected]>

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.

Quoted 4 lines

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

author
#16 lore raw
In reply to [email protected]
Quoted 7 lines · Venkatesh

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

Quoted 7 lines

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