Skip to contentSkip to conversation

cpufreq: governor: Fix potential races

PATCH Applied Viresh Kumar 3 participants 24 messages Jun 3, 2015 – Jun 15, 2015 CPU frequency scaling framework

lore
Share & tools
lore

Viresh Kumar

author maintainer
#0 lore raw

cover cpufreq: governor: Fix potential races

Hi Rafael,

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Rebased over: v4.1-rc6
Tested-on: ARM dual Cortex -A15 Exynos board.

[1] http://marc.info/?i=20150601064031.2972.59208.stgit%40perfhull-ltc.austin.ibm.com

Viresh Kumar (3):
cpufreq: governor: register notifier from cs_init()
cpufreq: governor: split cpufreq_governor_dbs()
cpufreq: governor: Serialize governor callbacks

 drivers/cpufreq/cpufreq_conservative.c |  28 +--
 drivers/cpufreq/cpufreq_governor.c     | 340 ++++++++++++++++++---------------
 drivers/cpufreq/cpufreq_governor.h     |  16 +-
 drivers/cpufreq/cpufreq_ondemand.c     |   6 +-
 4 files changed, 209 insertions(+), 181 deletions(-)
Signature · 1 line
2.4.0
author maintainer
#1 lore raw

PATCH 1/3 cpufreq: governor: register notifier from cs_init()

Notifiers are required only for conservative governor and the common governor code is unnecessarily polluted with that. Handle that from cs_init/exit() instead of cpufreq_governor_dbs().

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq_conservative.c | 26 +++++++++++++++-----------
 drivers/cpufreq/cpufreq_governor.c     | 22 +++-------------------
 drivers/cpufreq/cpufreq_governor.h     |  8 ++------
 drivers/cpufreq/cpufreq_ondemand.c     |  4 ++--
 4 files changed, 22 insertions(+), 38 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +15 -11
index 25a70d06c5bf..75f875bb155e 100644
@@ -148,6 +148,10 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 	return 0;
 }
 
+static struct notifier_block cs_cpufreq_notifier_block = {
+	.notifier_call = dbs_cpufreq_notifier,
+};
+
 /************************** sysfs interface ************************/
 static struct common_dbs_data cs_dbs_cdata;
 

@@ -317,7 +321,7 @@ static struct attribute_group cs_attr_group_gov_pol = {
 
 /************************** sysfs end ************************/
 
-static int cs_init(struct dbs_data *dbs_data)
+static int cs_init(struct dbs_data *dbs_data, bool notify)
 {
 	struct cs_dbs_tuners *tuners;
 

@@ -336,25 +340,26 @@ static int cs_init(struct dbs_data *dbs_data)
 	dbs_data->tuners = tuners;
 	dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
 		jiffies_to_usecs(10);
+
+	if (notify)
+		cpufreq_register_notifier(&cs_cpufreq_notifier_block,
+					  CPUFREQ_TRANSITION_NOTIFIER);
+
 	mutex_init(&dbs_data->mutex);
 	return 0;
 }
 
-static void cs_exit(struct dbs_data *dbs_data)
+static void cs_exit(struct dbs_data *dbs_data, bool notify)
 {
+	if (notify)
+		cpufreq_unregister_notifier(&cs_cpufreq_notifier_block,
+					    CPUFREQ_TRANSITION_NOTIFIER);
+
 	kfree(dbs_data->tuners);
 }
 
 define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
-static struct notifier_block cs_cpufreq_notifier_block = {
-	.notifier_call = dbs_cpufreq_notifier,
-};
-
-static struct cs_ops cs_ops = {
-	.notifier_block = &cs_cpufreq_notifier_block,
-};
-
 static struct common_dbs_data cs_dbs_cdata = {
 	.governor = GOV_CONSERVATIVE,
 	.attr_group_gov_sys = &cs_attr_group_gov_sys,

@@ -363,7 +368,6 @@ static struct common_dbs_data cs_dbs_cdata = {
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = cs_dbs_timer,
 	.gov_check_cpu = cs_check_cpu,
-	.gov_ops = &cs_ops,
 	.init = cs_init,
 	.exit = cs_exit,
 };
drivers/cpufreq/cpufreq_governor.c +3 -19
index 1b44496b2d2b..d64a82e6481a 100644
@@ -278,7 +278,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 
 		dbs_data->cdata = cdata;
 		dbs_data->usage_count = 1;
-		rc = cdata->init(dbs_data);
+		rc = cdata->init(dbs_data, !policy->governor->initialized);
 		if (rc) {
 			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
 			kfree(dbs_data);

@@ -291,7 +291,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		rc = sysfs_create_group(get_governor_parent_kobj(policy),
 				get_sysfs_attr(dbs_data));
 		if (rc) {
-			cdata->exit(dbs_data);
+			cdata->exit(dbs_data, !policy->governor->initialized);
 			kfree(dbs_data);
 			return rc;
 		}

@@ -309,14 +309,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
 					latency * LATENCY_MULTIPLIER));
 
-		if ((cdata->governor == GOV_CONSERVATIVE) &&
-				(!policy->governor->initialized)) {
-			struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
-
-			cpufreq_register_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
-		}
-
 		if (!have_governor_per_policy())
 			cdata->gdbs_data = dbs_data;
 

@@ -329,15 +321,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 			if (!have_governor_per_policy())
 				cpufreq_put_global_kobject();
 
-			if ((dbs_data->cdata->governor == GOV_CONSERVATIVE) &&
-				(policy->governor->initialized == 1)) {
-				struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
-
-				cpufreq_unregister_notifier(cs_ops->notifier_block,
-						CPUFREQ_TRANSITION_NOTIFIER);
-			}
-
-			cdata->exit(dbs_data);
+			cdata->exit(dbs_data, policy->governor->initialized == 1);
 			kfree(dbs_data);
 			cdata->gdbs_data = NULL;
 		}
drivers/cpufreq/cpufreq_governor.h +2 -6
index cc401d147e72..1690120df487 100644
@@ -208,8 +208,8 @@ struct common_dbs_data {
 	void *(*get_cpu_dbs_info_s)(int cpu);
 	void (*gov_dbs_timer)(struct work_struct *work);
 	void (*gov_check_cpu)(int cpu, unsigned int load);
-	int (*init)(struct dbs_data *dbs_data);
-	void (*exit)(struct dbs_data *dbs_data);
+	int (*init)(struct dbs_data *dbs_data, bool notify);
+	void (*exit)(struct dbs_data *dbs_data, bool notify);
 
 	/* Governor specific ops, see below */
 	void *gov_ops;

@@ -234,10 +234,6 @@ struct od_ops {
 	void (*freq_increase)(struct cpufreq_policy *policy, unsigned int freq);
 };
 
-struct cs_ops {
-	struct notifier_block *notifier_block;
-};
-
 static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 {
 	int delay = usecs_to_jiffies(sampling_rate);
drivers/cpufreq/cpufreq_ondemand.c +2 -2
index ad3f38fd3eb9..4fe78a9caa04 100644
@@ -475,7 +475,7 @@ static struct attribute_group od_attr_group_gov_pol = {
 
 /************************** sysfs end ************************/
 
-static int od_init(struct dbs_data *dbs_data)
+static int od_init(struct dbs_data *dbs_data, bool notify)
 {
 	struct od_dbs_tuners *tuners;
 	u64 idle_time;

@@ -517,7 +517,7 @@ static int od_init(struct dbs_data *dbs_data)
 	return 0;
 }
 
-static void od_exit(struct dbs_data *dbs_data)
+static void od_exit(struct dbs_data *dbs_data, bool notify)
 {
 	kfree(dbs_data->tuners);
 }
Signature · 1 line
2.4.0
#2 lore raw

Re: [PATCH 1/3] cpufreq: governor: register notifier from cs_init()

Quoted 123 lines · Viresh Kumar

Notifiers are required only for conservative governor and the common governor code is unnecessarily polluted with that. Handle that from cs_init/exit() instead of cpufreq_governor_dbs().

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq_conservative.c | 26 +++++++++++++++-----------
 drivers/cpufreq/cpufreq_governor.c     | 22 +++-------------------
 drivers/cpufreq/cpufreq_governor.h     |  8 ++------
 drivers/cpufreq/cpufreq_ondemand.c     |  4 ++--
 4 files changed, 22 insertions(+), 38 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +15 -11
index 25a70d06c5bf..75f875bb155e 100644
@@ -148,6 +148,10 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 	return 0;
 }
 
+static struct notifier_block cs_cpufreq_notifier_block = {
+	.notifier_call = dbs_cpufreq_notifier,
+};
+
 /************************** sysfs interface ************************/
 static struct common_dbs_data cs_dbs_cdata;
 

@@ -317,7 +321,7 @@ static struct attribute_group cs_attr_group_gov_pol = {
 
 /************************** sysfs end ************************/
 
-static int cs_init(struct dbs_data *dbs_data)
+static int cs_init(struct dbs_data *dbs_data, bool notify)
 {
 	struct cs_dbs_tuners *tuners;
 

@@ -336,25 +340,26 @@ static int cs_init(struct dbs_data *dbs_data)
 	dbs_data->tuners = tuners;
 	dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
 		jiffies_to_usecs(10);
+
+	if (notify)
+		cpufreq_register_notifier(&cs_cpufreq_notifier_block,
+					  CPUFREQ_TRANSITION_NOTIFIER);
+
 	mutex_init(&dbs_data->mutex);
 	return 0;
 }
 
-static void cs_exit(struct dbs_data *dbs_data)
+static void cs_exit(struct dbs_data *dbs_data, bool notify)
 {
+	if (notify)
+		cpufreq_unregister_notifier(&cs_cpufreq_notifier_block,
+					    CPUFREQ_TRANSITION_NOTIFIER);
+
 	kfree(dbs_data->tuners);
 }
 
 define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
-static struct notifier_block cs_cpufreq_notifier_block = {
-	.notifier_call = dbs_cpufreq_notifier,
-};
-
-static struct cs_ops cs_ops = {
-	.notifier_block = &cs_cpufreq_notifier_block,
-};
-
 static struct common_dbs_data cs_dbs_cdata = {
 	.governor = GOV_CONSERVATIVE,
 	.attr_group_gov_sys = &cs_attr_group_gov_sys,

@@ -363,7 +368,6 @@ static struct common_dbs_data cs_dbs_cdata = {
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = cs_dbs_timer,
 	.gov_check_cpu = cs_check_cpu,
-	.gov_ops = &cs_ops,
 	.init = cs_init,
 	.exit = cs_exit,
 };
drivers/cpufreq/cpufreq_governor.c +2 -12
index 1b44496b2d2b..d64a82e6481a 100644
@@ -278,7 +278,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 
 		dbs_data->cdata = cdata;
 		dbs_data->usage_count = 1;
-		rc = cdata->init(dbs_data);
+		rc = cdata->init(dbs_data, !policy->governor->initialized);
 		if (rc) {
 			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
 			kfree(dbs_data);

@@ -291,7 +291,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		rc = sysfs_create_group(get_governor_parent_kobj(policy),
 				get_sysfs_attr(dbs_data));
 		if (rc) {
-			cdata->exit(dbs_data);
+			cdata->exit(dbs_data, !policy->governor->initialized);
 			kfree(dbs_data);
 			return rc;
 		}

@@ -309,14 +309,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
 					latency * LATENCY_MULTIPLIER));
 
-		if ((cdata->governor == GOV_CONSERVATIVE) &&
-				(!policy->governor->initialized)) {
-			struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
-
-			cpufreq_register_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
-		}
-
 		if (!have_governor_per_policy())
 			cdata->gdbs_data = dbs_data;
 

@@ -329,15 +321,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 			if (!have_governor_per_policy())
 				cpufreq_put_global_kobject();
 
-			if ((dbs_data->cdata->governor == GOV_CONSERVATIVE) &&
-				(policy->governor->initialized == 1)) {

drivers/cpufreq/cpufreq_governor.c int cpufreq_governor_dbs(struct cpufreq_policy *policy, · line 326

Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +1 -7
-				struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
-
-				cpufreq_unregister_notifier(cs_ops->notifier_block,
-						CPUFREQ_TRANSITION_NOTIFIER);
-			}
-
-			cdata->exit(dbs_data);
+			cdata->exit(dbs_data, policy->governor->initialized == 1);
 			kfree(dbs_data);
 			cdata->gdbs_data = NULL;

I don't see why we need the check on policy->governor->initialized because we call cdata->init() and cdata->exit(), *only* when the first and last references to the governor are being made respectively (filtered by dbs_data->usage_count), which is precisely what the initialized flag checks. So passing policy->governor->initialized seems to be redundant? And this is the case for both gov_per_policy and otherwise.

Regards
Preeti U Murthy

author maintainer
#3 lore raw

Re: [PATCH 1/3] cpufreq: governor: register notifier from cs_init()

Quoted 6 lines · Preeti U Murthy

I don't see why we need the check on policy->governor->initialized because we call cdata->init() and cdata->exit(), *only* when the first and last references to the governor are being made respectively (filtered by dbs_data->usage_count), which is precisely what the initialized flag checks. So passing policy->governor->initialized seems to be redundant? And this is the case for both gov_per_policy and otherwise.

That's the case only for !gov_per_policy.

In case of gov_per_policy, the same governor is used by multiple policies but with different dbs_data objects. And in this case INIT will be called only once for a dbs_data and so usage count will be max 1 at any time. But we want to register the notifiers for a governor only once and so we need this extra check.

policy->governor->initialized is set to 1 when the governor is initialized for the first policy. For all others it is incremented to other values.

Signature · 1 line
viresh
Reviewed-by
#4 lore raw

Re: [PATCH 1/3] cpufreq: governor: register notifier from cs_init()

Quoted 16 lines · Viresh Kumar
Quoted 6 lines · Preeti U Murthy

I don't see why we need the check on policy->governor->initialized because we call cdata->init() and cdata->exit(), *only* when the first and last references to the governor are being made respectively (filtered by dbs_data->usage_count), which is precisely what the initialized flag checks. So passing policy->governor->initialized seems to be redundant? And this is the case for both gov_per_policy and otherwise.

That's the case only for !gov_per_policy.

In case of gov_per_policy, the same governor is used by multiple policies but with different dbs_data objects. And in this case INIT will be called only once for a dbs_data and so usage count will be max 1 at any time. But we want to register the notifiers for a governor only once and so we need this extra check.

Quoted 3 lines

policy->governor->initialized is set to 1 when the governor is initialized for the first policy. For all others it is incremented to other values.

Ok I see. The patch looks good to me then.

Reviewed-by Preeti U Murthy <[email protected]>
author maintainer
#5 lore raw

PATCH 2/3 cpufreq: governor: split cpufreq_governor_dbs()

In reply to Viresh Kumar

cpufreq_governor_dbs() is hardly readable, it is just too big and complicated. Lets make it more readable by splitting out event specific routines.

Order of statements is changed at few places, but that shouldn't bring any functional change.

Signed-off-by Viresh Kumar <[email protected]>

---
Best way to verify the changes here is to keep both copies of code side by side and comparing it event wise.

 drivers/cpufreq/cpufreq_governor.c | 326 +++++++++++++++++++++----------------
 1 file changed, 185 insertions(+), 141 deletions(-)
drivers/cpufreq/cpufreq_governor.c +185 -141
index d64a82e6481a..dc382a5a2158 100644
@@ -239,195 +239,239 @@ static void set_sampling_rate(struct dbs_data *dbs_data,
 	}
 }
 
-int cpufreq_governor_dbs(struct cpufreq_policy *policy,
-		struct common_dbs_data *cdata, unsigned int event)
+static int cpufreq_governor_init(struct cpufreq_policy *policy,
+				 struct dbs_data *dbs_data,
+				 struct common_dbs_data *cdata)
 {
-	struct dbs_data *dbs_data;
-	struct od_cpu_dbs_info_s *od_dbs_info = NULL;
-	struct cs_cpu_dbs_info_s *cs_dbs_info = NULL;
-	struct od_ops *od_ops = NULL;
-	struct od_dbs_tuners *od_tuners = NULL;
-	struct cs_dbs_tuners *cs_tuners = NULL;
-	struct cpu_dbs_common_info *cpu_cdbs;
-	unsigned int sampling_rate, latency, ignore_nice, j, cpu = policy->cpu;
-	int io_busy = 0;
-	int rc;
+	unsigned int latency;
+	int ret;
 
-	if (have_governor_per_policy())
-		dbs_data = policy->governor_data;
-	else
-		dbs_data = cdata->gdbs_data;
+	if (dbs_data) {
+		WARN_ON(have_governor_per_policy());
+		dbs_data->usage_count++;
+		policy->governor_data = dbs_data;
+		return 0;
+	}
 
-	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+	dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
+	if (!dbs_data)
+		return -ENOMEM;
 
-	switch (event) {
-	case CPUFREQ_GOV_POLICY_INIT:
-		if (have_governor_per_policy()) {
-			WARN_ON(dbs_data);
-		} else if (dbs_data) {
-			dbs_data->usage_count++;
-			policy->governor_data = dbs_data;
-			return 0;
-		}
+	dbs_data->cdata = cdata;
+	dbs_data->usage_count = 1;
 
-		dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
-		if (!dbs_data) {
-			pr_err("%s: POLICY_INIT: kzalloc failed\n", __func__);
-			return -ENOMEM;
-		}
+	ret = cdata->init(dbs_data, !policy->governor->initialized);
+	if (ret)
+		goto free_dbs_data;
 
-		dbs_data->cdata = cdata;
-		dbs_data->usage_count = 1;
-		rc = cdata->init(dbs_data, !policy->governor->initialized);
-		if (rc) {
-			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
-			kfree(dbs_data);
-			return rc;
-		}
+	/* policy latency is in ns. Convert it to us first */
+	latency = policy->cpuinfo.transition_latency / 1000;
+	if (latency == 0)
+		latency = 1;
 
-		if (!have_governor_per_policy())
-			WARN_ON(cpufreq_get_global_kobject());
+	/* Bring kernel and HW constraints together */
+	dbs_data->min_sampling_rate = max(dbs_data->min_sampling_rate,
+					  MIN_LATENCY_MULTIPLIER * latency);
+	set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
+					latency * LATENCY_MULTIPLIER));
 
-		rc = sysfs_create_group(get_governor_parent_kobj(policy),
-				get_sysfs_attr(dbs_data));
-		if (rc) {
-			cdata->exit(dbs_data, !policy->governor->initialized);
-			kfree(dbs_data);
-			return rc;
-		}
+	if (!have_governor_per_policy()) {
+		WARN_ON(cpufreq_get_global_kobject());
+		cdata->gdbs_data = dbs_data;
+	}
 
-		policy->governor_data = dbs_data;
+	ret = sysfs_create_group(get_governor_parent_kobj(policy),
+				 get_sysfs_attr(dbs_data));
+	if (ret)
+		goto cdata_exit;
 
-		/* policy latency is in ns. Convert it to us first */
-		latency = policy->cpuinfo.transition_latency / 1000;
-		if (latency == 0)
-			latency = 1;
+	policy->governor_data = dbs_data;
 
-		/* Bring kernel and HW constraints together */
-		dbs_data->min_sampling_rate = max(dbs_data->min_sampling_rate,
-				MIN_LATENCY_MULTIPLIER * latency);
-		set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
-					latency * LATENCY_MULTIPLIER));
+	return 0;
 
-		if (!have_governor_per_policy())
-			cdata->gdbs_data = dbs_data;
+cdata_exit:
+	if (!have_governor_per_policy()) {
+		cdata->gdbs_data = NULL;
+		cpufreq_put_global_kobject();
+	}
+	cdata->exit(dbs_data, !policy->governor->initialized);
+free_dbs_data:
+	kfree(dbs_data);
+	return ret;
+}
 
-		return 0;
-	case CPUFREQ_GOV_POLICY_EXIT:
-		if (!--dbs_data->usage_count) {
-			sysfs_remove_group(get_governor_parent_kobj(policy),
-					get_sysfs_attr(dbs_data));
+static void cpufreq_governor_exit(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
 
-			if (!have_governor_per_policy())
-				cpufreq_put_global_kobject();
+	policy->governor_data = NULL;
+	if (!--dbs_data->usage_count) {
+		sysfs_remove_group(get_governor_parent_kobj(policy),
+				   get_sysfs_attr(dbs_data));
 
-			cdata->exit(dbs_data, policy->governor->initialized == 1);
-			kfree(dbs_data);
+		if (!have_governor_per_policy()) {
 			cdata->gdbs_data = NULL;
+			cpufreq_put_global_kobject();
 		}
 
-		policy->governor_data = NULL;
-		return 0;
+		cdata->exit(dbs_data, policy->governor->initialized == 1);
+		kfree(dbs_data);
 	}
+}
 
-	cpu_cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+static int cpufreq_governor_start(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
+	int io_busy = 0;
+
+	if (!policy->cur)
+		return -EINVAL;
+
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 
-	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
-		cs_tuners = dbs_data->tuners;
-		cs_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
 		sampling_rate = cs_tuners->sampling_rate;
 		ignore_nice = cs_tuners->ignore_nice_load;
 	} else {
-		od_tuners = dbs_data->tuners;
-		od_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
+		struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+
 		sampling_rate = od_tuners->sampling_rate;
 		ignore_nice = od_tuners->ignore_nice_load;
-		od_ops = dbs_data->cdata->gov_ops;
 		io_busy = od_tuners->io_is_busy;
 	}
 
-	switch (event) {
-	case CPUFREQ_GOV_START:
-		if (!policy->cur)
-			return -EINVAL;
+	mutex_lock(&dbs_data->mutex);
 
-		mutex_lock(&dbs_data->mutex);
+	for_each_cpu(j, policy->cpus) {
+		struct cpu_dbs_common_info *j_cdbs = cdata->get_cpu_cdbs(j);
+		unsigned int prev_load;
 
-		for_each_cpu(j, policy->cpus) {
-			struct cpu_dbs_common_info *j_cdbs =
-				dbs_data->cdata->get_cpu_cdbs(j);
-			unsigned int prev_load;
+		j_cdbs->cpu = j;
+		j_cdbs->cur_policy = policy;
+		j_cdbs->prev_cpu_idle =
+			get_cpu_idle_time(j, &j_cdbs->prev_cpu_wall, io_busy);
 
-			j_cdbs->cpu = j;
-			j_cdbs->cur_policy = policy;
-			j_cdbs->prev_cpu_idle = get_cpu_idle_time(j,
-					       &j_cdbs->prev_cpu_wall, io_busy);
+		prev_load = (unsigned int)(j_cdbs->prev_cpu_wall -
+					    j_cdbs->prev_cpu_idle);
+		j_cdbs->prev_load = 100 * prev_load /
+				    (unsigned int)j_cdbs->prev_cpu_wall;
 
-			prev_load = (unsigned int)
-				(j_cdbs->prev_cpu_wall - j_cdbs->prev_cpu_idle);
-			j_cdbs->prev_load = 100 * prev_load /
-					(unsigned int) j_cdbs->prev_cpu_wall;
+		if (ignore_nice)
+			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-			if (ignore_nice)
-				j_cdbs->prev_cpu_nice =
-					kcpustat_cpu(j).cpustat[CPUTIME_NICE];
+		mutex_init(&j_cdbs->timer_mutex);
+		INIT_DEFERRABLE_WORK(&j_cdbs->work, cdata->gov_dbs_timer);
+	}
 
-			mutex_init(&j_cdbs->timer_mutex);
-			INIT_DEFERRABLE_WORK(&j_cdbs->work,
-					     dbs_data->cdata->gov_dbs_timer);
-		}
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_cpu_dbs_info_s *cs_dbs_info =
+			cdata->get_cpu_dbs_info_s(cpu);
 
-		if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
-			cs_dbs_info->down_skip = 0;
-			cs_dbs_info->enable = 1;
-			cs_dbs_info->requested_freq = policy->cur;
-		} else {
-			od_dbs_info->rate_mult = 1;
-			od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
-			od_ops->powersave_bias_init_cpu(cpu);
-		}
+		cs_dbs_info->down_skip = 0;
+		cs_dbs_info->enable = 1;
+		cs_dbs_info->requested_freq = policy->cur;
+	} else {
+		struct od_ops *od_ops = cdata->gov_ops;
+		struct od_cpu_dbs_info_s *od_dbs_info = cdata->get_cpu_dbs_info_s(cpu);
 
-		mutex_unlock(&dbs_data->mutex);
+		od_dbs_info->rate_mult = 1;
+		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
+		od_ops->powersave_bias_init_cpu(cpu);
+	}
 
-		/* Initiate timer time stamp */
-		cpu_cdbs->time_stamp = ktime_get();
+	mutex_unlock(&dbs_data->mutex);
 
-		gov_queue_work(dbs_data, policy,
-				delay_for_sampling_rate(sampling_rate), true);
-		break;
+	/* Initiate timer time stamp */
+	cpu_cdbs->time_stamp = ktime_get();
 
-	case CPUFREQ_GOV_STOP:
-		if (dbs_data->cdata->governor == GOV_CONSERVATIVE)
-			cs_dbs_info->enable = 0;
+	gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate),
+		       true);
+	return 0;
+}
+
+static void cpufreq_governor_stop(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
+
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_cpu_dbs_info_s *cs_dbs_info =
+			cdata->get_cpu_dbs_info_s(cpu);
 
-		gov_cancel_work(dbs_data, policy);
+		cs_dbs_info->enable = 0;
+	}
+
+	gov_cancel_work(dbs_data, policy);
+
+	mutex_lock(&dbs_data->mutex);
+	mutex_destroy(&cpu_cdbs->timer_mutex);
+	cpu_cdbs->cur_policy = NULL;
+	mutex_unlock(&dbs_data->mutex);
+}
 
-		mutex_lock(&dbs_data->mutex);
-		mutex_destroy(&cpu_cdbs->timer_mutex);
-		cpu_cdbs->cur_policy = NULL;
+static void cpufreq_governor_limits(struct cpufreq_policy *policy,
+				    struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
 
+	mutex_lock(&dbs_data->mutex);
+	if (!cpu_cdbs->cur_policy) {
 		mutex_unlock(&dbs_data->mutex);
+		return;
+	}
 
-		break;
+	mutex_lock(&cpu_cdbs->timer_mutex);
+	if (policy->max < cpu_cdbs->cur_policy->cur)
+		__cpufreq_driver_target(cpu_cdbs->cur_policy, policy->max,
+					CPUFREQ_RELATION_H);
+	else if (policy->min > cpu_cdbs->cur_policy->cur)
+		__cpufreq_driver_target(cpu_cdbs->cur_policy, policy->min,
+					CPUFREQ_RELATION_L);
+	dbs_check_cpu(dbs_data, cpu);
+	mutex_unlock(&cpu_cdbs->timer_mutex);
+
+	mutex_unlock(&dbs_data->mutex);
+}
 
+int cpufreq_governor_dbs(struct cpufreq_policy *policy,
+			 struct common_dbs_data *cdata, unsigned int event)
+{
+	struct dbs_data *dbs_data;
+	int ret = 0;
+
+	if (have_governor_per_policy())
+		dbs_data = policy->governor_data;
+	else
+		dbs_data = cdata->gdbs_data;
+
+	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+
+	switch (event) {
+	case CPUFREQ_GOV_POLICY_INIT:
+		ret = cpufreq_governor_init(policy, dbs_data, cdata);
+		break;
+	case CPUFREQ_GOV_POLICY_EXIT:
+		cpufreq_governor_exit(policy, dbs_data);
+		break;
+	case CPUFREQ_GOV_START:
+		ret = cpufreq_governor_start(policy, dbs_data);
+		break;
+	case CPUFREQ_GOV_STOP:
+		cpufreq_governor_stop(policy, dbs_data);
+		break;
 	case CPUFREQ_GOV_LIMITS:
-		mutex_lock(&dbs_data->mutex);
-		if (!cpu_cdbs->cur_policy) {
-			mutex_unlock(&dbs_data->mutex);
-			break;
-		}
-		mutex_lock(&cpu_cdbs->timer_mutex);
-		if (policy->max < cpu_cdbs->cur_policy->cur)
-			__cpufreq_driver_target(cpu_cdbs->cur_policy,
-					policy->max, CPUFREQ_RELATION_H);
-		else if (policy->min > cpu_cdbs->cur_policy->cur)
-			__cpufreq_driver_target(cpu_cdbs->cur_policy,
-					policy->min, CPUFREQ_RELATION_L);
-		dbs_check_cpu(dbs_data, cpu);
-		mutex_unlock(&cpu_cdbs->timer_mutex);
-		mutex_unlock(&dbs_data->mutex);
+		cpufreq_governor_limits(policy, dbs_data);
 		break;
 	}
-	return 0;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_governor_dbs);
Signature · 1 line
2.4.0
#6 lore raw

Re: [PATCH 2/3] cpufreq: governor: split cpufreq_governor_dbs()

Quoted 38 lines · Viresh Kumar

cpufreq_governor_dbs() is hardly readable, it is just too big and complicated. Lets make it more readable by splitting out event specific routines.

Order of statements is changed at few places, but that shouldn't bring any functional change.

Signed-off-by Viresh Kumar <[email protected]>

---
Best way to verify the changes here is to keep both copies of code side by side and comparing it event wise.

 drivers/cpufreq/cpufreq_governor.c | 326 +++++++++++++++++++++----------------
 1 file changed, 185 insertions(+), 141 deletions(-)
drivers/cpufreq/cpufreq_governor.c +3 -11
index d64a82e6481a..dc382a5a2158 100644
@@ -239,195 +239,239 @@ static void set_sampling_rate(struct dbs_data *dbs_data,
 	}
 }
 
-int cpufreq_governor_dbs(struct cpufreq_policy *policy,
-		struct common_dbs_data *cdata, unsigned int event)
+static int cpufreq_governor_init(struct cpufreq_policy *policy,
+				 struct dbs_data *dbs_data,
+				 struct common_dbs_data *cdata)
 {
-	struct dbs_data *dbs_data;
-	struct od_cpu_dbs_info_s *od_dbs_info = NULL;
-	struct cs_cpu_dbs_info_s *cs_dbs_info = NULL;
-	struct od_ops *od_ops = NULL;
-	struct od_dbs_tuners *od_tuners = NULL;
-	struct cs_dbs_tuners *cs_tuners = NULL;
-	struct cpu_dbs_common_info *cpu_cdbs;
-	unsigned int sampling_rate, latency, ignore_nice, j, cpu = policy->cpu;
-	int io_busy = 0;

drivers/cpufreq/cpufreq_governor.c static void set_sampling_rate(struct dbs_data *dbs_data, · line 250

Quoted 10 lines
drivers/cpufreq/cpufreq_governor.c +4 -5
-	int rc;
+	unsigned int latency;
+	int ret;
 
-	if (have_governor_per_policy())
-		dbs_data = policy->governor_data;
-	else
-		dbs_data = cdata->gdbs_data;
+	if (dbs_data) {
+		WARN_ON(have_governor_per_policy());

Shouldn't this be outside this loop ? We warn here and allocate dbs_dta freshly in the current code for the case where governor is per policy.

drivers/cpufreq/cpufreq_governor.c static void set_sampling_rate(struct dbs_data *dbs_data, · line 251

Quoted 1 line
drivers/cpufreq/cpufreq_governor.c +1 -0
+		dbs_data->usage_count++;

Besides, in the case where a governor exists per policy, we will end up incrementing the usage_count to more than 1 under this condition, which does not make sense.

drivers/cpufreq/cpufreq_governor.c static void set_sampling_rate(struct dbs_data *dbs_data, · line 254

Quoted 3 lines
drivers/cpufreq/cpufreq_governor.c +3 -0
+		policy->governor_data = dbs_data;
+		return 0;
+	}

Regards
Preeti U Murthy

author maintainer
#7 lore raw

Re: [PATCH 2/3] cpufreq: governor: split cpufreq_governor_dbs()

Quoted 4 lines · Preeti U Murthy

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (dbs_data) {
+		WARN_ON(have_governor_per_policy());

Shouldn't this be outside this loop ? We warn here and allocate dbs_dta

Loop ? Its just an 'if' block :)

Quoted 1 line

freshly in the current code for the case where governor is per policy.

So what we are doing in the current code is equally disgusting. We already have a pointer and we overwrite it.

Quoted 6 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+		dbs_data->usage_count++;

Besides, in the case where a governor exists per policy, we will end up incrementing the usage_count to more than 1 under this condition, which does not make sense.

So, the only sane option here is to return an error immediately I think.

drivers/cpufreq/cpufreq_governor.c +9 -4
index ed849a8777dd..57a39f8a92b7 100644
@@ -247,7 +247,8 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 	int ret;
 
 	if (dbs_data) {
-		WARN_ON(have_governor_per_policy());
+		if (WARN_ON(have_governor_per_policy()))
+			return -EINVAL;
 		dbs_data->usage_count++;
 		policy->governor_data = dbs_data;
 		return 0;

@@ -276,24 +277,28 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 					latency * LATENCY_MULTIPLIER));
 
 	if (!have_governor_per_policy()) {
-		WARN_ON(cpufreq_get_global_kobject());
+		if (WARN_ON(cpufreq_get_global_kobject())) {
+			ret = -EINVAL;
+			goto cdata_exit;
+		}
 		cdata->gdbs_data = dbs_data;
 	}
 
 	ret = sysfs_create_group(get_governor_parent_kobj(policy),
 				 get_sysfs_attr(dbs_data));
 	if (ret)
-		goto cdata_exit;
+		goto put_kobj;
 
 	policy->governor_data = dbs_data;
 
 	return 0;
 
-cdata_exit:
+put_kobj:
 	if (!have_governor_per_policy()) {
 		cdata->gdbs_data = NULL;
 		cpufreq_put_global_kobject();
 	}
+cdata_exit:
 	cdata->exit(dbs_data, !policy->governor->initialized);
 free_dbs_data:
 	kfree(dbs_data);
Signature · 1 line
viresh
author maintainer
#8 lore raw

PATCH v2 2/3 cpufreq: governor: split cpufreq_governor_dbs()

In reply to Viresh Kumar

cpufreq_governor_dbs() is hardly readable, it is just too big and complicated. Lets make it more readable by splitting out event specific routines.

Order of statements is changed at few places, but that shouldn't bring any functional change.

Signed-off-by Viresh Kumar <[email protected]>

---
V1->V2: Return errors after hitting WARN_ON. (Preeti)

 drivers/cpufreq/cpufreq_governor.c | 329 +++++++++++++++++++++----------------
 1 file changed, 189 insertions(+), 140 deletions(-)
drivers/cpufreq/cpufreq_governor.c +189 -140
index d64a82e6481a..ccf6ce7e5983 100644
@@ -239,195 +239,244 @@ static void set_sampling_rate(struct dbs_data *dbs_data,
 	}
 }
 
-int cpufreq_governor_dbs(struct cpufreq_policy *policy,
-		struct common_dbs_data *cdata, unsigned int event)
+static int cpufreq_governor_init(struct cpufreq_policy *policy,
+				 struct dbs_data *dbs_data,
+				 struct common_dbs_data *cdata)
 {
-	struct dbs_data *dbs_data;
-	struct od_cpu_dbs_info_s *od_dbs_info = NULL;
-	struct cs_cpu_dbs_info_s *cs_dbs_info = NULL;
-	struct od_ops *od_ops = NULL;
-	struct od_dbs_tuners *od_tuners = NULL;
-	struct cs_dbs_tuners *cs_tuners = NULL;
-	struct cpu_dbs_common_info *cpu_cdbs;
-	unsigned int sampling_rate, latency, ignore_nice, j, cpu = policy->cpu;
-	int io_busy = 0;
-	int rc;
+	unsigned int latency;
+	int ret;
 
-	if (have_governor_per_policy())
-		dbs_data = policy->governor_data;
-	else
-		dbs_data = cdata->gdbs_data;
+	if (dbs_data) {
+		if (WARN_ON(have_governor_per_policy()))
+			return -EINVAL;
+		dbs_data->usage_count++;
+		policy->governor_data = dbs_data;
+		return 0;
+	}
 
-	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+	dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
+	if (!dbs_data)
+		return -ENOMEM;
 
-	switch (event) {
-	case CPUFREQ_GOV_POLICY_INIT:
-		if (have_governor_per_policy()) {
-			WARN_ON(dbs_data);
-		} else if (dbs_data) {
-			dbs_data->usage_count++;
-			policy->governor_data = dbs_data;
-			return 0;
-		}
+	dbs_data->cdata = cdata;
+	dbs_data->usage_count = 1;
 
-		dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
-		if (!dbs_data) {
-			pr_err("%s: POLICY_INIT: kzalloc failed\n", __func__);
-			return -ENOMEM;
-		}
+	ret = cdata->init(dbs_data, !policy->governor->initialized);
+	if (ret)
+		goto free_dbs_data;
 
-		dbs_data->cdata = cdata;
-		dbs_data->usage_count = 1;
-		rc = cdata->init(dbs_data, !policy->governor->initialized);
-		if (rc) {
-			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
-			kfree(dbs_data);
-			return rc;
-		}
+	/* policy latency is in ns. Convert it to us first */
+	latency = policy->cpuinfo.transition_latency / 1000;
+	if (latency == 0)
+		latency = 1;
 
-		if (!have_governor_per_policy())
-			WARN_ON(cpufreq_get_global_kobject());
+	/* Bring kernel and HW constraints together */
+	dbs_data->min_sampling_rate = max(dbs_data->min_sampling_rate,
+					  MIN_LATENCY_MULTIPLIER * latency);
+	set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
+					latency * LATENCY_MULTIPLIER));
 
-		rc = sysfs_create_group(get_governor_parent_kobj(policy),
-				get_sysfs_attr(dbs_data));
-		if (rc) {
-			cdata->exit(dbs_data, !policy->governor->initialized);
-			kfree(dbs_data);
-			return rc;
+	if (!have_governor_per_policy()) {
+		if (WARN_ON(cpufreq_get_global_kobject())) {
+			ret = -EINVAL;
+			goto cdata_exit;
 		}
+		cdata->gdbs_data = dbs_data;
+	}
 
-		policy->governor_data = dbs_data;
+	ret = sysfs_create_group(get_governor_parent_kobj(policy),
+				 get_sysfs_attr(dbs_data));
+	if (ret)
+		goto put_kobj;
 
-		/* policy latency is in ns. Convert it to us first */
-		latency = policy->cpuinfo.transition_latency / 1000;
-		if (latency == 0)
-			latency = 1;
+	policy->governor_data = dbs_data;
 
-		/* Bring kernel and HW constraints together */
-		dbs_data->min_sampling_rate = max(dbs_data->min_sampling_rate,
-				MIN_LATENCY_MULTIPLIER * latency);
-		set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
-					latency * LATENCY_MULTIPLIER));
+	return 0;
 
-		if (!have_governor_per_policy())
-			cdata->gdbs_data = dbs_data;
+put_kobj:
+	if (!have_governor_per_policy()) {
+		cdata->gdbs_data = NULL;
+		cpufreq_put_global_kobject();
+	}
+cdata_exit:
+	cdata->exit(dbs_data, !policy->governor->initialized);
+free_dbs_data:
+	kfree(dbs_data);
+	return ret;
+}
 
-		return 0;
-	case CPUFREQ_GOV_POLICY_EXIT:
-		if (!--dbs_data->usage_count) {
-			sysfs_remove_group(get_governor_parent_kobj(policy),
-					get_sysfs_attr(dbs_data));
+static void cpufreq_governor_exit(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
 
-			if (!have_governor_per_policy())
-				cpufreq_put_global_kobject();
+	policy->governor_data = NULL;
+	if (!--dbs_data->usage_count) {
+		sysfs_remove_group(get_governor_parent_kobj(policy),
+				   get_sysfs_attr(dbs_data));
 
-			cdata->exit(dbs_data, policy->governor->initialized == 1);
-			kfree(dbs_data);
+		if (!have_governor_per_policy()) {
 			cdata->gdbs_data = NULL;
+			cpufreq_put_global_kobject();
 		}
 
-		policy->governor_data = NULL;
-		return 0;
+		cdata->exit(dbs_data, policy->governor->initialized == 1);
+		kfree(dbs_data);
 	}
+}
 
-	cpu_cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+static int cpufreq_governor_start(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
+	int io_busy = 0;
+
+	if (!policy->cur)
+		return -EINVAL;
+
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 
-	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
-		cs_tuners = dbs_data->tuners;
-		cs_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
 		sampling_rate = cs_tuners->sampling_rate;
 		ignore_nice = cs_tuners->ignore_nice_load;
 	} else {
-		od_tuners = dbs_data->tuners;
-		od_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
+		struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+
 		sampling_rate = od_tuners->sampling_rate;
 		ignore_nice = od_tuners->ignore_nice_load;
-		od_ops = dbs_data->cdata->gov_ops;
 		io_busy = od_tuners->io_is_busy;
 	}
 
-	switch (event) {
-	case CPUFREQ_GOV_START:
-		if (!policy->cur)
-			return -EINVAL;
+	mutex_lock(&dbs_data->mutex);
 
-		mutex_lock(&dbs_data->mutex);
+	for_each_cpu(j, policy->cpus) {
+		struct cpu_dbs_common_info *j_cdbs = cdata->get_cpu_cdbs(j);
+		unsigned int prev_load;
 
-		for_each_cpu(j, policy->cpus) {
-			struct cpu_dbs_common_info *j_cdbs =
-				dbs_data->cdata->get_cpu_cdbs(j);
-			unsigned int prev_load;
+		j_cdbs->cpu = j;
+		j_cdbs->cur_policy = policy;
+		j_cdbs->prev_cpu_idle =
+			get_cpu_idle_time(j, &j_cdbs->prev_cpu_wall, io_busy);
 
-			j_cdbs->cpu = j;
-			j_cdbs->cur_policy = policy;
-			j_cdbs->prev_cpu_idle = get_cpu_idle_time(j,
-					       &j_cdbs->prev_cpu_wall, io_busy);
+		prev_load = (unsigned int)(j_cdbs->prev_cpu_wall -
+					    j_cdbs->prev_cpu_idle);
+		j_cdbs->prev_load = 100 * prev_load /
+				    (unsigned int)j_cdbs->prev_cpu_wall;
 
-			prev_load = (unsigned int)
-				(j_cdbs->prev_cpu_wall - j_cdbs->prev_cpu_idle);
-			j_cdbs->prev_load = 100 * prev_load /
-					(unsigned int) j_cdbs->prev_cpu_wall;
+		if (ignore_nice)
+			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-			if (ignore_nice)
-				j_cdbs->prev_cpu_nice =
-					kcpustat_cpu(j).cpustat[CPUTIME_NICE];
+		mutex_init(&j_cdbs->timer_mutex);
+		INIT_DEFERRABLE_WORK(&j_cdbs->work, cdata->gov_dbs_timer);
+	}
 
-			mutex_init(&j_cdbs->timer_mutex);
-			INIT_DEFERRABLE_WORK(&j_cdbs->work,
-					     dbs_data->cdata->gov_dbs_timer);
-		}
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_cpu_dbs_info_s *cs_dbs_info =
+			cdata->get_cpu_dbs_info_s(cpu);
 
-		if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
-			cs_dbs_info->down_skip = 0;
-			cs_dbs_info->enable = 1;
-			cs_dbs_info->requested_freq = policy->cur;
-		} else {
-			od_dbs_info->rate_mult = 1;
-			od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
-			od_ops->powersave_bias_init_cpu(cpu);
-		}
+		cs_dbs_info->down_skip = 0;
+		cs_dbs_info->enable = 1;
+		cs_dbs_info->requested_freq = policy->cur;
+	} else {
+		struct od_ops *od_ops = cdata->gov_ops;
+		struct od_cpu_dbs_info_s *od_dbs_info = cdata->get_cpu_dbs_info_s(cpu);
 
-		mutex_unlock(&dbs_data->mutex);
+		od_dbs_info->rate_mult = 1;
+		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
+		od_ops->powersave_bias_init_cpu(cpu);
+	}
 
-		/* Initiate timer time stamp */
-		cpu_cdbs->time_stamp = ktime_get();
+	mutex_unlock(&dbs_data->mutex);
 
-		gov_queue_work(dbs_data, policy,
-				delay_for_sampling_rate(sampling_rate), true);
-		break;
+	/* Initiate timer time stamp */
+	cpu_cdbs->time_stamp = ktime_get();
 
-	case CPUFREQ_GOV_STOP:
-		if (dbs_data->cdata->governor == GOV_CONSERVATIVE)
-			cs_dbs_info->enable = 0;
+	gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate),
+		       true);
+	return 0;
+}
+
+static void cpufreq_governor_stop(struct cpufreq_policy *policy,
+				  struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
+
+	if (cdata->governor == GOV_CONSERVATIVE) {
+		struct cs_cpu_dbs_info_s *cs_dbs_info =
+			cdata->get_cpu_dbs_info_s(cpu);
 
-		gov_cancel_work(dbs_data, policy);
+		cs_dbs_info->enable = 0;
+	}
+
+	gov_cancel_work(dbs_data, policy);
+
+	mutex_lock(&dbs_data->mutex);
+	mutex_destroy(&cpu_cdbs->timer_mutex);
+	cpu_cdbs->cur_policy = NULL;
+	mutex_unlock(&dbs_data->mutex);
+}
 
-		mutex_lock(&dbs_data->mutex);
-		mutex_destroy(&cpu_cdbs->timer_mutex);
-		cpu_cdbs->cur_policy = NULL;
+static void cpufreq_governor_limits(struct cpufreq_policy *policy,
+				    struct dbs_data *dbs_data)
+{
+	struct common_dbs_data *cdata = dbs_data->cdata;
+	unsigned int cpu = policy->cpu;
+	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
 
+	mutex_lock(&dbs_data->mutex);
+	if (!cpu_cdbs->cur_policy) {
 		mutex_unlock(&dbs_data->mutex);
+		return;
+	}
 
-		break;
+	mutex_lock(&cpu_cdbs->timer_mutex);
+	if (policy->max < cpu_cdbs->cur_policy->cur)
+		__cpufreq_driver_target(cpu_cdbs->cur_policy, policy->max,
+					CPUFREQ_RELATION_H);
+	else if (policy->min > cpu_cdbs->cur_policy->cur)
+		__cpufreq_driver_target(cpu_cdbs->cur_policy, policy->min,
+					CPUFREQ_RELATION_L);
+	dbs_check_cpu(dbs_data, cpu);
+	mutex_unlock(&cpu_cdbs->timer_mutex);
+
+	mutex_unlock(&dbs_data->mutex);
+}
 
+int cpufreq_governor_dbs(struct cpufreq_policy *policy,
+			 struct common_dbs_data *cdata, unsigned int event)
+{
+	struct dbs_data *dbs_data;
+	int ret = 0;
+
+	if (have_governor_per_policy())
+		dbs_data = policy->governor_data;
+	else
+		dbs_data = cdata->gdbs_data;
+
+	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+
+	switch (event) {
+	case CPUFREQ_GOV_POLICY_INIT:
+		ret = cpufreq_governor_init(policy, dbs_data, cdata);
+		break;
+	case CPUFREQ_GOV_POLICY_EXIT:
+		cpufreq_governor_exit(policy, dbs_data);
+		break;
+	case CPUFREQ_GOV_START:
+		ret = cpufreq_governor_start(policy, dbs_data);
+		break;
+	case CPUFREQ_GOV_STOP:
+		cpufreq_governor_stop(policy, dbs_data);
+		break;
 	case CPUFREQ_GOV_LIMITS:
-		mutex_lock(&dbs_data->mutex);
-		if (!cpu_cdbs->cur_policy) {
-			mutex_unlock(&dbs_data->mutex);
-			break;
-		}
-		mutex_lock(&cpu_cdbs->timer_mutex);
-		if (policy->max < cpu_cdbs->cur_policy->cur)
-			__cpufreq_driver_target(cpu_cdbs->cur_policy,
-					policy->max, CPUFREQ_RELATION_H);
-		else if (policy->min > cpu_cdbs->cur_policy->cur)
-			__cpufreq_driver_target(cpu_cdbs->cur_policy,
-					policy->min, CPUFREQ_RELATION_L);
-		dbs_check_cpu(dbs_data, cpu);
-		mutex_unlock(&cpu_cdbs->timer_mutex);
-		mutex_unlock(&dbs_data->mutex);
+		cpufreq_governor_limits(policy, dbs_data);
 		break;
 	}
-	return 0;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_governor_dbs);
Signature · 1 line
2.4.0
Reviewed-by on v2 2/3
#9 lore raw

Re: [PATCH V2 2/3] cpufreq: governor: split cpufreq_governor_dbs()

Quoted 10 lines · Viresh Kumar

cpufreq_governor_dbs() is hardly readable, it is just too big and complicated. Lets make it more readable by splitting out event specific routines.

Order of statements is changed at few places, but that shouldn't bring any functional change.

Signed-off-by Viresh Kumar <[email protected]>

---
V1->V2: Return errors after hitting WARN_ON. (Preeti)

Reviewed-by Preeti U Murthy <[email protected]>

Regards
Preeti U Murthy

author maintainer
#10 lore raw

PATCH 3/3 cpufreq: governor: Serialize governor callbacks

In reply to Viresh Kumar

There are several races reported in cpufreq core around governors (only ondemand and conservative) by different people.

There are at least two race scenarios present in governor code:
(a) Concurrent access/updates of governor internal structures.

It is possible that fields such as 'dbs_data->usage_count', etc. are accessed simultaneously for different policies using same governor structure (i.e. CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag unset). And because of this we can dereference bad pointers.

For example consider a system with two CPUs with separate 'struct cpufreq_policy' instances. CPU0 governor: ondemand and CPU1: powersave. CPU0 switching to powersave and CPU1 to ondemand:

	CPU0				CPU1

	store*				store*

	cpufreq_governor_exit()		cpufreq_governor_init()
					dbs_data = cdata->gdbs_data;

	if (!--dbs_data->usage_count)
		kfree(dbs_data);

					dbs_data->usage_count++;
					*Bad pointer dereference*

There are other races possible between EXIT and START/STOP/LIMIT as well. Its really complicated.

(b) Switching governor state in bad sequence:

For example trying to switch a governor to START state, when the governor is in EXIT state. There are some checks present in __cpufreq_governor() but they aren't sufficient as they compare events against 'policy->governor_enabled', where as we need to take governor's state into account, which can be used by multiple policies.

These two issues need to be solved separately and the responsibility should be properly divided between cpufreq and governor core.

The first problem is more about the governor core, as it needs to protect its structures properly. And the second problem should be fixed in cpufreq core instead of governor, as its all about sequence of events.

This patch is trying to solve only the first problem.

There are two types of data we need to protect,
- 'struct common_dbs_data': No matter what, there is going to be a single copy of this per governor.
- 'struct dbs_data': With CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag set, we will have per-policy copy of this data, otherwise a single copy.

Because of such complexities, the mutex present in 'struct dbs_data' is insufficient to solve our problem. For example we need to protect fetching of 'dbs_data' from different structures at the beginning of cpufreq_governor_dbs(), to make sure it isn't currently being updated.

This can be fixed if we can guarantee serialization of event parsing code for an individual governor. This is best solved with a mutex per governor, and the placeholder for that is 'struct common_dbs_data'.

And so this patch moves the mutex from 'struct dbs_data' to 'struct common_dbs_data' and takes it at the beginning and drops it at the end of cpufreq_governor_dbs().

Tested with and without following configuration options:

CONFIG_LOCKDEP_SUPPORT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_PI_LIST=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq_conservative.c |  2 +-
 drivers/cpufreq/cpufreq_governor.c     | 24 +++++++++++-------------
 drivers/cpufreq/cpufreq_governor.h     |  8 +++++---
 drivers/cpufreq/cpufreq_ondemand.c     |  2 +-
 4 files changed, 18 insertions(+), 18 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +1 -1
index 75f875bb155e..c86a10c30912 100644
@@ -345,7 +345,6 @@ static int cs_init(struct dbs_data *dbs_data, bool notify)
 		cpufreq_register_notifier(&cs_cpufreq_notifier_block,
 					  CPUFREQ_TRANSITION_NOTIFIER);
 
-	mutex_init(&dbs_data->mutex);
 	return 0;
 }
 

@@ -370,6 +369,7 @@ static struct common_dbs_data cs_dbs_cdata = {
 	.gov_check_cpu = cs_check_cpu,
 	.init = cs_init,
 	.exit = cs_exit,
+	.mutex = __MUTEX_INITIALIZER(cs_dbs_cdata.mutex),
 };
 
 static int cs_cpufreq_governor_dbs(struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq_governor.c +11 -13
index dc382a5a2158..ed849a8777dd 100644
@@ -344,8 +344,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		io_busy = od_tuners->io_is_busy;
 	}
 
-	mutex_lock(&dbs_data->mutex);
-
 	for_each_cpu(j, policy->cpus) {
 		struct cpu_dbs_common_info *j_cdbs = cdata->get_cpu_cdbs(j);
 		unsigned int prev_load;

@@ -383,8 +381,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		od_ops->powersave_bias_init_cpu(cpu);
 	}
 
-	mutex_unlock(&dbs_data->mutex);
-
 	/* Initiate timer time stamp */
 	cpu_cdbs->time_stamp = ktime_get();
 

@@ -409,10 +405,8 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,
 
 	gov_cancel_work(dbs_data, policy);
 
-	mutex_lock(&dbs_data->mutex);
 	mutex_destroy(&cpu_cdbs->timer_mutex);
 	cpu_cdbs->cur_policy = NULL;
-	mutex_unlock(&dbs_data->mutex);
 }
 
 static void cpufreq_governor_limits(struct cpufreq_policy *policy,

@@ -422,11 +416,8 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy,
 	unsigned int cpu = policy->cpu;
 	struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs(cpu);
 
-	mutex_lock(&dbs_data->mutex);
-	if (!cpu_cdbs->cur_policy) {
-		mutex_unlock(&dbs_data->mutex);
+	if (!cpu_cdbs->cur_policy)
 		return;
-	}
 
 	mutex_lock(&cpu_cdbs->timer_mutex);
 	if (policy->max < cpu_cdbs->cur_policy->cur)

@@ -437,8 +428,6 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy,
 					CPUFREQ_RELATION_L);
 	dbs_check_cpu(dbs_data, cpu);
 	mutex_unlock(&cpu_cdbs->timer_mutex);
-
-	mutex_unlock(&dbs_data->mutex);
 }
 
 int cpufreq_governor_dbs(struct cpufreq_policy *policy,

@@ -447,12 +436,18 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 	struct dbs_data *dbs_data;
 	int ret = 0;
 
+	/* Lock governor to block concurrent initialization of governor */
+	mutex_lock(&cdata->mutex);
+
 	if (have_governor_per_policy())
 		dbs_data = policy->governor_data;
 	else
 		dbs_data = cdata->gdbs_data;
 
-	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+	if (WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	switch (event) {
 	case CPUFREQ_GOV_POLICY_INIT:

@@ -472,6 +467,9 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		break;
 	}
 
+unlock:
+	mutex_unlock(&cdata->mutex);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_governor_dbs);
drivers/cpufreq/cpufreq_governor.h +5 -3
index 1690120df487..34736f5e869d 100644
@@ -213,6 +213,11 @@ struct common_dbs_data {
 
 	/* Governor specific ops, see below */
 	void *gov_ops;
+
+	/*
+	 * Protects governor's data (struct dbs_data and struct common_dbs_data)
+	 */
+	struct mutex mutex;
 };
 
 /* Governor Per policy data */

@@ -221,9 +226,6 @@ struct dbs_data {
 	unsigned int min_sampling_rate;
 	int usage_count;
 	void *tuners;
-
-	/* dbs_mutex protects dbs_enable in governor start/stop */
-	struct mutex mutex;
 };
 
 /* Governor specific ops, will be passed to dbs_data->gov_ops */
drivers/cpufreq/cpufreq_ondemand.c +1 -1
index 4fe78a9caa04..3c1e10f2304c 100644
@@ -513,7 +513,6 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
 	tuners->io_is_busy = should_io_be_busy();
 
 	dbs_data->tuners = tuners;
-	mutex_init(&dbs_data->mutex);
 	return 0;
 }
 

@@ -541,6 +540,7 @@ static struct common_dbs_data od_dbs_cdata = {
 	.gov_ops = &od_ops,
 	.init = od_init,
 	.exit = od_exit,
+	.mutex = __MUTEX_INITIALIZER(od_dbs_cdata.mutex),
 };
 
 static void od_set_powersave_bias(unsigned int powersave_bias)
Signature · 1 line
2.4.0
Reviewed-by on v1 3/3
#11 lore raw

Re: [PATCH 3/3] cpufreq: governor: Serialize governor callbacks

Quoted 79 lines · Viresh Kumar

There are several races reported in cpufreq core around governors (only ondemand and conservative) by different people.

There are at least two race scenarios present in governor code:
(a) Concurrent access/updates of governor internal structures.

It is possible that fields such as 'dbs_data->usage_count', etc. are accessed simultaneously for different policies using same governor structure (i.e. CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag unset). And because of this we can dereference bad pointers.

For example consider a system with two CPUs with separate 'struct cpufreq_policy' instances. CPU0 governor: ondemand and CPU1: powersave. CPU0 switching to powersave and CPU1 to ondemand:

	CPU0				CPU1

	store*				store*

	cpufreq_governor_exit()		cpufreq_governor_init()
					dbs_data = cdata->gdbs_data;

	if (!--dbs_data->usage_count)
		kfree(dbs_data);

					dbs_data->usage_count++;
					*Bad pointer dereference*

There are other races possible between EXIT and START/STOP/LIMIT as well. Its really complicated.

(b) Switching governor state in bad sequence:

For example trying to switch a governor to START state, when the governor is in EXIT state. There are some checks present in __cpufreq_governor() but they aren't sufficient as they compare events against 'policy->governor_enabled', where as we need to take governor's state into account, which can be used by multiple policies.

These two issues need to be solved separately and the responsibility should be properly divided between cpufreq and governor core.

The first problem is more about the governor core, as it needs to protect its structures properly. And the second problem should be fixed in cpufreq core instead of governor, as its all about sequence of events.

This patch is trying to solve only the first problem.

There are two types of data we need to protect,
- 'struct common_dbs_data': No matter what, there is going to be a single copy of this per governor.
- 'struct dbs_data': With CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag set, we will have per-policy copy of this data, otherwise a single copy.

Because of such complexities, the mutex present in 'struct dbs_data' is insufficient to solve our problem. For example we need to protect fetching of 'dbs_data' from different structures at the beginning of cpufreq_governor_dbs(), to make sure it isn't currently being updated.

This can be fixed if we can guarantee serialization of event parsing code for an individual governor. This is best solved with a mutex per governor, and the placeholder for that is 'struct common_dbs_data'.

And so this patch moves the mutex from 'struct dbs_data' to 'struct common_dbs_data' and takes it at the beginning and drops it at the end of cpufreq_governor_dbs().

Tested with and without following configuration options:

CONFIG_LOCKDEP_SUPPORT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_PI_LIST=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y

Quoted 1 line
Signed-off-by Viresh Kumar <[email protected]>
Reviewed-by Preeti U Murthy <[email protected]>
#12 lore raw
In reply to Viresh Kumar
Quoted 33 lines · Viresh Kumar

Hi Rafael,

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Rebased over: v4.1-rc6
Tested-on: ARM dual Cortex -A15 Exynos board.

[1] http://marc.info/?i=20150601064031.2972.59208.stgit%40perfhull-ltc.austin.ibm.com

Viresh Kumar (3):
cpufreq: governor: register notifier from cs_init()
cpufreq: governor: split cpufreq_governor_dbs()
cpufreq: governor: Serialize governor callbacks

Quoted 6 lines
 drivers/cpufreq/cpufreq_conservative.c |  28 +--
 drivers/cpufreq/cpufreq_governor.c     | 340 ++++++++++++++++++---------------
 drivers/cpufreq/cpufreq_governor.h     |  16 +-
 drivers/cpufreq/cpufreq_ondemand.c     |   6 +-
 4 files changed, 209 insertions(+), 181 deletions(-)

I did a hotplug test on a single core alongside changing governors between ondemand and conservative on the same core. The policy is per core on powerpc. Within a second of that run the kernel panics. The backtrace is below:

[  165.981836] Unable to handle kernel paging request for data at
address 0x00000000
[  165.981929] Faulting instruction address: 0xc00000000053b3e0
cpu 0x4: Vector: 300 (Data Access) at [c000000fe0b2b880]
    pc: c00000000053b3e0: __bitmap_weight+0x70/0x100
    lr: c00000000085a008: need_load_eval+0x38/0xf0
    sp: c000000fe0b2bb00
   msr: 9000000100009033
   dar: 0
 dsisr: 40000000
  current = 0xc000000003e4fc90
  paca    = 0xc000000007da2600	 softe: 0	 irq_happened: 0x01
    pid   = 812, comm = kworker/4:2

enter ? for help
[c000000fe0b2bb50] c00000000085a008 need_load_eval+0x38/0xf0 [c000000fe0b2bb80] c00000000085815c cs_dbs_timer+0xdc/0x150 [c000000fe0b2bbe0] c0000000000f489c process_one_work+0x24c/0x910 [c000000fe0b2bc90] c0000000000f50dc worker_thread+0x17c/0x540 [c000000fe0b2bd20] c0000000000fed70 kthread+0x120/0x140 [c000000fe0b2be30] c000000000009678 ret_from_kernel_thread+0x5c/0x64

The crash is the same as was reported at
http://www.gossamer-threads.com/lists/linux/kernel/2186336.

Regards
Preeti U Murthy

#13 lore raw
Quoted 67 lines · Preeti U Murthy
Quoted 33 lines · Viresh Kumar

Hi Rafael,

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Rebased over: v4.1-rc6
Tested-on: ARM dual Cortex -A15 Exynos board.

[1] http://marc.info/?i=20150601064031.2972.59208.stgit%40perfhull-ltc.austin.ibm.com

Viresh Kumar (3):
cpufreq: governor: register notifier from cs_init()
cpufreq: governor: split cpufreq_governor_dbs()
cpufreq: governor: Serialize governor callbacks

Quoted 6 lines
 drivers/cpufreq/cpufreq_conservative.c |  28 +--
 drivers/cpufreq/cpufreq_governor.c     | 340 ++++++++++++++++++---------------
 drivers/cpufreq/cpufreq_governor.h     |  16 +-
 drivers/cpufreq/cpufreq_ondemand.c     |   6 +-
 4 files changed, 209 insertions(+), 181 deletions(-)

I did a hotplug test on a single core alongside changing governors between ondemand and conservative on the same core. The policy is per core on powerpc. Within a second of that run the kernel panics. The backtrace is below:

[  165.981836] Unable to handle kernel paging request for data at
address 0x00000000
[  165.981929] Faulting instruction address: 0xc00000000053b3e0
cpu 0x4: Vector: 300 (Data Access) at [c000000fe0b2b880]
    pc: c00000000053b3e0: __bitmap_weight+0x70/0x100
    lr: c00000000085a008: need_load_eval+0x38/0xf0
    sp: c000000fe0b2bb00
   msr: 9000000100009033
   dar: 0
 dsisr: 40000000
  current = 0xc000000003e4fc90
  paca    = 0xc000000007da2600	 softe: 0	 irq_happened: 0x01
    pid   = 812, comm = kworker/4:2

enter ? for help
[c000000fe0b2bb50] c00000000085a008 need_load_eval+0x38/0xf0 [c000000fe0b2bb80] c00000000085815c cs_dbs_timer+0xdc/0x150 [c000000fe0b2bbe0] c0000000000f489c process_one_work+0x24c/0x910 [c000000fe0b2bc90] c0000000000f50dc worker_thread+0x17c/0x540 [c000000fe0b2bd20] c0000000000fed70 kthread+0x120/0x140 [c000000fe0b2be30] c000000000009678 ret_from_kernel_thread+0x5c/0x64

Quoted 5 lines

The crash is the same as was reported at
http://www.gossamer-threads.com/lists/linux/kernel/2186336.

Regards
Preeti U Murthy

And a crash at the cpufreq worker thread again due to data access exception when I change governors in parallel on a single core.

cpu 0x3: Vector: 300 (Data Access) at [c000000fedb538f0]

    pc: c000000000856750: od_dbs_timer+0x60/0x1e0
    lr: c0000000000f489c: process_one_work+0x24c/0x910
    sp: c000000fedb53b70
   msr: 9000000100009033
   dar: 10
 dsisr: 40000000
  current = 0xc000000fe3d128e0
  paca    = 0xc000000007da1c80	 softe: 0	 irq_happened: 0x01
    pid   = 17227, comm = kworker/3:1

With the backtrace being:

[c000000fedb53be0] c0000000000f489c process_one_work+0x24c/0x910 [c000000fedb53c90] c0000000000f50dc worker_thread+0x17c/0x540 [c000000fedb53d20] c0000000000fed70 kthread+0x120/0x140 [c000000fedb53e30] c000000000009678 ret_from_kernel_thread+0x5c/0x64

But the kernel stays sane longer than before with the patchset. The above crash happens around 15 seconds after the test begins, while earlier it wouldn't survive 2 seconds even.

Regards
Preeti U Murthy

Quoted 1 line
author maintainer
#14 lore raw
Quoted 16 lines · Preeti U Murthy

And a crash at the cpufreq worker thread again due to data access exception when I change governors in parallel on a single core.

cpu 0x3: Vector: 300 (Data Access) at [c000000fedb538f0]

    pc: c000000000856750: od_dbs_timer+0x60/0x1e0
    lr: c0000000000f489c: process_one_work+0x24c/0x910
    sp: c000000fedb53b70
   msr: 9000000100009033
   dar: 10
 dsisr: 40000000
  current = 0xc000000fe3d128e0
  paca    = 0xc000000007da1c80	 softe: 0	 irq_happened: 0x01
    pid   = 17227, comm = kworker/3:1

With the backtrace being:

Quoted 8 lines

[c000000fedb53be0] c0000000000f489c process_one_work+0x24c/0x910 [c000000fedb53c90] c0000000000f50dc worker_thread+0x17c/0x540 [c000000fedb53d20] c0000000000fed70 kthread+0x120/0x140 [c000000fedb53e30] c000000000009678 ret_from_kernel_thread+0x5c/0x64

But the kernel stays sane longer than before with the patchset. The above crash happens around 15 seconds after the test begins, while earlier it wouldn't survive 2 seconds even.

I haven't attempted to solve the race between the worker threads and governor-callbacks yet. What I have tried to solve is the race between different callbacks. And you shouldn't see a race there for now. For example a race between INIT/EXIT/START/STOP/LIMITS.

Signature · 1 line
viresh
#15 lore raw
Quoted 22 lines · Viresh Kumar
Quoted 21 lines · Preeti U Murthy

And a crash at the cpufreq worker thread again due to data access exception when I change governors in parallel on a single core.

cpu 0x3: Vector: 300 (Data Access) at [c000000fedb538f0]

    pc: c000000000856750: od_dbs_timer+0x60/0x1e0
    lr: c0000000000f489c: process_one_work+0x24c/0x910
    sp: c000000fedb53b70
   msr: 9000000100009033
   dar: 10
 dsisr: 40000000
  current = 0xc000000fe3d128e0
  paca    = 0xc000000007da1c80	 softe: 0	 irq_happened: 0x01
    pid   = 17227, comm = kworker/3:1

With the backtrace being:

[c000000fedb53be0] c0000000000f489c process_one_work+0x24c/0x910 [c000000fedb53c90] c0000000000f50dc worker_thread+0x17c/0x540 [c000000fedb53d20] c0000000000fed70 kthread+0x120/0x140 [c000000fedb53e30] c000000000009678 ret_from_kernel_thread+0x5c/0x64

Quoted 8 lines
Quoted 3 lines

But the kernel stays sane longer than before with the patchset. The above crash happens around 15 seconds after the test begins, while earlier it wouldn't survive 2 seconds even.

I haven't attempted to solve the race between the worker threads and governor-callbacks yet. What I have tried to solve is the race between different callbacks. And you shouldn't see a race there for now. For example a race between INIT/EXIT/START/STOP/LIMITS.

Your fix may not be complete and here is why. The reason we see the crash is because we have *only* attempted to serialize calls to cpufreq_governor_dbs() and not attempted to serialize *entire logical sequence of operations*. Let's take a look at what is happening as a consequence.

CPU0 CPU1
store_scaling_governor() __cpufreq_remove_dev_finish()
__cpufreq_governor(CPUFREQ_GOV_STOP) __cpufreq_governor(CPUFREQ_GOV_START)
policy->governor_enabled = false
cpufreq_governor_dbs() policy->governor_enabled = true
mutex_lock()
gov_cancel_work() cpufreq_governor_dbs()
wait on lock

may call gov_queue_work()
if (!policy->enabled) : fails
and we end up queuing work

mutex_unlock()
mutex_lock() gov_queue_work() mutex_unlock()

__cpufreq_governor(CPUFREQ_GOV_POLICY_EXIT)
mutex_lock()
cpufreq_governor_dbs()
kfree(dbs_data)

timer fires and od_dbs_timer/cs_dbs_timer() runs References governor data structures which are freed

The issue as I see it is one set of operations must be allowed to run *completely* before another begins. When store_scaling_governor() says STOP, all governor operations must be stopped, till the time store_scaling_governor() itself gives permission to restart. Somebody else, in this case __cpufreq_remove_dev_finish() cannot overrule this if it arrives late. This is what is happening above.

So if store_scaling_governor() arrives first, STOP|EXIT|START|LIMIT must complete before START|LIMIT of __cpufreq_remove_dev_finish() is allowed to run. So it is just not about serializing, its about *what needs to be serialized*.

Regards
Preeti U Murthy

Quoted 1 line
author maintainer
#16 lore raw
Quoted 4 lines · Preeti U Murthy

Your fix may not be complete and here is why. The reason we see the crash is because we have *only* attempted to serialize calls to cpufreq_governor_dbs() and not attempted to serialize *entire logical sequence of operations*. Let's take a look at what is happening as a consequence.

You missed my logs (For the first time in my life I wrote them so well). This is what I mentioned in 3/3:

"
These two issues need to be solved separately and the responsibility should be properly divided between cpufreq and governor core.

The first problem is more about the governor core, as it needs to protect its structures properly. And the second problem should be fixed in cpufreq core instead of governor, as its all about sequence of events.

This patch is trying to solve only the first problem. "

I NEVER claimed that I solved all the issues.

Signature · 1 line
viresh
#17 lore raw
Quoted 14 lines · Viresh Kumar
Quoted 4 lines · Preeti U Murthy

Your fix may not be complete and here is why. The reason we see the crash is because we have *only* attempted to serialize calls to cpufreq_governor_dbs() and not attempted to serialize *entire logical sequence of operations*. Let's take a look at what is happening as a consequence.

You missed my logs (For the first time in my life I wrote them so well). This is what I mentioned in 3/3:

"
These two issues need to be solved separately and the responsibility should be properly divided between cpufreq and governor core.

Quoted 4 lines

The first problem is more about the governor core, as it needs to protect its structures properly. And the second problem should be fixed in cpufreq core instead of governor, as its all about sequence of events.

My point is do we really need to treat them as separate problems ? Will not serializing sequence of events help solve both issues ?

When we know the problem, why not fix it proper, rather than breaking it up ?

Quoted 5 lines

This patch is trying to solve only the first problem. "

I NEVER claimed that I solved all the issues.

That is true. My intention was to point out explicitly what still remains to be solved. It is true that you have mentioned that the problem in the cpufreq core is about sequencing of events. I intended to highlight what it was.

I would have restrained from pointing it out had the issues that I am seeing waned a wee bit, but it has not, which is why I did not see value in having the third patch as a stand alone patch, with more going in as series.

Regards
Preeti U Murthy

Quoted 1 line
author maintainer
#18 lore raw
Quoted 1 line · Preeti U Murthy

My point is do we really need to treat them as separate problems ?

Yes.

Quoted 1 line

Will not serializing sequence of events help solve both issues ?

That's not the point. Even if it solves the problem, it may not be the right approach. There are two problems here:
- One lies in cpufreq.c or in policies domain.
- Other one is about governor. Governor code shouldn't rely of cpufreq.c locking to guarantee that access to its structures isn't racy.

And the way you proposed to solve it (yes the original idea was from one of my earlier patches) is not the right way to do it.

For example, cpufreq_set_policy() shouldn't care about how the governor code is placed. It should just do enough to get rid of racy code belonging to that policy.

But with our other approach, we are trying to stop the governor to be used by anyone else in the kernel. Who the hell is that 'policy' to decide who can access the governor ?

That's why I divided it up, so that we don't come to it again. I haven't learnt these things earlier, and wrote messy locking code earlier. But after looking/working on core code like timers etc, I understood the importance of right design and proper partitioning of responsibilities.

Quoted 2 lines

When we know the problem, why not fix it proper, rather than breaking it up ?

What are we breaking here ?

Quoted 9 lines
Quoted 4 lines

This patch is trying to solve only the first problem. "

I NEVER claimed that I solved all the issues.

That is true. My intention was to point out explicitly what still remains to be solved. It is true that you have mentioned that the problem in the cpufreq core is about sequencing of events. I intended to highlight what it was.

Okay, that should be the next target we have to fix after applying these patches.

Quoted 4 lines

I would have restrained from pointing it out had the issues that I am seeing waned a wee bit, but it has not, which is why I did not see value in having the third patch as a stand alone patch, with more going in as series.

Its solving what it is intended to solve. But we need more patches over it to fix problems outside the scope of governors.

Signature · 1 line
viresh
#19 lore raw
Quoted 25 lines · Viresh Kumar
Quoted 1 line · Preeti U Murthy

My point is do we really need to treat them as separate problems ?

Yes.

Quoted 1 line

Will not serializing sequence of events help solve both issues ?

That's not the point. Even if it solves the problem, it may not be the right approach. There are two problems here:
- One lies in cpufreq.c or in policies domain.
- Other one is about governor. Governor code shouldn't rely of cpufreq.c locking to guarantee that access to its structures isn't racy.

And the way you proposed to solve it (yes the original idea was from one of my earlier patches) is not the right way to do it.

For example, cpufreq_set_policy() shouldn't care about how the governor code is placed. It should just do enough to get rid of racy code belonging to that policy.

But with our other approach, we are trying to stop the governor to be used by anyone else in the kernel. Who the hell is that 'policy' to decide who can access the governor ?

Quoted 5 lines

That's why I divided it up, so that we don't come to it again. I haven't learnt these things earlier, and wrote messy locking code earlier. But after looking/working on core code like timers etc, I understood the importance of right design and proper partitioning of responsibilities.

Ok, fair enough.

Regards
Preeti U Murthy

author maintainer
#20 lore raw
In reply to Preeti U Murthy
Quoted 15 lines · Preeti U Murthy
Quoted 14 lines · Viresh Kumar

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

Quoted 6 lines
Quoted 6 lines

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Hi Preeti,

Thanks for giving your RBY tags for all the patches, would you also like to give Tested-by's if you have done any testing on these.

That is just to confirm it hasn't broken things any further and that we haven't seen any crashes in races between INIT/EXIT/START/STOP/LIMITS.

Signature · 1 line
viresh
#21 lore raw
Quoted 23 lines · Viresh Kumar
Quoted 15 lines · Preeti U Murthy
Quoted 14 lines · Viresh Kumar

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

Quoted 6 lines
Quoted 6 lines

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Quoted 9 lines

Hi Preeti,

Thanks for giving your RBY tags for all the patches, would you also like to give Tested-by's if you have done any testing on these.

That is just to confirm it hasn't broken things any further and that we haven't seen any crashes in races between INIT/EXIT/START/STOP/LIMITS.

Let me run a fair bit of tests today and confirm this.

Regards
Preeti U Murthy

#22 lore raw
In reply to Viresh Kumar
Quoted 23 lines · Viresh Kumar
Quoted 15 lines · Preeti U Murthy
Quoted 14 lines · Viresh Kumar

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

Quoted 6 lines
Quoted 6 lines

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Quoted 8 lines

Hi Preeti,

Thanks for giving your RBY tags for all the patches, would you also like to give Tested-by's if you have done any testing on these.

That is just to confirm it hasn't broken things any further and that we haven't seen any crashes in races between INIT/EXIT/START/STOP/LIMITS.

I realize that I am not currently hitting the races on my machine that this patchset is trying to solve. The races that I am hitting have been conveyed on this thread and we should be fixing that separately as Viresh pointed out. I can therefore provide only by Reviewed-bys to this patchset.

Thanks

Regards
Preeti U Murthy

Quoted 1 line
Applied
#23 lore raw
In reply to Viresh Kumar
Quoted 33 lines · Viresh Kumar

Hi Rafael,

Preeti recently highlighted [1] some issues in cpufreq core locking with respect to governors. I wanted to solve them after we have simplified the hotplug paths in cpufreq core with my latest patches, but now that she has poked me, I have done some work in that area.

I am trying to solve only a part of the bigger problem (in a way that I feel is the right way ahead). The first patches restructures code to make it more readable and the last patch does all the major changes. The logs in that one should be good enough to explain why and what I am doing.

The first two shouldn't bring any functional change and so can be applied early if you are confident about them.

@Preeti: I would like you to test these patches. These should get rid of the crashes you were facing but may generate a WARN() from line 447 of cpufreq_governor.c, if the sequence is wrong. That has to be fixed separately.

Line 447: WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT))

Rebased over: v4.1-rc6
Tested-on: ARM dual Cortex -A15 Exynos board.

[1] http://marc.info/?i=20150601064031.2972.59208.stgit%40perfhull-ltc.austin.ibm.com

Viresh Kumar (3):
cpufreq: governor: register notifier from cs_init()
cpufreq: governor: split cpufreq_governor_dbs()
cpufreq: governor: Serialize governor callbacks

Quoted 5 lines
 drivers/cpufreq/cpufreq_conservative.c |  28 +--
 drivers/cpufreq/cpufreq_governor.c     | 340 ++++++++++++++++++---------------
 drivers/cpufreq/cpufreq_governor.h     |  16 +-
 drivers/cpufreq/cpufreq_ondemand.c     |   6 +-
 4 files changed, 209 insertions(+), 181 deletions(-)

I've queued up the series (with the replacement [2/3]) for 4.2, thanks!

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.