[PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[Viresh Kumar]

Hi Rafael,

Sorry for doing this, I know you were also looking to fix this in a
possibly different way. But I thought, it would be better if we fix
that. We can scrap this version and take yours if that looks better.

The root cause of all the issues we were facing, was that we were taking
policy->rwsem while accessing governor sysfs attributes. And that
happened because we were sharing the show/store calls present in
cpufreq.c.

I thought, perhaps the best way to fix it is to give separate sysfs-ops
to governors. And that's what I did.

@Juri: I need your help in testing these. My platform doesn't give me
those lockups (even without these patches) and Juno/Tc2 would fit
better.

Can you please run some tests on these?

They are pushed here for easy access (and auto test by build-bot):
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject

--
viresh

Viresh Kumar (5):
  cpufreq: governor: Kill declare_show_sampling_rate_min()
  cpufreq: governor: Create separate sysfs-ops
  cpufreq: governor: Remove unused sysfs attribute macros
  cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT
  cpufreq: Get rid of ->governor_enabled and its lock

 drivers/cpufreq/cpufreq.c              |  29 ----------
 drivers/cpufreq/cpufreq_conservative.c |  77 ++++++++++---------------
 drivers/cpufreq/cpufreq_governor.c     |  86 ++++++++++++++++++++--------
 drivers/cpufreq/cpufreq_governor.h     | 101 +++++++--------------------------
 drivers/cpufreq/cpufreq_ondemand.c     |  77 ++++++++++---------------
 include/linux/cpufreq.h                |   5 --
 6 files changed, 143 insertions(+), 232 deletions(-)

-- 
2.7.0.79.gdc08a19

[PATCH 1/5] cpufreq: governor: Kill declare_show_sampling_rate_min()

[Viresh Kumar]

This extra macro is required because the variable min_sampling_rate is
made part of 'struct dbs_data' instead of governor specific tunables.

For further optimization, its better that we kill
declare_show_sampling_rate_min() by moving min_sampling_rate to governor
specific tunables.

Lets do it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 14 ++++++-------
 drivers/cpufreq/cpufreq_governor.c     | 36 ++++++++++++++++++++--------------
 drivers/cpufreq/cpufreq_governor.h     | 18 ++---------------
 drivers/cpufreq/cpufreq_ondemand.c     | 14 ++++++-------
 4 files changed, 37 insertions(+), 45 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 606ad74abe6e..57750367bd26 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -185,7 +185,7 @@ static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,
 	if (ret != 1)
 		return -EINVAL;
 
-	cs_tuners->sampling_rate = max(input, dbs_data->min_sampling_rate);
+	cs_tuners->sampling_rate = max(input, cs_tuners->min_sampling_rate);
 	return count;
 }
 
@@ -281,7 +281,7 @@ show_store_one(cs, up_threshold);
 show_store_one(cs, down_threshold);
 show_store_one(cs, ignore_nice_load);
 show_store_one(cs, freq_step);
-declare_show_sampling_rate_min(cs);
+show_one(cs, min_sampling_rate);
 
 gov_sys_pol_attr_rw(sampling_rate);
 gov_sys_pol_attr_rw(sampling_down_factor);
@@ -289,10 +289,10 @@ gov_sys_pol_attr_rw(up_threshold);
 gov_sys_pol_attr_rw(down_threshold);
 gov_sys_pol_attr_rw(ignore_nice_load);
 gov_sys_pol_attr_rw(freq_step);
-gov_sys_pol_attr_ro(sampling_rate_min);
+gov_sys_pol_attr_ro(min_sampling_rate);
 
 static struct attribute *dbs_attributes_gov_sys[] = {
-	&sampling_rate_min_gov_sys.attr,
+	&min_sampling_rate_gov_sys.attr,
 	&sampling_rate_gov_sys.attr,
 	&sampling_down_factor_gov_sys.attr,
 	&up_threshold_gov_sys.attr,
@@ -308,7 +308,7 @@ static struct attribute_group cs_attr_group_gov_sys = {
 };
 
 static struct attribute *dbs_attributes_gov_pol[] = {
-	&sampling_rate_min_gov_pol.attr,
+	&min_sampling_rate_gov_pol.attr,
 	&sampling_rate_gov_pol.attr,
 	&sampling_down_factor_gov_pol.attr,
 	&up_threshold_gov_pol.attr,
@@ -340,10 +340,10 @@ static int cs_init(struct dbs_data *dbs_data, bool notify)
 	tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
 	tuners->ignore_nice_load = 0;
 	tuners->freq_step = DEF_FREQUENCY_STEP;
+	tuners->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
+		jiffies_to_usecs(10);
 
 	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,
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index e0d111024d48..9a7edc91ad57 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -286,16 +286,32 @@ static void dbs_timer_handler(unsigned long data)
 		queue_work(system_wq, &shared->work);
 }
 
-static void set_sampling_rate(struct dbs_data *dbs_data,
-		unsigned int sampling_rate)
+static void set_sampling_rate(struct cpufreq_policy *policy,
+			      struct dbs_data *dbs_data)
 {
+	unsigned int *sampling_rate;
+	unsigned int *min_sampling_rate;
+	unsigned int latency;
+
+	/* policy latency is in ns. Convert it to us first */
+	latency = policy->cpuinfo.transition_latency / 1000;
+	if (latency == 0)
+		latency = 1;
+
 	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
 		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
-		cs_tuners->sampling_rate = sampling_rate;
+		sampling_rate = &cs_tuners->sampling_rate;
+		min_sampling_rate = &cs_tuners->min_sampling_rate;
 	} else {
 		struct od_dbs_tuners *od_tuners = dbs_data->tuners;
-		od_tuners->sampling_rate = sampling_rate;
+		sampling_rate = &od_tuners->sampling_rate;
+		min_sampling_rate = &od_tuners->min_sampling_rate;
 	}
+
+	/* Bring kernel and HW constraints together */
+	*min_sampling_rate = max(*min_sampling_rate,
+				 MIN_LATENCY_MULTIPLIER * latency);
+	*sampling_rate = max(*min_sampling_rate, latency * LATENCY_MULTIPLIER);
 }
 
 static int alloc_common_dbs_info(struct cpufreq_policy *policy,
@@ -338,7 +354,6 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 				 struct dbs_data *dbs_data,
 				 struct common_dbs_data *cdata)
 {
-	unsigned int latency;
 	int ret;
 
 	/* State should be equivalent to EXIT */
@@ -373,16 +388,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 	if (ret)
 		goto free_common_dbs_info;
 
-	/* policy latency is in ns. Convert it to us first */
-	latency = policy->cpuinfo.transition_latency / 1000;
-	if (latency == 0)
-		latency = 1;
-
-	/* 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));
+	set_sampling_rate(policy, dbs_data);
 
 	if (!have_governor_per_policy())
 		cdata->gdbs_data = dbs_data;
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 91e767a058a7..ad44a8546a3a 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -183,6 +183,7 @@ struct od_dbs_tuners {
 	unsigned int up_threshold;
 	unsigned int powersave_bias;
 	unsigned int io_is_busy;
+	unsigned int min_sampling_rate;
 };
 
 struct cs_dbs_tuners {
@@ -192,6 +193,7 @@ struct cs_dbs_tuners {
 	unsigned int up_threshold;
 	unsigned int down_threshold;
 	unsigned int freq_step;
+	unsigned int min_sampling_rate;
 };
 
 /* Common Governor data across policies */
@@ -230,7 +232,6 @@ struct common_dbs_data {
 /* Governor Per policy data */
 struct dbs_data {
 	struct common_dbs_data *cdata;
-	unsigned int min_sampling_rate;
 	int usage_count;
 	void *tuners;
 };
@@ -254,21 +255,6 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 	return delay;
 }
 
-#define declare_show_sampling_rate_min(_gov)				\
-static ssize_t show_sampling_rate_min_gov_sys				\
-(struct kobject *kobj, struct attribute *attr, char *buf)		\
-{									\
-	struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data;		\
-	return sprintf(buf, "%u\n", dbs_data->min_sampling_rate);	\
-}									\
-									\
-static ssize_t show_sampling_rate_min_gov_pol				\
-(struct cpufreq_policy *policy, char *buf)				\
-{									\
-	struct dbs_data *dbs_data = policy->governor_data;		\
-	return sprintf(buf, "%u\n", dbs_data->min_sampling_rate);	\
-}
-
 extern struct mutex cpufreq_governor_lock;
 
 void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index eae51070c034..b31f64745232 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -250,7 +250,7 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
 	int cpu;
 
 	od_tuners->sampling_rate = new_rate = max(new_rate,
-			dbs_data->min_sampling_rate);
+			od_tuners->min_sampling_rate);
 
 	/*
 	 * Lock governor so that governor start/stop can't execute in parallel.
@@ -442,7 +442,7 @@ show_store_one(od, up_threshold);
 show_store_one(od, sampling_down_factor);
 show_store_one(od, ignore_nice_load);
 show_store_one(od, powersave_bias);
-declare_show_sampling_rate_min(od);
+show_one(od, min_sampling_rate);
 
 gov_sys_pol_attr_rw(sampling_rate);
 gov_sys_pol_attr_rw(io_is_busy);
@@ -450,10 +450,10 @@ gov_sys_pol_attr_rw(up_threshold);
 gov_sys_pol_attr_rw(sampling_down_factor);
 gov_sys_pol_attr_rw(ignore_nice_load);
 gov_sys_pol_attr_rw(powersave_bias);
-gov_sys_pol_attr_ro(sampling_rate_min);
+gov_sys_pol_attr_ro(min_sampling_rate);
 
 static struct attribute *dbs_attributes_gov_sys[] = {
-	&sampling_rate_min_gov_sys.attr,
+	&min_sampling_rate_gov_sys.attr,
 	&sampling_rate_gov_sys.attr,
 	&up_threshold_gov_sys.attr,
 	&sampling_down_factor_gov_sys.attr,
@@ -469,7 +469,7 @@ static struct attribute_group od_attr_group_gov_sys = {
 };
 
 static struct attribute *dbs_attributes_gov_pol[] = {
-	&sampling_rate_min_gov_pol.attr,
+	&min_sampling_rate_gov_pol.attr,
 	&sampling_rate_gov_pol.attr,
 	&up_threshold_gov_pol.attr,
 	&sampling_down_factor_gov_pol.attr,
@@ -509,12 +509,12 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
 		 * not depending on HZ, but fixed (very low). The deferred
 		 * timer might skip some samples if idle/sleeping as needed.
 		*/
-		dbs_data->min_sampling_rate = MICRO_FREQUENCY_MIN_SAMPLE_RATE;
+		tuners->min_sampling_rate = MICRO_FREQUENCY_MIN_SAMPLE_RATE;
 	} else {
 		tuners->up_threshold = DEF_FREQUENCY_UP_THRESHOLD;
 
 		/* For correct statistics, we need 10 ticks for each measure */
-		dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
+		tuners->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
 			jiffies_to_usecs(10);
 	}
 
-- 
2.7.0.79.gdc08a19

Re: [PATCH 1/5] cpufreq: governor: Kill declare_show_sampling_rate_min()

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> This extra macro is required because the variable min_sampling_rate is
> made part of 'struct dbs_data' instead of governor specific tunables.
>
> For further optimization, its better that we kill
> declare_show_sampling_rate_min() by moving min_sampling_rate to governor
> specific tunables.
>
> Lets do it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

To me, this is not about the macro, but about moving min_sampling_rate
to governor tunables, so my subject would be something like "cpufreq:
governor: Treat min_sampling_rate as a governor-specific tunable".

My changelog, then, would be something like the following:

"The min_sampling_rate governor tunable is a field in struct dbs_data,
so it has to be handled in a special way separate from the rest of
governor tunables.  In particular, that requires a special macro to be
present for creating its show/store sysfs attribute callbacks.

However, there is no real need for the data structures and code in
question to be arranged this way and if min_sampling_rate is moved to
data structures holding the other governor tunables, the sysfs
attribute creation macros that work with those tunables will also work
with min_sampling_rate and the special macro for it won't be necessary
any more.  That will make it easier to modify the governor code going
forward, so do it."

Thanks,
Rafael

Re: [PATCH 1/5] cpufreq: governor: Kill declare_show_sampling_rate_min()

[Viresh Kumar]

On 02-02-16, 21:23, Rafael J. Wysocki wrote:
> To me, this is not about the macro, but about moving min_sampling_rate
> to governor tunables, so my subject would be something like "cpufreq:
> governor: Treat min_sampling_rate as a governor-specific tunable".
> 
> My changelog, then, would be something like the following:
> 
> "The min_sampling_rate governor tunable is a field in struct dbs_data,
> so it has to be handled in a special way separate from the rest of
> governor tunables.  In particular, that requires a special macro to be
> present for creating its show/store sysfs attribute callbacks.
> 
> However, there is no real need for the data structures and code in
> question to be arranged this way and if min_sampling_rate is moved to
> data structures holding the other governor tunables, the sysfs
> attribute creation macros that work with those tunables will also work
> with min_sampling_rate and the special macro for it won't be necessary
> any more.  That will make it easier to modify the governor code going
> forward, so do it."

I just copy pasted it with:

[ Rafael: Written changelog ]

just before my sign-off. Thanks.

-- 
viresh

[PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

Until now, governors (ondemand/conservative) were using the
'global-attr' or 'freq-attr', depending on the sysfs location where we
want to create governor's directory.

The problem is that, in case of 'freq-attr', we are forced to use
show()/store() present in cpufreq.c, which always take policy->rwsem.

And because of that we were facing some ABBA lockups during governor
callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
event.

That caused further problems and it never worked perfectly.

This patch attempts to fix that by creating separate sysfs-ops for
cpufreq governors.

Because things got much simplified now, we don't need separate
show/store callbacks for governor-for-system and governor-per-policy
cases.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 71 +++++++++++++---------------------
 drivers/cpufreq/cpufreq_governor.c     | 50 +++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     | 31 +++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 71 +++++++++++++---------------------
 4 files changed, 122 insertions(+), 101 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 57750367bd26..980145da796a 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -275,51 +275,35 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(cs, sampling_rate);
-show_store_one(cs, sampling_down_factor);
-show_store_one(cs, up_threshold);
-show_store_one(cs, down_threshold);
-show_store_one(cs, ignore_nice_load);
-show_store_one(cs, freq_step);
-show_one(cs, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(down_threshold);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(freq_step);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&down_threshold_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&freq_step_gov_sys.attr,
+gov_show_one(cs, sampling_rate);
+gov_show_one(cs, sampling_down_factor);
+gov_show_one(cs, up_threshold);
+gov_show_one(cs, down_threshold);
+gov_show_one(cs, ignore_nice_load);
+gov_show_one(cs, freq_step);
+gov_show_one(cs, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(up_threshold);
+gov_attr_rw(down_threshold);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(freq_step);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *dbs_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&sampling_down_factor.attr,
+	&up_threshold.attr,
+	&down_threshold.attr,
+	&ignore_nice_load.attr,
+	&freq_step.attr,
 	NULL
 };
 
-static struct attribute_group cs_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "conservative",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&down_threshold_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&freq_step_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group cs_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
+static struct attribute_group cs_attr_group = {
+	.attrs = dbs_attributes,
 	.name = "conservative",
 };
 
@@ -365,8 +349,7 @@ define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
 static struct common_dbs_data cs_dbs_cdata = {
 	.governor = GOV_CONSERVATIVE,
-	.attr_group_gov_sys = &cs_attr_group_gov_sys,
-	.attr_group_gov_pol = &cs_attr_group_gov_pol,
+	.attr_group = &cs_attr_group,
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = cs_dbs_timer,
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 9a7edc91ad57..e785a118cbdc 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,14 +22,37 @@
 
 #include "cpufreq_governor.h"
 
-static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
+#define to_dbs_data(k) container_of(k, struct dbs_data, kobj)
+#define to_attr(a) container_of(a, struct governor_attr, attr)
+
+static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 {
-	if (have_governor_per_policy())
-		return dbs_data->cdata->attr_group_gov_pol;
-	else
-		return dbs_data->cdata->attr_group_gov_sys;
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_attr(attr);
+
+	if (gattr->show)
+		return gattr->show(dbs_data, buf);
+
+	return -EIO;
+}
+
+static ssize_t store(struct kobject *kobj, struct attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_attr(attr);
+
+	if (gattr->store)
+		return gattr->store(dbs_data, buf, count);
+
+	return -EIO;
 }
 
+static const struct sysfs_ops sysfs_ops = {
+	.show	= show,
+	.store	= store,
+};
+
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 {
 	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
@@ -354,6 +377,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 				 struct dbs_data *dbs_data,
 				 struct common_dbs_data *cdata)
 {
+	struct attribute_group *attr_group;
 	int ret;
 
 	/* State should be equivalent to EXIT */
@@ -395,10 +419,17 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 
 	policy->governor_data = dbs_data;
 
-	ret = sysfs_create_group(get_governor_parent_kobj(policy),
-				 get_sysfs_attr(dbs_data));
-	if (ret)
+	attr_group = dbs_data->cdata->attr_group;
+	dbs_data->kobj_type.sysfs_ops = &sysfs_ops;
+	dbs_data->kobj_type.default_attrs = attr_group->attrs;
+
+	ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
+				   get_governor_parent_kobj(policy),
+				   attr_group->name);
+	if (ret) {
+		pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
 		goto reset_gdbs_data;
+	}
 
 	return 0;
 
@@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
 		return -EBUSY;
 
 	if (!--dbs_data->usage_count) {
-		sysfs_remove_group(get_governor_parent_kobj(policy),
-				   get_sysfs_attr(dbs_data));
+		kobject_put(&dbs_data->kobj);
 
 		policy->governor_data = NULL;
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index ad44a8546a3a..59b28133dd68 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol				\
 show_one(_gov, file_name);						\
 store_one(_gov, file_name)
 
+/* Governor's specific attributes */
+struct dbs_data;
+struct governor_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
+	ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
+			 size_t count);
+};
+
+#define gov_show_one(_gov, file_name)					\
+static ssize_t show_##file_name						\
+(struct dbs_data *dbs_data, char *buf)					\
+{									\
+	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
+	return sprintf(buf, "%u\n", tuners->file_name);			\
+}
+
+#define gov_attr_ro(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0444, show_##_name, NULL)
+
+#define gov_attr_rw(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 /* create helper routines */
 #define define_get_cpu_dbs_routines(_dbs_info)				\
 static struct cpu_dbs_info *get_cpu_cdbs(int cpu)			\
@@ -197,14 +222,12 @@ struct cs_dbs_tuners {
 };
 
 /* Common Governor data across policies */
-struct dbs_data;
 struct common_dbs_data {
 	/* Common across governors */
 	#define GOV_ONDEMAND		0
 	#define GOV_CONSERVATIVE	1
 	int governor;
-	struct attribute_group *attr_group_gov_sys; /* one governor - system */
-	struct attribute_group *attr_group_gov_pol; /* one governor - policy */
+	struct attribute_group *attr_group; /* one governor - system */
 
 	/*
 	 * Common data for platforms that don't set
@@ -234,6 +257,8 @@ struct dbs_data {
 	struct common_dbs_data *cdata;
 	int usage_count;
 	void *tuners;
+	struct kobject kobj;
+	struct kobj_type kobj_type;
 };
 
 /* Governor specific ops, will be passed to dbs_data->gov_ops */
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index b31f64745232..b7983dd02e24 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -436,51 +436,35 @@ static ssize_t store_powersave_bias(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(od, sampling_rate);
-show_store_one(od, io_is_busy);
-show_store_one(od, up_threshold);
-show_store_one(od, sampling_down_factor);
-show_store_one(od, ignore_nice_load);
-show_store_one(od, powersave_bias);
-show_one(od, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(io_is_busy);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(powersave_bias);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&powersave_bias_gov_sys.attr,
-	&io_is_busy_gov_sys.attr,
+gov_show_one(od, sampling_rate);
+gov_show_one(od, io_is_busy);
+gov_show_one(od, up_threshold);
+gov_show_one(od, sampling_down_factor);
+gov_show_one(od, ignore_nice_load);
+gov_show_one(od, powersave_bias);
+gov_show_one(od, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(io_is_busy);
+gov_attr_rw(up_threshold);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(powersave_bias);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *dbs_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&up_threshold.attr,
+	&sampling_down_factor.attr,
+	&ignore_nice_load.attr,
+	&powersave_bias.attr,
+	&io_is_busy.attr,
 	NULL
 };
 
-static struct attribute_group od_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "ondemand",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&powersave_bias_gov_pol.attr,
-	&io_is_busy_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group od_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
+static struct attribute_group od_attr_group = {
+	.attrs = dbs_attributes,
 	.name = "ondemand",
 };
 
@@ -542,8 +526,7 @@ static struct od_ops od_ops = {
 
 static struct common_dbs_data od_dbs_cdata = {
 	.governor = GOV_ONDEMAND,
-	.attr_group_gov_sys = &od_attr_group_gov_sys,
-	.attr_group_gov_pol = &od_attr_group_gov_pol,
+	.attr_group = &od_attr_group,
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = od_dbs_timer,
-- 
2.7.0.79.gdc08a19

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Juri Lelli]

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Until now, governors (ondemand/conservative) were using the
> 'global-attr' or 'freq-attr', depending on the sysfs location where we
> want to create governor's directory.
> 
> The problem is that, in case of 'freq-attr', we are forced to use
> show()/store() present in cpufreq.c, which always take policy->rwsem.
> 
> And because of that we were facing some ABBA lockups during governor
> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
> event.
> 
> That caused further problems and it never worked perfectly.
> 
> This patch attempts to fix that by creating separate sysfs-ops for
> cpufreq governors.
> 
> Because things got much simplified now, we don't need separate
> show/store callbacks for governor-for-system and governor-per-policy
> cases.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This patch cleans things up a lot, that's good.

One thing I'm still concerned about, though: don't we need some locking
in place for some of the store operations on governors attributes? Are
store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
It seems that we can call them from different cpus concurrently.

Best,

- Juri

> ---
>  drivers/cpufreq/cpufreq_conservative.c | 71 +++++++++++++---------------------
>  drivers/cpufreq/cpufreq_governor.c     | 50 +++++++++++++++++++-----
>  drivers/cpufreq/cpufreq_governor.h     | 31 +++++++++++++--
>  drivers/cpufreq/cpufreq_ondemand.c     | 71 +++++++++++++---------------------
>  4 files changed, 122 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 57750367bd26..980145da796a 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -275,51 +275,35 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,
>  	return count;
>  }
>  
> -show_store_one(cs, sampling_rate);
> -show_store_one(cs, sampling_down_factor);
> -show_store_one(cs, up_threshold);
> -show_store_one(cs, down_threshold);
> -show_store_one(cs, ignore_nice_load);
> -show_store_one(cs, freq_step);
> -show_one(cs, min_sampling_rate);
> -
> -gov_sys_pol_attr_rw(sampling_rate);
> -gov_sys_pol_attr_rw(sampling_down_factor);
> -gov_sys_pol_attr_rw(up_threshold);
> -gov_sys_pol_attr_rw(down_threshold);
> -gov_sys_pol_attr_rw(ignore_nice_load);
> -gov_sys_pol_attr_rw(freq_step);
> -gov_sys_pol_attr_ro(min_sampling_rate);
> -
> -static struct attribute *dbs_attributes_gov_sys[] = {
> -	&min_sampling_rate_gov_sys.attr,
> -	&sampling_rate_gov_sys.attr,
> -	&sampling_down_factor_gov_sys.attr,
> -	&up_threshold_gov_sys.attr,
> -	&down_threshold_gov_sys.attr,
> -	&ignore_nice_load_gov_sys.attr,
> -	&freq_step_gov_sys.attr,
> +gov_show_one(cs, sampling_rate);
> +gov_show_one(cs, sampling_down_factor);
> +gov_show_one(cs, up_threshold);
> +gov_show_one(cs, down_threshold);
> +gov_show_one(cs, ignore_nice_load);
> +gov_show_one(cs, freq_step);
> +gov_show_one(cs, min_sampling_rate);
> +
> +gov_attr_rw(sampling_rate);
> +gov_attr_rw(sampling_down_factor);
> +gov_attr_rw(up_threshold);
> +gov_attr_rw(down_threshold);
> +gov_attr_rw(ignore_nice_load);
> +gov_attr_rw(freq_step);
> +gov_attr_ro(min_sampling_rate);
> +
> +static struct attribute *dbs_attributes[] = {
> +	&min_sampling_rate.attr,
> +	&sampling_rate.attr,
> +	&sampling_down_factor.attr,
> +	&up_threshold.attr,
> +	&down_threshold.attr,
> +	&ignore_nice_load.attr,
> +	&freq_step.attr,
>  	NULL
>  };
>  
> -static struct attribute_group cs_attr_group_gov_sys = {
> -	.attrs = dbs_attributes_gov_sys,
> -	.name = "conservative",
> -};
> -
> -static struct attribute *dbs_attributes_gov_pol[] = {
> -	&min_sampling_rate_gov_pol.attr,
> -	&sampling_rate_gov_pol.attr,
> -	&sampling_down_factor_gov_pol.attr,
> -	&up_threshold_gov_pol.attr,
> -	&down_threshold_gov_pol.attr,
> -	&ignore_nice_load_gov_pol.attr,
> -	&freq_step_gov_pol.attr,
> -	NULL
> -};
> -
> -static struct attribute_group cs_attr_group_gov_pol = {
> -	.attrs = dbs_attributes_gov_pol,
> +static struct attribute_group cs_attr_group = {
> +	.attrs = dbs_attributes,
>  	.name = "conservative",
>  };
>  
> @@ -365,8 +349,7 @@ define_get_cpu_dbs_routines(cs_cpu_dbs_info);
>  
>  static struct common_dbs_data cs_dbs_cdata = {
>  	.governor = GOV_CONSERVATIVE,
> -	.attr_group_gov_sys = &cs_attr_group_gov_sys,
> -	.attr_group_gov_pol = &cs_attr_group_gov_pol,
> +	.attr_group = &cs_attr_group,
>  	.get_cpu_cdbs = get_cpu_cdbs,
>  	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
>  	.gov_dbs_timer = cs_dbs_timer,
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 9a7edc91ad57..e785a118cbdc 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -22,14 +22,37 @@
>  
>  #include "cpufreq_governor.h"
>  
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +#define to_dbs_data(k) container_of(k, struct dbs_data, kobj)
> +#define to_attr(a) container_of(a, struct governor_attr, attr)
> +
> +static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  {
> -	if (have_governor_per_policy())
> -		return dbs_data->cdata->attr_group_gov_pol;
> -	else
> -		return dbs_data->cdata->attr_group_gov_sys;
> +	struct dbs_data *dbs_data = to_dbs_data(kobj);
> +	struct governor_attr *gattr = to_attr(attr);
> +
> +	if (gattr->show)
> +		return gattr->show(dbs_data, buf);
> +
> +	return -EIO;
> +}
> +
> +static ssize_t store(struct kobject *kobj, struct attribute *attr,
> +		     const char *buf, size_t count)
> +{
> +	struct dbs_data *dbs_data = to_dbs_data(kobj);
> +	struct governor_attr *gattr = to_attr(attr);
> +
> +	if (gattr->store)
> +		return gattr->store(dbs_data, buf, count);
> +
> +	return -EIO;
>  }
>  
> +static const struct sysfs_ops sysfs_ops = {
> +	.show	= show,
> +	.store	= store,
> +};
> +
>  void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  {
>  	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> @@ -354,6 +377,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>  				 struct dbs_data *dbs_data,
>  				 struct common_dbs_data *cdata)
>  {
> +	struct attribute_group *attr_group;
>  	int ret;
>  
>  	/* State should be equivalent to EXIT */
> @@ -395,10 +419,17 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>  
>  	policy->governor_data = dbs_data;
>  
> -	ret = sysfs_create_group(get_governor_parent_kobj(policy),
> -				 get_sysfs_attr(dbs_data));
> -	if (ret)
> +	attr_group = dbs_data->cdata->attr_group;
> +	dbs_data->kobj_type.sysfs_ops = &sysfs_ops;
> +	dbs_data->kobj_type.default_attrs = attr_group->attrs;
> +
> +	ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
> +				   get_governor_parent_kobj(policy),
> +				   attr_group->name);
> +	if (ret) {
> +		pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
>  		goto reset_gdbs_data;
> +	}
>  
>  	return 0;
>  
> @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>  		return -EBUSY;
>  
>  	if (!--dbs_data->usage_count) {
> -		sysfs_remove_group(get_governor_parent_kobj(policy),
> -				   get_sysfs_attr(dbs_data));
> +		kobject_put(&dbs_data->kobj);
>  
>  		policy->governor_data = NULL;
>  
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ad44a8546a3a..59b28133dd68 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol				\
>  show_one(_gov, file_name);						\
>  store_one(_gov, file_name)
>  
> +/* Governor's specific attributes */
> +struct dbs_data;
> +struct governor_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
> +	ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
> +			 size_t count);
> +};
> +
> +#define gov_show_one(_gov, file_name)					\
> +static ssize_t show_##file_name						\
> +(struct dbs_data *dbs_data, char *buf)					\
> +{									\
> +	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
> +	return sprintf(buf, "%u\n", tuners->file_name);			\
> +}
> +
> +#define gov_attr_ro(_name)						\
> +static struct governor_attr _name =					\
> +__ATTR(_name, 0444, show_##_name, NULL)
> +
> +#define gov_attr_rw(_name)						\
> +static struct governor_attr _name =					\
> +__ATTR(_name, 0644, show_##_name, store_##_name)
> +
>  /* create helper routines */
>  #define define_get_cpu_dbs_routines(_dbs_info)				\
>  static struct cpu_dbs_info *get_cpu_cdbs(int cpu)			\
> @@ -197,14 +222,12 @@ struct cs_dbs_tuners {
>  };
>  
>  /* Common Governor data across policies */
> -struct dbs_data;
>  struct common_dbs_data {
>  	/* Common across governors */
>  	#define GOV_ONDEMAND		0
>  	#define GOV_CONSERVATIVE	1
>  	int governor;
> -	struct attribute_group *attr_group_gov_sys; /* one governor - system */
> -	struct attribute_group *attr_group_gov_pol; /* one governor - policy */
> +	struct attribute_group *attr_group; /* one governor - system */
>  
>  	/*
>  	 * Common data for platforms that don't set
> @@ -234,6 +257,8 @@ struct dbs_data {
>  	struct common_dbs_data *cdata;
>  	int usage_count;
>  	void *tuners;
> +	struct kobject kobj;
> +	struct kobj_type kobj_type;
>  };
>  
>  /* Governor specific ops, will be passed to dbs_data->gov_ops */
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index b31f64745232..b7983dd02e24 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -436,51 +436,35 @@ static ssize_t store_powersave_bias(struct dbs_data *dbs_data, const char *buf,
>  	return count;
>  }
>  
> -show_store_one(od, sampling_rate);
> -show_store_one(od, io_is_busy);
> -show_store_one(od, up_threshold);
> -show_store_one(od, sampling_down_factor);
> -show_store_one(od, ignore_nice_load);
> -show_store_one(od, powersave_bias);
> -show_one(od, min_sampling_rate);
> -
> -gov_sys_pol_attr_rw(sampling_rate);
> -gov_sys_pol_attr_rw(io_is_busy);
> -gov_sys_pol_attr_rw(up_threshold);
> -gov_sys_pol_attr_rw(sampling_down_factor);
> -gov_sys_pol_attr_rw(ignore_nice_load);
> -gov_sys_pol_attr_rw(powersave_bias);
> -gov_sys_pol_attr_ro(min_sampling_rate);
> -
> -static struct attribute *dbs_attributes_gov_sys[] = {
> -	&min_sampling_rate_gov_sys.attr,
> -	&sampling_rate_gov_sys.attr,
> -	&up_threshold_gov_sys.attr,
> -	&sampling_down_factor_gov_sys.attr,
> -	&ignore_nice_load_gov_sys.attr,
> -	&powersave_bias_gov_sys.attr,
> -	&io_is_busy_gov_sys.attr,
> +gov_show_one(od, sampling_rate);
> +gov_show_one(od, io_is_busy);
> +gov_show_one(od, up_threshold);
> +gov_show_one(od, sampling_down_factor);
> +gov_show_one(od, ignore_nice_load);
> +gov_show_one(od, powersave_bias);
> +gov_show_one(od, min_sampling_rate);
> +
> +gov_attr_rw(sampling_rate);
> +gov_attr_rw(io_is_busy);
> +gov_attr_rw(up_threshold);
> +gov_attr_rw(sampling_down_factor);
> +gov_attr_rw(ignore_nice_load);
> +gov_attr_rw(powersave_bias);
> +gov_attr_ro(min_sampling_rate);
> +
> +static struct attribute *dbs_attributes[] = {
> +	&min_sampling_rate.attr,
> +	&sampling_rate.attr,
> +	&up_threshold.attr,
> +	&sampling_down_factor.attr,
> +	&ignore_nice_load.attr,
> +	&powersave_bias.attr,
> +	&io_is_busy.attr,
>  	NULL
>  };
>  
> -static struct attribute_group od_attr_group_gov_sys = {
> -	.attrs = dbs_attributes_gov_sys,
> -	.name = "ondemand",
> -};
> -
> -static struct attribute *dbs_attributes_gov_pol[] = {
> -	&min_sampling_rate_gov_pol.attr,
> -	&sampling_rate_gov_pol.attr,
> -	&up_threshold_gov_pol.attr,
> -	&sampling_down_factor_gov_pol.attr,
> -	&ignore_nice_load_gov_pol.attr,
> -	&powersave_bias_gov_pol.attr,
> -	&io_is_busy_gov_pol.attr,
> -	NULL
> -};
> -
> -static struct attribute_group od_attr_group_gov_pol = {
> -	.attrs = dbs_attributes_gov_pol,
> +static struct attribute_group od_attr_group = {
> +	.attrs = dbs_attributes,
>  	.name = "ondemand",
>  };
>  
> @@ -542,8 +526,7 @@ static struct od_ops od_ops = {
>  
>  static struct common_dbs_data od_dbs_cdata = {
>  	.governor = GOV_ONDEMAND,
> -	.attr_group_gov_sys = &od_attr_group_gov_sys,
> -	.attr_group_gov_pol = &od_attr_group_gov_pol,
> +	.attr_group = &od_attr_group,
>  	.get_cpu_cdbs = get_cpu_cdbs,
>  	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
>  	.gov_dbs_timer = od_dbs_timer,
> -- 
> 2.7.0.79.gdc08a19
> 

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
> Hi Viresh,
>
> On 02/02/16 16:27, Viresh Kumar wrote:
>> Until now, governors (ondemand/conservative) were using the
>> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>> want to create governor's directory.
>>
>> The problem is that, in case of 'freq-attr', we are forced to use
>> show()/store() present in cpufreq.c, which always take policy->rwsem.
>>
>> And because of that we were facing some ABBA lockups during governor
>> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
>> event.
>>
>> That caused further problems and it never worked perfectly.
>>
>> This patch attempts to fix that by creating separate sysfs-ops for
>> cpufreq governors.
>>
>> Because things got much simplified now, we don't need separate
>> show/store callbacks for governor-for-system and governor-per-policy
>> cases.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> This patch cleans things up a lot, that's good.
>
> One thing I'm still concerned about, though: don't we need some locking
> in place for some of the store operations on governors attributes? Are
> store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?

That would require some investigation I suppose.

> It seems that we can call them from different cpus concurrently.

Yes, we can.

One quick-and-dirty way of dealing with that might be to introduce a
"sysfs lock" into struct dbs_data and hold that around the invocation
of gattr->store() in the sysfs_ops's ->store callback.

>
> Best,
>
> - Juri

BTW, you could have dropped the stuff below this line from your reply
message.  That at least would have prevented tools like Patchwork from
storing useless garbage.

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Juri Lelli]

Hi Rafael,

On 02/02/16 17:35, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
> > Hi Viresh,
> >
> > On 02/02/16 16:27, Viresh Kumar wrote:
> >> Until now, governors (ondemand/conservative) were using the
> >> 'global-attr' or 'freq-attr', depending on the sysfs location where we
> >> want to create governor's directory.
> >>
> >> The problem is that, in case of 'freq-attr', we are forced to use
> >> show()/store() present in cpufreq.c, which always take policy->rwsem.
> >>
> >> And because of that we were facing some ABBA lockups during governor
> >> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
> >> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
> >> event.
> >>
> >> That caused further problems and it never worked perfectly.
> >>
> >> This patch attempts to fix that by creating separate sysfs-ops for
> >> cpufreq governors.
> >>
> >> Because things got much simplified now, we don't need separate
> >> show/store callbacks for governor-for-system and governor-per-policy
> >> cases.
> >>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > This patch cleans things up a lot, that's good.
> >
> > One thing I'm still concerned about, though: don't we need some locking
> > in place for some of the store operations on governors attributes? Are
> > store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
> 
> That would require some investigation I suppose.
> 
> > It seems that we can call them from different cpus concurrently.
> 
> Yes, we can.
> 
> One quick-and-dirty way of dealing with that might be to introduce a
> "sysfs lock" into struct dbs_data and hold that around the invocation
> of gattr->store() in the sysfs_ops's ->store callback.
> 

There is value in trying to solve this issue by using some of the
existing locks, IMHO.

Can't we actually try to use the policy->rwsem (or one of the core
locks) + wait_for_completion approach as we do in cpufreq core?

> BTW, you could have dropped the stuff below this line from your reply
> message.  That at least would have prevented tools like Patchwork from
> storing useless garbage.
> 

Right. Sorry for the garbage; I'll check twice that I trim my replies in
the future.

Best,

- Juri

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
> Hi Rafael,
>
> On 02/02/16 17:35, Rafael J. Wysocki wrote:
>> On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>> > Hi Viresh,
>> >
>> > On 02/02/16 16:27, Viresh Kumar wrote:
>> >> Until now, governors (ondemand/conservative) were using the
>> >> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>> >> want to create governor's directory.
>> >>
>> >> The problem is that, in case of 'freq-attr', we are forced to use
>> >> show()/store() present in cpufreq.c, which always take policy->rwsem.
>> >>
>> >> And because of that we were facing some ABBA lockups during governor
>> >> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>> >> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
>> >> event.
>> >>
>> >> That caused further problems and it never worked perfectly.
>> >>
>> >> This patch attempts to fix that by creating separate sysfs-ops for
>> >> cpufreq governors.
>> >>
>> >> Because things got much simplified now, we don't need separate
>> >> show/store callbacks for governor-for-system and governor-per-policy
>> >> cases.
>> >>
>> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >
>> > This patch cleans things up a lot, that's good.
>> >
>> > One thing I'm still concerned about, though: don't we need some locking
>> > in place for some of the store operations on governors attributes? Are
>> > store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
>>
>> That would require some investigation I suppose.
>>
>> > It seems that we can call them from different cpus concurrently.
>>
>> Yes, we can.
>>
>> One quick-and-dirty way of dealing with that might be to introduce a
>> "sysfs lock" into struct dbs_data and hold that around the invocation
>> of gattr->store() in the sysfs_ops's ->store callback.
>>
>
> There is value in trying to solve this issue by using some of the
> existing locks, IMHO.

Some value - maybe.  I'm not sure how much of it, though.

Finer-grained locking is generally easier to follow, because the locks
tend to be used for specific purposes only.

> Can't we actually try to use the policy->rwsem (or one of the core
> locks) + wait_for_completion approach as we do in cpufreq core?

No.  Too many things depend on that lock already and some of them work
by accident rather than by design.

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>> Hi Rafael,
>>
>> On 02/02/16 17:35, Rafael J. Wysocki wrote:
>>> On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>> Hi Viresh,
>>>>
>>>> On 02/02/16 16:27, Viresh Kumar wrote:
>>>>> Until now, governors (ondemand/conservative) were using the
>>>>> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>>>>> want to create governor's directory.
>>>>>
>>>>> The problem is that, in case of 'freq-attr', we are forced to use
>>>>> show()/store() present in cpufreq.c, which always take policy->rwsem.
>>>>>
>>>>> And because of that we were facing some ABBA lockups during governor
>>>>> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>>>>> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
>>>>> event.
>>>>>
>>>>> That caused further problems and it never worked perfectly.
>>>>>
>>>>> This patch attempts to fix that by creating separate sysfs-ops for
>>>>> cpufreq governors.
>>>>>
>>>>> Because things got much simplified now, we don't need separate
>>>>> show/store callbacks for governor-for-system and governor-per-policy
>>>>> cases.
>>>>>
>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>
>>>> This patch cleans things up a lot, that's good.
>>>>
>>>> One thing I'm still concerned about, though: don't we need some locking
>>>> in place for some of the store operations on governors attributes? Are
>>>> store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
>>>
>>> That would require some investigation I suppose.
>>>
>>>> It seems that we can call them from different cpus concurrently.
>>>
>>> Yes, we can.
>>>
>>> One quick-and-dirty way of dealing with that might be to introduce a
>>> "sysfs lock" into struct dbs_data and hold that around the invocation
>>> of gattr->store() in the sysfs_ops's ->store callback.
>>>
>>
>> There is value in trying to solve this issue by using some of the
>> existing locks, IMHO.
>
> Some value - maybe.  I'm not sure how much of it, though.
>
> Finer-grained locking is generally easier to follow, because the locks
> tend to be used for specific purposes only.
>
>> Can't we actually try to use the policy->rwsem (or one of the core
>> locks) + wait_for_completion approach as we do in cpufreq core?
>
> No.  Too many things depend on that lock already and some of them work
> by accident rather than by design.

Also, wait_for_completion() and complete() is just another way to 
implement a lock. So, it won't necessarily solve any deadlock issues.

I also don't like this patch because it forces governors to either 
implement their own macros and management of their attributes or force 
them to use the governor structs that come with cpufreq_governor.h. 
cpufreq_governor.h IMHO is very ondemand and conservative governor 
specific and is very irrelevant for sched-dvfs or any other governors 
(hint hint).

The only time this ABBA locking is an issue is when governor are 
changing and trying to add/remove attributes. That can easily be checked 
in store_governor and dealt with without holding the policy rwsem if the 
governors can provide their per sys and per policy attribute arrays as 
part of registering themselves.

I'm sorry that I just keep talking about the idea and not sending out 
the patches.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>
>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>
>>> Hi Rafael,
>>>
>>> On 02/02/16 17:35, Rafael J. Wysocki wrote:
>>>>
>>>> On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>
>>>>> Hi Viresh,
>>>>>
>>>>> On 02/02/16 16:27, Viresh Kumar wrote:
>>>>>>
>>>>>> Until now, governors (ondemand/conservative) were using the
>>>>>> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>>>>>> want to create governor's directory.
>>>>>>
>>>>>> The problem is that, in case of 'freq-attr', we are forced to use
>>>>>> show()/store() present in cpufreq.c, which always take policy->rwsem.
>>>>>>
>>>>>> And because of that we were facing some ABBA lockups during governor
>>>>>> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>>>>>> rwsem right before calling governor callback for
>>>>>> CPUFREQ_GOV_POLICY_EXIT
>>>>>> event.
>>>>>>
>>>>>> That caused further problems and it never worked perfectly.
>>>>>>
>>>>>> This patch attempts to fix that by creating separate sysfs-ops for
>>>>>> cpufreq governors.
>>>>>>
>>>>>> Because things got much simplified now, we don't need separate
>>>>>> show/store callbacks for governor-for-system and governor-per-policy
>>>>>> cases.
>>>>>>
>>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>
>>>>>
>>>>> This patch cleans things up a lot, that's good.
>>>>>
>>>>> One thing I'm still concerned about, though: don't we need some locking
>>>>> in place for some of the store operations on governors attributes? Are
>>>>> store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
>>>>
>>>>
>>>> That would require some investigation I suppose.
>>>>
>>>>> It seems that we can call them from different cpus concurrently.
>>>>
>>>>
>>>> Yes, we can.
>>>>
>>>> One quick-and-dirty way of dealing with that might be to introduce a
>>>> "sysfs lock" into struct dbs_data and hold that around the invocation
>>>> of gattr->store() in the sysfs_ops's ->store callback.
>>>>
>>>
>>> There is value in trying to solve this issue by using some of the
>>> existing locks, IMHO.
>>
>>
>> Some value - maybe.  I'm not sure how much of it, though.
>>
>> Finer-grained locking is generally easier to follow, because the locks
>> tend to be used for specific purposes only.
>>
>>> Can't we actually try to use the policy->rwsem (or one of the core
>>> locks) + wait_for_completion approach as we do in cpufreq core?
>>
>>
>> No.  Too many things depend on that lock already and some of them work
>> by accident rather than by design.
>
>
> Also, wait_for_completion() and complete() is just another way to implement
> a lock. So, it won't necessarily solve any deadlock issues.
>
> I also don't like this patch because it forces governors to either implement
> their own macros and management of their attributes or force them to use the
> governor structs that come with cpufreq_governor.h. cpufreq_governor.h IMHO
> is very ondemand and conservative governor specific and is very irrelevant
> for sched-dvfs or any other governors (hint hint).
>
> The only time this ABBA locking is an issue is when governor are changing
> and trying to add/remove attributes. That can easily be checked in
> store_governor and dealt with without holding the policy rwsem if the
> governors can provide their per sys and per policy attribute arrays as part
> of registering themselves.
>
> I'm sorry that I just keep talking about the idea and not sending out the
> patches.

I think you have a point, though.

The deadlock really is specific to the governors using the code in
cpufreq_governor.c.

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>>
>>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>

[cut]

>>
>> I also don't like this patch because it forces governors to either implement
>> their own macros and management of their attributes or force them to use the
>> governor structs that come with cpufreq_governor.h. cpufreq_governor.h IMHO
>> is very ondemand and conservative governor specific and is very irrelevant
>> for sched-dvfs or any other governors (hint hint).
>>
>> The only time this ABBA locking is an issue is when governor are changing
>> and trying to add/remove attributes. That can easily be checked in
>> store_governor and dealt with without holding the policy rwsem if the
>> governors can provide their per sys and per policy attribute arrays as part
>> of registering themselves.
>>
>> I'm sorry that I just keep talking about the idea and not sending out the
>> patches.
>
> I think you have a point, though.
>
> The deadlock really is specific to the governors using the code in
> cpufreq_governor.c.

That said no other governors in the tree use any sysfs attributes for
tunables AFAICS and the out-of-the tree ones are out of interest here.

Also the deadlock happens if one of the tunable attributes is accessed
while we're trying to remove it which very well may happen on read
access too.

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 05:07 PM, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>>> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>>>
>>>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>
>
> [cut]
>
>>>
>>> I also don't like this patch because it forces governors to either implement
>>> their own macros and management of their attributes or force them to use the
>>> governor structs that come with cpufreq_governor.h. cpufreq_governor.h IMHO
>>> is very ondemand and conservative governor specific and is very irrelevant
>>> for sched-dvfs or any other governors (hint hint).
>>>
>>> The only time this ABBA locking is an issue is when governor are changing
>>> and trying to add/remove attributes. That can easily be checked in
>>> store_governor and dealt with without holding the policy rwsem if the
>>> governors can provide their per sys and per policy attribute arrays as part
>>> of registering themselves.
>>>
>>> I'm sorry that I just keep talking about the idea and not sending out the
>>> patches.
>>
>> I think you have a point, though.
>>
>> The deadlock really is specific to the governors using the code in
>> cpufreq_governor.c.
>
> That said no other governors in the tree use any sysfs attributes for
> tunables AFAICS and the out-of-the tree ones are out of interest here.

But if we are expecting sched dvfs to come in, why make it worse for it. 
It would be completely pointless to try and shoehorn sched dvfs to use 
cpufreq_governor.c

> Also the deadlock happens if one of the tunable attributes is accessed
> while we're trying to remove it which very well may happen on read
> access too.

Isn't this THE deadlock we are talking about? The removal of the 
attributes only happen when governors are changes and we send a 
POLICY_EXIT and or all the cores are hotplugged out. And my suggestion 
would work just as well there.

Why are you prefixing your sentence with "Also"? Is there some other 
case I'm not considering?

-Saravana
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 2:32 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 02/02/2016 05:07 PM, Rafael J. Wysocki wrote:
>>
>> On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@kernel.org>
>> wrote:
>>>
>>> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org>
>>> wrote:
>>>>
>>>> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>>>>
>>>>>
>>>>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>>
>>>>>>
>>
>> [cut]
>>
>>>>
>>>> I also don't like this patch because it forces governors to either
>>>> implement
>>>> their own macros and management of their attributes or force them to use
>>>> the
>>>> governor structs that come with cpufreq_governor.h. cpufreq_governor.h
>>>> IMHO
>>>> is very ondemand and conservative governor specific and is very
>>>> irrelevant
>>>> for sched-dvfs or any other governors (hint hint).
>>>>
>>>> The only time this ABBA locking is an issue is when governor are
>>>> changing
>>>> and trying to add/remove attributes. That can easily be checked in
>>>> store_governor and dealt with without holding the policy rwsem if the
>>>> governors can provide their per sys and per policy attribute arrays as
>>>> part
>>>> of registering themselves.
>>>>
>>>> I'm sorry that I just keep talking about the idea and not sending out
>>>> the
>>>> patches.
>>>
>>>
>>> I think you have a point, though.
>>>
>>> The deadlock really is specific to the governors using the code in
>>> cpufreq_governor.c.
>>
>>
>> That said no other governors in the tree use any sysfs attributes for
>> tunables AFAICS and the out-of-the tree ones are out of interest here.
>
>
> But if we are expecting sched dvfs to come in, why make it worse for it. It
> would be completely pointless to try and shoehorn sched dvfs to use
> cpufreq_governor.c

Well, do you honestly think that using the existing stuff in it would
be a good idea?

If not, then why it matters at all?

>> Also the deadlock happens if one of the tunable attributes is accessed
>> while we're trying to remove it which very well may happen on read
>> access too.
>
> Isn't this THE deadlock we are talking about? The removal of the attributes
> only happen when governors are changes and we send a POLICY_EXIT and or all
> the cores are hotplugged out.

It generally happens when the "old" governor is going away, whatever the reason.

> And my suggestion would work just as well there.
>
> Why are you prefixing your sentence with "Also"? Is there some other case
> I'm not considering?

Say someone is reading sampling_rate for a policy with 1 CPU in it and
someone else is taking the CPU offline.  The governor EXIT code path
(that will trigger as a result) will try to remove the sampling_rate
attribute and (if it does that under policy->rwsem) it'll wait for the
read access to finish.  Where exactly would you put the deadlock
prevention in this case?

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 05:52 PM, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2016 at 2:32 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> On 02/02/2016 05:07 PM, Rafael J. Wysocki wrote:
>>>
>>> On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@kernel.org>
>>> wrote:
>>>>
>>>> On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@codeaurora.org>
>>>> wrote:
>>>>>
>>>>> On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>>>>>>
>>>>>>>
>>>
>>> [cut]
>>>
>>>>>
>>>>> I also don't like this patch because it forces governors to either
>>>>> implement
>>>>> their own macros and management of their attributes or force them to use
>>>>> the
>>>>> governor structs that come with cpufreq_governor.h. cpufreq_governor.h
>>>>> IMHO
>>>>> is very ondemand and conservative governor specific and is very
>>>>> irrelevant
>>>>> for sched-dvfs or any other governors (hint hint).
>>>>>
>>>>> The only time this ABBA locking is an issue is when governor are
>>>>> changing
>>>>> and trying to add/remove attributes. That can easily be checked in
>>>>> store_governor and dealt with without holding the policy rwsem if the
>>>>> governors can provide their per sys and per policy attribute arrays as
>>>>> part
>>>>> of registering themselves.
>>>>>
>>>>> I'm sorry that I just keep talking about the idea and not sending out
>>>>> the
>>>>> patches.
>>>>
>>>>
>>>> I think you have a point, though.
>>>>
>>>> The deadlock really is specific to the governors using the code in
>>>> cpufreq_governor.c.
>>>
>>>
>>> That said no other governors in the tree use any sysfs attributes for
>>> tunables AFAICS and the out-of-the tree ones are out of interest here.
>>
>>
>> But if we are expecting sched dvfs to come in, why make it worse for it. It
>> would be completely pointless to try and shoehorn sched dvfs to use
>> cpufreq_governor.c
>
> Well, do you honestly think that using the existing stuff in it would
> be a good idea?
>
> If not, then why it matters at all?
>
>>> Also the deadlock happens if one of the tunable attributes is accessed
>>> while we're trying to remove it which very well may happen on read
>>> access too.
>>
>> Isn't this THE deadlock we are talking about? The removal of the attributes
>> only happen when governors are changes and we send a POLICY_EXIT and or all
>> the cores are hotplugged out.
>
> It generally happens when the "old" governor is going away, whatever the reason.
>
>> And my suggestion would work just as well there.
>>
>> Why are you prefixing your sentence with "Also"? Is there some other case
>> I'm not considering?
>
> Say someone is reading sampling_rate for a policy with 1 CPU in it and
> someone else is taking the CPU offline.  The governor EXIT code path
> (that will trigger as a result) will try to remove the sampling_rate
> attribute and (if it does that under policy->rwsem) it'll wait for the
> read access to finish.  Where exactly would you put the deadlock
> prevention in this case?

This is the hotplug case I mentioned. The sysfs file removals will 
happen only for the last CPU in that policy (we thankfully optimized 
that part last year). We also know that multiple CPUs can't be 
hotplugged at the same time. So, in the start of 
cpufreq_offline_prepare, we just need to check if this is the last CPU 
in the policy and if that's the case, do the gov sysfs remove and then 
grab the policy lock and do all our crap. If a read is going on, that's 
going to finish before the sysfs attr remove can go ahead and it can 
grab the policy lock if it needs to and that still won't cause a 
deadlock because we haven't yet grabbed the policy lock in 
cpufreq_offline_prepare(). If the read comes after the sysfs remove, 
then the read is obviously going to fail (we can depend on the sysfs 
framework on doing its job there).

Will that still leave any race conditions in?

-Saravana
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 02-02-16, 20:03, Saravana Kannan wrote:
> This is the hotplug case I mentioned. The sysfs file removals will happen
> only for the last CPU in that policy (we thankfully optimized that part last
> year). We also know that multiple CPUs can't be hotplugged at the same time.
> So, in the start of cpufreq_offline_prepare, we just need to check if this
> is the last CPU in the policy and if that's the case, do the gov sysfs
> remove and then grab the policy lock and do all our crap. If a read is going
> on, that's going to finish before the sysfs attr remove can go ahead and it
> can grab the policy lock if it needs to and that still won't cause a
> deadlock because we haven't yet grabbed the policy lock in
> cpufreq_offline_prepare(). If the read comes after the sysfs remove, then
> the read is obviously going to fail (we can depend on the sysfs framework on
> doing its job there).

IMHO, these are all dirty hacks we should stay away from. Adding such
hunks in code is considered a band-aid kind of solution and hurts
readability badly. The new solution (new governor show/store)
implement this in a very clean and proper way I feel..

Others are free to disagree though :)

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 10:57 PM, Viresh Kumar wrote:
> On 02-02-16, 20:03, Saravana Kannan wrote:
>> This is the hotplug case I mentioned. The sysfs file removals will happen
>> only for the last CPU in that policy (we thankfully optimized that part last
>> year). We also know that multiple CPUs can't be hotplugged at the same time.
>> So, in the start of cpufreq_offline_prepare, we just need to check if this
>> is the last CPU in the policy and if that's the case, do the gov sysfs
>> remove and then grab the policy lock and do all our crap. If a read is going
>> on, that's going to finish before the sysfs attr remove can go ahead and it
>> can grab the policy lock if it needs to and that still won't cause a
>> deadlock because we haven't yet grabbed the policy lock in
>> cpufreq_offline_prepare(). If the read comes after the sysfs remove, then
>> the read is obviously going to fail (we can depend on the sysfs framework on
>> doing its job there).
>
> IMHO, these are all dirty hacks we should stay away from. Adding such
> hunks in code is considered a band-aid kind of solution and hurts
> readability badly. The new solution (new governor show/store)
> implement this in a very clean and proper way I feel..
>
> Others are free to disagree though :)
>

I think it looks clean since we haven't sorted out the race conditions 
that Juri pointed out. So, it's early to call this series clean :)

Also, I don't see it as a dirty hack at all. What's so hacky about it? 
We are just identifying conditions when we'll have to remove the sysfs 
files and removing them before grabbing the policy lock.

The unlock/lock that we have now is what is a dirty hack.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 02-02-16, 17:32, Saravana Kannan wrote:
> But if we are expecting sched dvfs to come in, why make it worse for it. It
> would be completely pointless to try and shoehorn sched dvfs to use
> cpufreq_governor.c

We can move the common part to cpufreq core and not make sched-dvfs
reuse cpufreq_governor.c

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Juri Lelli]

On 03/02/16 12:24, Viresh Kumar wrote:
> On 02-02-16, 17:32, Saravana Kannan wrote:
> > But if we are expecting sched dvfs to come in, why make it worse for it. It
> > would be completely pointless to try and shoehorn sched dvfs to use
> > cpufreq_governor.c
> 
> We can move the common part to cpufreq core and not make sched-dvfs
> reuse cpufreq_governor.c
> 

I also think that sched-dvfs should not use cpufreq_governor.c.  It is
useful boilerplate code for ondemand and conservative, as they share lot
of data structures and how they work, but it doesn't necessarily suit
everybody's needs, IMHO.

OTOH, fixing the current issue in the best way we can come up with has
still value of course :).

Best,

- Juri

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 03-02-16, 10:51, Juri Lelli wrote:
> I also think that sched-dvfs should not use cpufreq_governor.c.  It is
> useful boilerplate code for ondemand and conservative, as they share lot
> of data structures and how they work, but it doesn't necessarily suit
> everybody's needs, IMHO.
> 
> OTOH, fixing the current issue in the best way we can come up with has
> still value of course :).

Right. cpufreq_governor.c is more about the technique where we do load
evaluation using deferred timers and workqueues, which isn't required
for sched-dvfs.

We can just move the common parts, like, governor_show/governor_store
routines, and the new macros being added here.

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 10:54 PM, Viresh Kumar wrote:
> On 02-02-16, 17:32, Saravana Kannan wrote:
>> But if we are expecting sched dvfs to come in, why make it worse for it. It
>> would be completely pointless to try and shoehorn sched dvfs to use
>> cpufreq_governor.c
>
> We can move the common part to cpufreq core and not make sched-dvfs
> reuse cpufreq_governor.c
>

Let's do this please.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 02-02-16, 14:21, Saravana Kannan wrote:
> I also don't like this patch because it forces governors to either implement
> their own macros and management of their attributes or force them to use the
> governor structs that come with cpufreq_governor.h. cpufreq_governor.h IMHO
> is very ondemand and conservative governor specific and is very irrelevant
> for sched-dvfs or any other governors (hint hint).

But who is stopping us from breaking that file and moving some of it
into include/linux/cpufreq.h ?

We can do that today as well, but it would be fine to do that, when we
add more governors to the core. Though, it would only take a simple
patch if people want me to do it now.

> The only time this ABBA locking is an issue is when governor are changing
> and trying to add/remove attributes. That can easily be checked in
> store_governor

store_scaling_governor ??

> and dealt with without holding the policy rwsem if the

Are you saying that we could have taken the rwsem from the generic
cpufreq.c:store() and dropped it from store_scaling_governor() ? That
would have been something similar to what I tried earlier, which I
never posted (I gave the link to that few days back).

> governors can provide their per sys and per policy attribute arrays as part
> of registering themselves.

These per-sys and per-policy attributes really suck. There is nothing
really different in the implementation, just that the show/store
callbacks have different prototype. One accept 'kboj' as the parameter,
other accept 'policy'. I would call that a HACK as well (I only
implemented it though).

That should just die. A single list of attributes is what we should
have had initially as well.

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 02-02-16, 17:01, Juri Lelli wrote:
> Hi Rafael,
> 
> On 02/02/16 17:35, Rafael J. Wysocki wrote:
> > On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@arm.com> wrote:

> > > This patch cleans things up a lot, that's good.
> > >
> > > One thing I'm still concerned about, though: don't we need some locking
> > > in place for some of the store operations on governors attributes? Are
> > > store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?
> > 
> > That would require some investigation I suppose.

Yeah, that protection is required. Sorry about that.

> > > It seems that we can call them from different cpus concurrently.
> > 
> > Yes, we can.
> > 
> > One quick-and-dirty way of dealing with that might be to introduce a
> > "sysfs lock" into struct dbs_data and hold that around the invocation
> > of gattr->store() in the sysfs_ops's ->store callback.

s/dirty/sane ? :)

> Can't we actually try to use the policy->rwsem (or one of the core
> locks) + wait_for_completion approach as we do in cpufreq core?

policy->rwsem will defeat the purpose of this change as it will
reintroduce the ABBA issue.

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

First, the subject might be better.  What about something like
"cpufreq: governor: New sysfs show/store callbacks for governor
tunables", for example?

> Until now, governors (ondemand/conservative) were using the
> 'global-attr' or 'freq-attr', depending on the sysfs location where we
> want to create governor's directory.

"The ondemand and conservative governors use the global-attr or
freq-attr structures to represent sysfs attributes corresponding to
their tunables (which of them is actually used depends on whether or
not different policy objects can use different governors at the same
time and, consequently, on where those attributes are located in
sysfs).

Unfortunately, in the freq-attr case, the standard cpufreq show/store
sysfs attribute callbacks are applied to the governor tunable
attributes and they always acquire the policy->rwsem lock before
carrying out the operation.  That may lead to an ABBA deadlock if
governor tunable attributes are removed under policy->rwsem while one
of them is being accessed concurrently (if sysfs attributes removal
wins the race, it will wait for the access to complete with
policy->rwsem held while the attribute callback will block on
policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of
the ->governor callback with the event arg equal to
CPUFREQ_GOV_POLICY_EXIT) in cpufreq_set_policy(), but that opened up
race conditions that had not been possible with policy->rwsem held all
the time.  Therefore policy->rwsem cannot be dropped in
cpufreq_set_policy() at any point, but the deadlock situation
described above must be avoided too.

To that end, use the observation that in principle governor tunables
may be represented by the same data type regardless of whether the
governor is system-wide or per-policy and introduce a new structure,
struct governor_attr, for representing them and new corresponding
macros for creating show/store sysfs callbacks for them.  Also make
their parent kobject use a new kobject type whose default show/store
callbacks are not related to the standard core cpufreq ones in any way
(and they don't acquire policy->rwsem in particular)."

IOW, (1) describe the problem you're addressing so that people
unfamiliar with the code in question can understand it, (2) describe
what is done to address the problem (what's the idea and what changes
are made to implement it).

[cut]

> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -22,14 +22,37 @@
>
>  #include "cpufreq_governor.h"
>
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +#define to_dbs_data(k) container_of(k, struct dbs_data, kobj)
> +#define to_attr(a) container_of(a, struct governor_attr, attr)

Please change the above to static inline routines.

> +
> +static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)

A better name please.  Something that will correspond to the purpose.

>  {
> -       if (have_governor_per_policy())
> -               return dbs_data->cdata->attr_group_gov_pol;
> -       else
> -               return dbs_data->cdata->attr_group_gov_sys;
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_attr(attr);
> +
> +       if (gattr->show)
> +               return gattr->show(dbs_data, buf);
> +
> +       return -EIO;
> +}
> +
> +static ssize_t store(struct kobject *kobj, struct attribute *attr,
> +                    const char *buf, size_t count)

Ditto.

> +{
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_attr(attr);
> +
> +       if (gattr->store)
> +               return gattr->store(dbs_data, buf, count);

Say two instances of this run in parallel with each other, either for
the same attribute or for different attributes under the same
dbs_data.  What's the guarantee that they won't make conflicting
changes?

> +
> +       return -EIO;
>  }
>
> +static const struct sysfs_ops sysfs_ops = {
> +       .show   = show,
> +       .store  = store,
> +};

That is completely enigmatic, so please at least add a comment describing it.

> +
>  void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  {
>         struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> @@ -354,6 +377,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>                                  struct dbs_data *dbs_data,
>                                  struct common_dbs_data *cdata)
>  {
> +       struct attribute_group *attr_group;
>         int ret;
>
>         /* State should be equivalent to EXIT */
> @@ -395,10 +419,17 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>
>         policy->governor_data = dbs_data;
>
> -       ret = sysfs_create_group(get_governor_parent_kobj(policy),
> -                                get_sysfs_attr(dbs_data));
> -       if (ret)
> +       attr_group = dbs_data->cdata->attr_group;
> +       dbs_data->kobj_type.sysfs_ops = &sysfs_ops;
> +       dbs_data->kobj_type.default_attrs = attr_group->attrs;

Why can't the kobject type be defined in struct common_dbs_data?
Surely, it will be the same for all dbs_data objects corresponding to
the same governor, won't it?

> +
> +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
> +                                  get_governor_parent_kobj(policy),
> +                                  attr_group->name);
> +       if (ret) {
> +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);

pr_debug() would be better here.

>                 goto reset_gdbs_data;
> +       }
>
>         return 0;
>
> @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>                 return -EBUSY;
>
>         if (!--dbs_data->usage_count) {
> -               sysfs_remove_group(get_governor_parent_kobj(policy),
> -                                  get_sysfs_attr(dbs_data));
> +               kobject_put(&dbs_data->kobj);

Don't we need a ->release callback for this kobject?

>
>                 policy->governor_data = NULL;
>
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ad44a8546a3a..59b28133dd68 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol                         \
>  show_one(_gov, file_name);                                             \
>  store_one(_gov, file_name)
>
> +/* Governor's specific attributes */
> +struct dbs_data;
> +struct governor_attr {
> +       struct attribute attr;
> +       ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
> +       ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
> +                        size_t count);
> +};
> +
> +#define gov_show_one(_gov, file_name)                                  \
> +static ssize_t show_##file_name                                                \
> +(struct dbs_data *dbs_data, char *buf)                                 \
> +{                                                                      \
> +       struct _gov##_dbs_tuners *tuners = dbs_data->tuners;            \
> +       return sprintf(buf, "%u\n", tuners->file_name);                 \
> +}
> +
> +#define gov_attr_ro(_name)                                             \
> +static struct governor_attr _name =                                    \
> +__ATTR(_name, 0444, show_##_name, NULL)
> +
> +#define gov_attr_rw(_name)                                             \
> +static struct governor_attr _name =                                    \
> +__ATTR(_name, 0644, show_##_name, store_##_name)
> +
>  /* create helper routines */
>  #define define_get_cpu_dbs_routines(_dbs_info)                         \
>  static struct cpu_dbs_info *get_cpu_cdbs(int cpu)                      \
> @@ -197,14 +222,12 @@ struct cs_dbs_tuners {
>  };
>
>  /* Common Governor data across policies */
> -struct dbs_data;
>  struct common_dbs_data {
>         /* Common across governors */
>         #define GOV_ONDEMAND            0
>         #define GOV_CONSERVATIVE        1
>         int governor;
> -       struct attribute_group *attr_group_gov_sys; /* one governor - system */
> -       struct attribute_group *attr_group_gov_pol; /* one governor - policy */
> +       struct attribute_group *attr_group; /* one governor - system */
>
>         /*
>          * Common data for platforms that don't set
> @@ -234,6 +257,8 @@ struct dbs_data {
>         struct common_dbs_data *cdata;
>         int usage_count;
>         void *tuners;
> +       struct kobject kobj;
> +       struct kobj_type kobj_type;

This is questionable.  The kobject type doesn't have to be dynamic IMO.

>  };

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 02-02-16, 22:23, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> "The ondemand and conservative governors use the global-attr or
> freq-attr structures to represent sysfs attributes corresponding to
> their tunables

> (which of them is actually used depends on whether or
> not different policy objects can use different governors at the same
> time

Not exactly. Different policies can always use different governors.
What made the difference was that different policies using same
governor, with different tunables or separate governor directories.

I have reworded this para like:

    The ondemand and conservative governors use the global-attr or freq-attr
    structures to represent sysfs attributes corresponding to their tunables
    (which of them is actually used depends on whether or not different
    policy objects can use same governor with different tunables at the same
    time and, consequently, on where those attributes are located in sysfs).

Please let me know if isn't clear.

> > --- a/drivers/cpufreq/cpufreq_governor.c
> > +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
> > +                                  get_governor_parent_kobj(policy),
> > +                                  attr_group->name);
> > +       if (ret) {
> > +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
> 
> pr_debug() would be better here.

Its a real error, why pr_debug for that ?

> >                 goto reset_gdbs_data;
> > +       }
> >
> >         return 0;
> >
> > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
> >                 return -EBUSY;
> >
> >         if (!--dbs_data->usage_count) {
> > -               sysfs_remove_group(get_governor_parent_kobj(policy),
> > -                                  get_sysfs_attr(dbs_data));
> > +               kobject_put(&dbs_data->kobj);
> 
> Don't we need a ->release callback for this kobject?

There is nothing that we need to free from the ->release() callback.
We are using the kobject here just to get separate show/store
callbacks.

Here is the new version based on the review comments received until
now:

-------------------------8<-------------------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Feb 2016 12:35:01 +0530
Subject: [PATCH] cpufreq: governor: New sysfs show/store callbacks for
 governor tunables

The ondemand and conservative governors use the global-attr or freq-attr
structures to represent sysfs attributes corresponding to their tunables
(which of them is actually used depends on whether or not different
policy objects can use same governor with different tunables at the same
time and, consequently, on where those attributes are located in sysfs).

Unfortunately, in the freq-attr case, the standard cpufreq show/store
sysfs attribute callbacks are applied to the governor tunable attributes
and they always acquire the policy->rwsem lock before carrying out the
operation.  That may lead to an ABBA deadlock if governor tunable
attributes are removed under policy->rwsem while one of them is being
accessed concurrently (if sysfs attributes removal wins the race, it
will wait for the access to complete with policy->rwsem held while the
attribute callback will block on policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of the
->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
in cpufreq_set_policy(), but that opened up race conditions that had not
been possible with policy->rwsem held all the time.  Therefore
policy->rwsem cannot be dropped in cpufreq_set_policy() at any point,
but the deadlock situation described above must be avoided too.

To that end, use the observation that in principle governor tunables may
be represented by the same data type regardless of whether the governor
is system-wide or per-policy and introduce a new structure, struct
governor_attr, for representing them and new corresponding macros for
creating show/store sysfs callbacks for them.  Also make their parent
kobject use a new kobject type whose default show/store callbacks are
not related to the standard core cpufreq ones in any way (and they don't
acquire policy->rwsem in particular).

[ Rafael: Written changelog ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 73 ++++++++++++----------------------
 drivers/cpufreq/cpufreq_governor.c     | 71 ++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     | 34 ++++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 73 ++++++++++++----------------------
 4 files changed, 144 insertions(+), 107 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 57750367bd26..c749fb4fe5d2 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -275,54 +275,33 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(cs, sampling_rate);
-show_store_one(cs, sampling_down_factor);
-show_store_one(cs, up_threshold);
-show_store_one(cs, down_threshold);
-show_store_one(cs, ignore_nice_load);
-show_store_one(cs, freq_step);
-show_one(cs, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(down_threshold);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(freq_step);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&down_threshold_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&freq_step_gov_sys.attr,
+gov_show_one(cs, sampling_rate);
+gov_show_one(cs, sampling_down_factor);
+gov_show_one(cs, up_threshold);
+gov_show_one(cs, down_threshold);
+gov_show_one(cs, ignore_nice_load);
+gov_show_one(cs, freq_step);
+gov_show_one(cs, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(up_threshold);
+gov_attr_rw(down_threshold);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(freq_step);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *cs_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&sampling_down_factor.attr,
+	&up_threshold.attr,
+	&down_threshold.attr,
+	&ignore_nice_load.attr,
+	&freq_step.attr,
 	NULL
 };
 
-static struct attribute_group cs_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "conservative",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&down_threshold_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&freq_step_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group cs_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
-	.name = "conservative",
-};
-
 /************************** sysfs end ************************/
 
 static int cs_init(struct dbs_data *dbs_data, bool notify)
@@ -365,8 +344,8 @@ define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
 static struct common_dbs_data cs_dbs_cdata = {
 	.governor = GOV_CONSERVATIVE,
-	.attr_group_gov_sys = &cs_attr_group_gov_sys,
-	.attr_group_gov_pol = &cs_attr_group_gov_pol,
+	.kobj_name = "conservative",
+	.kobj_type = { .sysfs_ops = &governor_sysfs_ops, .default_attrs = cs_attributes },
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = cs_dbs_timer,
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 9a7edc91ad57..a9f335c4e461 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,14 +22,62 @@
 
 #include "cpufreq_governor.h"
 
-static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
+static inline struct dbs_data *to_dbs_data(struct kobject *kobj)
 {
-	if (have_governor_per_policy())
-		return dbs_data->cdata->attr_group_gov_pol;
-	else
-		return dbs_data->cdata->attr_group_gov_sys;
+	return container_of(kobj, struct dbs_data, kobj);
+}
+
+static inline struct governor_attr *to_gov_attr(struct attribute *attr)
+{
+	return container_of(attr, struct governor_attr, attr);
+}
+
+static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
+			     char *buf)
+{
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_gov_attr(attr);
+	int ret = -EIO;
+
+	down_read(&dbs_data->rwsem);
+
+	if (gattr->show)
+		ret = gattr->show(dbs_data, buf);
+
+	up_read(&dbs_data->rwsem);
+
+	return ret;
 }
 
+static ssize_t governor_store(struct kobject *kobj, struct attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct dbs_data *dbs_data = to_dbs_data(kobj);
+	struct governor_attr *gattr = to_gov_attr(attr);
+	int ret = -EIO;
+
+	down_write(&dbs_data->rwsem);
+
+	if (gattr->store)
+		ret = gattr->store(dbs_data, buf, count);
+
+	up_write(&dbs_data->rwsem);
+
+	return ret;
+}
+
+/*
+ * Sysfs Ops for accessing governor attributes.
+ *
+ * All show/store invocations for governor specific sysfs attributes, will first
+ * call the below show/store callbacks and the attribute specific callback will
+ * be called from within it.
+ */
+const struct sysfs_ops governor_sysfs_ops = {
+	.show	= governor_show,
+	.store	= governor_store,
+};
+
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 {
 	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
@@ -383,6 +431,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 
 	dbs_data->cdata = cdata;
 	dbs_data->usage_count = 1;
+	init_rwsem(&dbs_data->rwsem);
 
 	ret = cdata->init(dbs_data, !policy->governor->initialized);
 	if (ret)
@@ -395,10 +444,13 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 
 	policy->governor_data = dbs_data;
 
-	ret = sysfs_create_group(get_governor_parent_kobj(policy),
-				 get_sysfs_attr(dbs_data));
-	if (ret)
+	ret = kobject_init_and_add(&dbs_data->kobj, &cdata->kobj_type,
+				   get_governor_parent_kobj(policy),
+				   cdata->kobj_name);
+	if (ret) {
+		pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
 		goto reset_gdbs_data;
+	}
 
 	return 0;
 
@@ -426,8 +478,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
 		return -EBUSY;
 
 	if (!--dbs_data->usage_count) {
-		sysfs_remove_group(get_governor_parent_kobj(policy),
-				   get_sysfs_attr(dbs_data));
+		kobject_put(&dbs_data->kobj);
 
 		policy->governor_data = NULL;
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index ad44a8546a3a..67500a1015cf 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol				\
 show_one(_gov, file_name);						\
 store_one(_gov, file_name)
 
+/* Governor's specific attributes */
+struct dbs_data;
+struct governor_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct dbs_data *dbs_data, char *buf);
+	ssize_t (*store)(struct dbs_data *dbs_data, const char *buf,
+			 size_t count);
+};
+
+#define gov_show_one(_gov, file_name)					\
+static ssize_t show_##file_name						\
+(struct dbs_data *dbs_data, char *buf)					\
+{									\
+	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
+	return sprintf(buf, "%u\n", tuners->file_name);			\
+}
+
+#define gov_attr_ro(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0444, show_##_name, NULL)
+
+#define gov_attr_rw(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 /* create helper routines */
 #define define_get_cpu_dbs_routines(_dbs_info)				\
 static struct cpu_dbs_info *get_cpu_cdbs(int cpu)			\
@@ -197,14 +222,13 @@ struct cs_dbs_tuners {
 };
 
 /* Common Governor data across policies */
-struct dbs_data;
 struct common_dbs_data {
 	/* Common across governors */
 	#define GOV_ONDEMAND		0
 	#define GOV_CONSERVATIVE	1
 	int governor;
-	struct attribute_group *attr_group_gov_sys; /* one governor - system */
-	struct attribute_group *attr_group_gov_pol; /* one governor - policy */
+	const char *kobj_name;
+	struct kobj_type kobj_type;
 
 	/*
 	 * Common data for platforms that don't set
@@ -234,6 +258,9 @@ struct dbs_data {
 	struct common_dbs_data *cdata;
 	int usage_count;
 	void *tuners;
+	struct kobject kobj;
+	/* Protect concurrent updates to governor tunables from sysfs */
+	struct rw_semaphore rwsem;
 };
 
 /* Governor specific ops, will be passed to dbs_data->gov_ops */
@@ -256,6 +283,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 }
 
 extern struct mutex cpufreq_governor_lock;
+extern const struct sysfs_ops governor_sysfs_ops;
 
 void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
 void gov_cancel_work(struct cpu_common_dbs_info *shared);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index b31f64745232..82ed490f7de0 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -436,54 +436,33 @@ static ssize_t store_powersave_bias(struct dbs_data *dbs_data, const char *buf,
 	return count;
 }
 
-show_store_one(od, sampling_rate);
-show_store_one(od, io_is_busy);
-show_store_one(od, up_threshold);
-show_store_one(od, sampling_down_factor);
-show_store_one(od, ignore_nice_load);
-show_store_one(od, powersave_bias);
-show_one(od, min_sampling_rate);
-
-gov_sys_pol_attr_rw(sampling_rate);
-gov_sys_pol_attr_rw(io_is_busy);
-gov_sys_pol_attr_rw(up_threshold);
-gov_sys_pol_attr_rw(sampling_down_factor);
-gov_sys_pol_attr_rw(ignore_nice_load);
-gov_sys_pol_attr_rw(powersave_bias);
-gov_sys_pol_attr_ro(min_sampling_rate);
-
-static struct attribute *dbs_attributes_gov_sys[] = {
-	&min_sampling_rate_gov_sys.attr,
-	&sampling_rate_gov_sys.attr,
-	&up_threshold_gov_sys.attr,
-	&sampling_down_factor_gov_sys.attr,
-	&ignore_nice_load_gov_sys.attr,
-	&powersave_bias_gov_sys.attr,
-	&io_is_busy_gov_sys.attr,
+gov_show_one(od, sampling_rate);
+gov_show_one(od, io_is_busy);
+gov_show_one(od, up_threshold);
+gov_show_one(od, sampling_down_factor);
+gov_show_one(od, ignore_nice_load);
+gov_show_one(od, powersave_bias);
+gov_show_one(od, min_sampling_rate);
+
+gov_attr_rw(sampling_rate);
+gov_attr_rw(io_is_busy);
+gov_attr_rw(up_threshold);
+gov_attr_rw(sampling_down_factor);
+gov_attr_rw(ignore_nice_load);
+gov_attr_rw(powersave_bias);
+gov_attr_ro(min_sampling_rate);
+
+static struct attribute *od_attributes[] = {
+	&min_sampling_rate.attr,
+	&sampling_rate.attr,
+	&up_threshold.attr,
+	&sampling_down_factor.attr,
+	&ignore_nice_load.attr,
+	&powersave_bias.attr,
+	&io_is_busy.attr,
 	NULL
 };
 
-static struct attribute_group od_attr_group_gov_sys = {
-	.attrs = dbs_attributes_gov_sys,
-	.name = "ondemand",
-};
-
-static struct attribute *dbs_attributes_gov_pol[] = {
-	&min_sampling_rate_gov_pol.attr,
-	&sampling_rate_gov_pol.attr,
-	&up_threshold_gov_pol.attr,
-	&sampling_down_factor_gov_pol.attr,
-	&ignore_nice_load_gov_pol.attr,
-	&powersave_bias_gov_pol.attr,
-	&io_is_busy_gov_pol.attr,
-	NULL
-};
-
-static struct attribute_group od_attr_group_gov_pol = {
-	.attrs = dbs_attributes_gov_pol,
-	.name = "ondemand",
-};
-
 /************************** sysfs end ************************/
 
 static int od_init(struct dbs_data *dbs_data, bool notify)
@@ -542,8 +521,8 @@ static struct od_ops od_ops = {
 
 static struct common_dbs_data od_dbs_cdata = {
 	.governor = GOV_ONDEMAND,
-	.attr_group_gov_sys = &od_attr_group_gov_sys,
-	.attr_group_gov_pol = &od_attr_group_gov_pol,
+	.kobj_name = "ondemand",
+	.kobj_type = { .sysfs_ops = &governor_sysfs_ops, .default_attrs = od_attributes },
 	.get_cpu_cdbs = get_cpu_cdbs,
 	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
 	.gov_dbs_timer = od_dbs_timer,

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 7:58 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-16, 22:23, Rafael J. Wysocki wrote:
>> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> "The ondemand and conservative governors use the global-attr or
>> freq-attr structures to represent sysfs attributes corresponding to
>> their tunables
>
>> (which of them is actually used depends on whether or
>> not different policy objects can use different governors at the same
>> time
>
> Not exactly. Different policies can always use different governors.
> What made the difference was that different policies using same
> governor, with different tunables or separate governor directories.
>
> I have reworded this para like:
>
>     The ondemand and conservative governors use the global-attr or freq-attr
>     structures to represent sysfs attributes corresponding to their tunables
>     (which of them is actually used depends on whether or not different
>     policy objects can use same governor with different tunables at the same
>     time and, consequently, on where those attributes are located in sysfs).
>
> Please let me know if isn't clear.

That's OK.  IMO you should say "use the same governor", but that's
easily fixable. :-)

>> > --- a/drivers/cpufreq/cpufreq_governor.c
>> > +       ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
>> > +                                  get_governor_parent_kobj(policy),
>> > +                                  attr_group->name);
>> > +       if (ret) {
>> > +               pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
>>
>> pr_debug() would be better here.
>
> Its a real error, why pr_debug for that ?

What's the value of printing that on user systems?  It contains debug
information only and it is not useful to anyone unfamiliar with the
code in question anyway.

>> >                 goto reset_gdbs_data;
>> > +       }
>> >
>> >         return 0;
>> >
>> > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>> >                 return -EBUSY;
>> >
>> >         if (!--dbs_data->usage_count) {
>> > -               sysfs_remove_group(get_governor_parent_kobj(policy),
>> > -                                  get_sysfs_attr(dbs_data));
>> > +               kobject_put(&dbs_data->kobj);
>>
>> Don't we need a ->release callback for this kobject?
>
> There is nothing that we need to free from the ->release() callback.
> We are using the kobject here just to get separate show/store
> callbacks.

Well, I guess the answer should be that there can't be more active
references to the kobject, so it is safe to free it synchronously
later.

> Here is the new version based on the review comments received until
> now:
>
> -------------------------8<-------------------------
>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 2 Feb 2016 12:35:01 +0530
> Subject: [PATCH] cpufreq: governor: New sysfs show/store callbacks for
>  governor tunables
>

[cut]

> @@ -22,14 +22,62 @@
>
>  #include "cpufreq_governor.h"
>
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +static inline struct dbs_data *to_dbs_data(struct kobject *kobj)
>  {
> -       if (have_governor_per_policy())
> -               return dbs_data->cdata->attr_group_gov_pol;
> -       else
> -               return dbs_data->cdata->attr_group_gov_sys;
> +       return container_of(kobj, struct dbs_data, kobj);
> +}
> +
> +static inline struct governor_attr *to_gov_attr(struct attribute *attr)
> +{
> +       return container_of(attr, struct governor_attr, attr);
> +}
> +
> +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
> +                            char *buf)
> +{
> +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> +       struct governor_attr *gattr = to_gov_attr(attr);
> +       int ret = -EIO;
> +
> +       down_read(&dbs_data->rwsem);
> +
> +       if (gattr->show)
> +               ret = gattr->show(dbs_data, buf);
> +
> +       up_read(&dbs_data->rwsem);

Do we need the lock here too?

show() is only going to read the value, isn't it?  And everything u32
or smaller is read atomically anyway.

Apart from this it looks good to me.

When you're ready, please resend the whole series without patch [5/5]
which is premature IMO.

Thanks,
Rafael

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Viresh Kumar]

On 03-02-16, 13:42, Rafael J. Wysocki wrote:
> > +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
> > +                            char *buf)
> > +{
> > +       struct dbs_data *dbs_data = to_dbs_data(kobj);
> > +       struct governor_attr *gattr = to_gov_attr(attr);
> > +       int ret = -EIO;
> > +
> > +       down_read(&dbs_data->rwsem);
> > +
> > +       if (gattr->show)
> > +               ret = gattr->show(dbs_data, buf);
> > +
> > +       up_read(&dbs_data->rwsem);
> 
> Do we need the lock here too?
> 
> show() is only going to read the value, isn't it?  And everything u32
> or smaller is read atomically anyway.

Okay, will drop it for now.

> Apart from this it looks good to me.
> 
> When you're ready, please resend the whole series without patch [5/5]
> which is premature IMO.

I have changed that patch a bit, and am dropping just the lock now and
not governor_enable thing. That should be sane enough I believe.

Anyway I will be posting 7 patches now, pick only first 4 if you
aren't confident about the rest.

-- 
viresh

Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 2:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 03-02-16, 13:42, Rafael J. Wysocki wrote:
>> > +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
>> > +                            char *buf)
>> > +{
>> > +       struct dbs_data *dbs_data = to_dbs_data(kobj);
>> > +       struct governor_attr *gattr = to_gov_attr(attr);
>> > +       int ret = -EIO;
>> > +
>> > +       down_read(&dbs_data->rwsem);
>> > +
>> > +       if (gattr->show)
>> > +               ret = gattr->show(dbs_data, buf);
>> > +
>> > +       up_read(&dbs_data->rwsem);
>>
>> Do we need the lock here too?
>>
>> show() is only going to read the value, isn't it?  And everything u32
>> or smaller is read atomically anyway.
>
> Okay, will drop it for now.
>
>> Apart from this it looks good to me.
>>
>> When you're ready, please resend the whole series without patch [5/5]
>> which is premature IMO.
>
> I have changed that patch a bit, and am dropping just the lock now and
> not governor_enable thing. That should be sane enough I believe.

In any case this is not suitable for 4.5 IMO.

> Anyway I will be posting 7 patches now, pick only first 4 if you
> aren't confident about the rest.

OK

Thanks,
Rafael

[PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros

[Viresh Kumar]

The older macros aren't used anymore, remove them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.h | 68 --------------------------------------
 1 file changed, 68 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 59b28133dd68..5a7de52815c4 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -40,74 +40,6 @@
 /* Ondemand Sampling types */
 enum {OD_NORMAL_SAMPLE, OD_SUB_SAMPLE};
 
-/*
- * Macro for creating governors sysfs routines
- *
- * - gov_sys: One governor instance per whole system
- * - gov_pol: One governor instance per policy
- */
-
-/* Create attributes */
-#define gov_sys_attr_ro(_name)						\
-static struct global_attr _name##_gov_sys =				\
-__ATTR(_name, 0444, show_##_name##_gov_sys, NULL)
-
-#define gov_sys_attr_rw(_name)						\
-static struct global_attr _name##_gov_sys =				\
-__ATTR(_name, 0644, show_##_name##_gov_sys, store_##_name##_gov_sys)
-
-#define gov_pol_attr_ro(_name)						\
-static struct freq_attr _name##_gov_pol =				\
-__ATTR(_name, 0444, show_##_name##_gov_pol, NULL)
-
-#define gov_pol_attr_rw(_name)						\
-static struct freq_attr _name##_gov_pol =				\
-__ATTR(_name, 0644, show_##_name##_gov_pol, store_##_name##_gov_pol)
-
-#define gov_sys_pol_attr_rw(_name)					\
-	gov_sys_attr_rw(_name);						\
-	gov_pol_attr_rw(_name)
-
-#define gov_sys_pol_attr_ro(_name)					\
-	gov_sys_attr_ro(_name);						\
-	gov_pol_attr_ro(_name)
-
-/* Create show/store routines */
-#define show_one(_gov, file_name)					\
-static ssize_t show_##file_name##_gov_sys				\
-(struct kobject *kobj, struct attribute *attr, char *buf)		\
-{									\
-	struct _gov##_dbs_tuners *tuners = _gov##_dbs_cdata.gdbs_data->tuners; \
-	return sprintf(buf, "%u\n", tuners->file_name);			\
-}									\
-									\
-static ssize_t show_##file_name##_gov_pol				\
-(struct cpufreq_policy *policy, char *buf)				\
-{									\
-	struct dbs_data *dbs_data = policy->governor_data;		\
-	struct _gov##_dbs_tuners *tuners = dbs_data->tuners;		\
-	return sprintf(buf, "%u\n", tuners->file_name);			\
-}
-
-#define store_one(_gov, file_name)					\
-static ssize_t store_##file_name##_gov_sys				\
-(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) \
-{									\
-	struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data;		\
-	return store_##file_name(dbs_data, buf, count);			\
-}									\
-									\
-static ssize_t store_##file_name##_gov_pol				\
-(struct cpufreq_policy *policy, const char *buf, size_t count)		\
-{									\
-	struct dbs_data *dbs_data = policy->governor_data;		\
-	return store_##file_name(dbs_data, buf, count);			\
-}
-
-#define show_store_one(_gov, file_name)					\
-show_one(_gov, file_name);						\
-store_one(_gov, file_name)
-
 /* Governor's specific attributes */
 struct dbs_data;
 struct governor_attr {
-- 
2.7.0.79.gdc08a19

Re: [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The older macros aren't used anymore, remove them.

It doesn't follow from either the subject or the changelog which
macros *in* *particular* are in question.

What about changing the subject to something like "cpufreq: governor:
Drop unused macros for creating governor tunable attributes" and the
changelog to something like:

"The previous commit introduced a new set of macros for creating sysfs
attributes that represent governor tunables and the old macros used
for this purpose are not needed any more, so drop them."

Thanks,
Rafael

[PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
issues.

The previous commit has fixed them all and we don't need to drop these
locks anymore.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 5 -----
 include/linux/cpufreq.h   | 4 ----
 2 files changed, 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e979ec78b695..5f7e24567e0e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2155,10 +2155,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 			return ret;
 		}
 
-		up_write(&policy->rwsem);
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
-
 		if (ret) {
 			pr_err("%s: Failed to Exit Governor: %s (%d)\n",
 			       __func__, old_gov->name, ret);
@@ -2174,9 +2171,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 		if (!ret)
 			goto out;
 
-		up_write(&policy->rwsem);
 		__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
 	}
 
 	/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 88a4215125bc..79b87cebaa9c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
 	 * - Any routine that will write to the policy structure and/or may take away
 	 *   the policy altogether (eg. CPU hotplug), will hold this lock in write
 	 *   mode before doing so.
-	 *
-	 * Additional rules:
-	 * - Lock should not be held across
-	 *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 	 */
 	struct rw_semaphore	rwsem;
 
-- 
2.7.0.79.gdc08a19

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
> CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
> issues.
>
> The previous commit has fixed them all and we don't need to drop these
> locks anymore.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

First of all, this is effectively reverting commit 955ef4833574, so
the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
lock around CPUFREQ_GOV_POLICY_EXIT)".

There should be a Fixes: tag pointing to commit 955ef4833574 and a
Reported-by: for Juri.

If there is a link to a bug report related to this, it should be
pointed to by a Link: tag.

The changelog should say why the original commit was there and why the
way it attempted to solve the problem was incorrect.  It also should
say that the original problem was addressed by a previous commit, so
this one can be reverted without consequences.

But I'm not going to write that changelog.  I actually am not going to
write any changelogs for you any more, because I'm seriously tired of
doing that.  Moreover, if I see a patch from you with a changelog
that's not acceptable to me, it will immediately go to the "not
applicable" trash bin no matter what the changes below look like.  You
*have* *to* write useful changelogs.  This isn't optional or best
effort.  This is mandatory and important.

Now, I'm not really sure if the ordering of this patchset is right.
Maybe we should just revert upfront with the "we'll address the
original problem in the following commits" statement in the changelog
and fix it in a different way?  It looks like patches [1-3/5] fix a
problem that isn't there even, but would appear after the [4/5] if
they were not applied previously, which doesn't sound really
straightforward to me.

Thanks,
Rafael

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 02-02-16, 22:53, Rafael J. Wysocki wrote:
> First of all, this is effectively reverting commit 955ef4833574, so
> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT)".
> 
> There should be a Fixes: tag pointing to commit 955ef4833574 and a
> Reported-by: for Juri.
> 
> If there is a link to a bug report related to this, it should be
> pointed to by a Link: tag.
> 
> The changelog should say why the original commit was there and why the
> way it attempted to solve the problem was incorrect.  It also should
> say that the original problem was addressed by a previous commit, so
> this one can be reverted without consequences.

How about this:

    Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
    
    Earlier, when the struct freq-attr was used to represent governor
    attributes, the standard cpufreq show/store sysfs attribute callbacks
    were applied to the governor tunable attributes and they always acquire
    the policy->rwsem lock before carrying out the operation.  That could
    have resulted in an ABBA deadlock if governor tunable attributes are
    removed under policy->rwsem while one of them is being accessed
    concurrently (if sysfs attributes removal wins the race, it will wait
    for the access to complete with policy->rwsem held while the attribute
    callback will block on policy->rwsem indefinitely).
    
    We attempted to address this issue by dropping policy->rwsem around
    governor tunable attributes removal (that is, around invocations of the
    ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
    in cpufreq_set_policy(), but that opened up race conditions that had not
    been possible with policy->rwsem held all the time.
    
    The previous commit, "cpufreq: governor: New sysfs show/store callbacks
    for governor tunables", fixed the original ABBA deadlock by adding new
    governor specific show/store callbacks.
    
    We don't have to drop rwsem around invocations of governor event
    CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
    
    Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
    Reported-by: Juri Lelli <juri.lelli@arm.com>
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> But I'm not going to write that changelog.  I actually am not going to
> write any changelogs for you any more, because I'm seriously tired of
> doing that.  Moreover, if I see a patch from you with a changelog
> that's not acceptable to me, it will immediately go to the "not
> applicable" trash bin no matter what the changes below look like.  You
> *have* *to* write useful changelogs.  This isn't optional or best
> effort.  This is mandatory and important.

Will try to improve, sorry about that (again).

> Now, I'm not really sure if the ordering of this patchset is right.
> Maybe we should just revert upfront with the "we'll address the
> original problem in the following commits" statement in the changelog
> and fix it in a different way?

Wouldn't that break things like 'git bisect'? People running kernels
after the reverted commits may start hitting lockdeps.

> It looks like patches [1-3/5] fix a
> problem that isn't there even, but would appear after the [4/5] if
> they were not applied previously, which doesn't sound really
> straightforward to me.

I am going to fight hard for it, if you feel 4/5 should be the first
patch here, I will do that.

-- 
viresh

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 6:51 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-16, 22:53, Rafael J. Wysocki wrote:
>> First of all, this is effectively reverting commit 955ef4833574, so
>> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT)".
>>
>> There should be a Fixes: tag pointing to commit 955ef4833574 and a
>> Reported-by: for Juri.
>>
>> If there is a link to a bug report related to this, it should be
>> pointed to by a Link: tag.
>>
>> The changelog should say why the original commit was there and why the
>> way it attempted to solve the problem was incorrect.  It also should
>> say that the original problem was addressed by a previous commit, so
>> this one can be reverted without consequences.
>
> How about this:
>
>     Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
>
>     Earlier, when the struct freq-attr was used to represent governor
>     attributes, the standard cpufreq show/store sysfs attribute callbacks
>     were applied to the governor tunable attributes and they always acquire
>     the policy->rwsem lock before carrying out the operation.  That could
>     have resulted in an ABBA deadlock if governor tunable attributes are
>     removed under policy->rwsem while one of them is being accessed
>     concurrently (if sysfs attributes removal wins the race, it will wait
>     for the access to complete with policy->rwsem held while the attribute
>     callback will block on policy->rwsem indefinitely).
>
>     We attempted to address this issue by dropping policy->rwsem around
>     governor tunable attributes removal (that is, around invocations of the
>     ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
>     in cpufreq_set_policy(), but that opened up race conditions that had not
>     been possible with policy->rwsem held all the time.
>
>     The previous commit, "cpufreq: governor: New sysfs show/store callbacks
>     for governor tunables", fixed the original ABBA deadlock by adding new
>     governor specific show/store callbacks.
>
>     We don't have to drop rwsem around invocations of governor event
>     CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
>
>     Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
>     Reported-by: Juri Lelli <juri.lelli@arm.com>
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Much better.

>> But I'm not going to write that changelog.  I actually am not going to
>> write any changelogs for you any more, because I'm seriously tired of
>> doing that.  Moreover, if I see a patch from you with a changelog
>> that's not acceptable to me, it will immediately go to the "not
>> applicable" trash bin no matter what the changes below look like.  You
>> *have* *to* write useful changelogs.  This isn't optional or best
>> effort.  This is mandatory and important.
>
> Will try to improve, sorry about that (again).
>
>> Now, I'm not really sure if the ordering of this patchset is right.
>> Maybe we should just revert upfront with the "we'll address the
>> original problem in the following commits" statement in the changelog
>> and fix it in a different way?
>
> Wouldn't that break things like 'git bisect'? People running kernels
> after the reverted commits may start hitting lockdeps.

Well, we have at least one bug in there before applying the whole
series anyway, regardless of the ordering.

>> It looks like patches [1-3/5] fix a
>> problem that isn't there even, but would appear after the [4/5] if
>> they were not applied previously, which doesn't sound really
>> straightforward to me.
>
> I am going to fight hard for it, if you feel 4/5 should be the first
> patch here, I will do that.

I guess this was supposed to be "I am not ...".

With the above changelog the current order of patches in the series is fine.

Thanks,
Rafael

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 03-02-16, 13:24, Rafael J. Wysocki wrote:
> I guess this was supposed to be "I am not ...".

Urg..

-- 
viresh

[PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Viresh Kumar]

Invalid state-transitions is verified by governor core now and there is
no need to replicate that in cpufreq core. Also we don't drop
policy->rwsem anymore, which makes rest of the races go away.

Simplify code a bit now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 24 ------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 25 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5f7e24567e0e..052ad1b9372c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list);
 static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
-DEFINE_MUTEX(cpufreq_governor_lock);
 
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
@@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 
 	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
 
-	mutex_lock(&cpufreq_governor_lock);
-	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
-	    || (!policy->governor_enabled
-	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
-		mutex_unlock(&cpufreq_governor_lock);
-		return -EBUSY;
-	}
-
-	if (event == CPUFREQ_GOV_STOP)
-		policy->governor_enabled = false;
-	else if (event == CPUFREQ_GOV_START)
-		policy->governor_enabled = true;
-
-	mutex_unlock(&cpufreq_governor_lock);
-
 	ret = policy->governor->governor(policy, event);
 
 	if (!ret) {
@@ -1985,14 +1969,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 			policy->governor->initialized++;
 		else if (event == CPUFREQ_GOV_POLICY_EXIT)
 			policy->governor->initialized--;
-	} else {
-		/* Restore original values */
-		mutex_lock(&cpufreq_governor_lock);
-		if (event == CPUFREQ_GOV_STOP)
-			policy->governor_enabled = true;
-		else if (event == CPUFREQ_GOV_START)
-			policy->governor_enabled = false;
-		mutex_unlock(&cpufreq_governor_lock);
 	}
 
 	if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 79b87cebaa9c..e90cf5d31e85 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -80,7 +80,6 @@ struct cpufreq_policy {
 	unsigned int		last_policy; /* policy before unplug */
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
-	bool			governor_enabled; /* governor start/stop flag */
 	char			last_governor[CPUFREQ_NAME_LEN]; /* last governor used */
 
 	struct work_struct	update; /* if update_policy() needs to be
-- 
2.7.0.79.gdc08a19

Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Juri Lelli]

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Invalid state-transitions is verified by governor core now and there is
> no need to replicate that in cpufreq core. Also we don't drop
> policy->rwsem anymore, which makes rest of the races go away.

There are still paths where we call __cpufreq_governor() without holding
policy->rwsem, but those should be fixed with my cleanups (that I intend
to refresh and post soon). So, I'm not sure we can safely remove this
yet.

> 
> Simplify code a bit now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 24 ------------------------
>  include/linux/cpufreq.h   |  1 -
>  2 files changed, 25 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5f7e24567e0e..052ad1b9372c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list);
>  static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
> -DEFINE_MUTEX(cpufreq_governor_lock);
>  
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
> @@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  
>  	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
>  
> -	mutex_lock(&cpufreq_governor_lock);
> -	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> -	    || (!policy->governor_enabled
> -	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> -		mutex_unlock(&cpufreq_governor_lock);
> -		return -EBUSY;
> -	}
> -
> -	if (event == CPUFREQ_GOV_STOP)
> -		policy->governor_enabled = false;
> -	else if (event == CPUFREQ_GOV_START)
> -		policy->governor_enabled = true;
> -
> -	mutex_unlock(&cpufreq_governor_lock);
> -
>  	ret = policy->governor->governor(policy, event);

So, __cpufreq_governor() becomes effectively a wrapper around
->governor() calls and governors are left responsible for implementing
the state machine with appropriate checks.

I'm wondering if this approach is completely sane, but what we end up
with your changes should work (and we kill a lock! :)).

Maybe we add a comment somewhere stating exactly how things are meant to
work?

Thanks,

- Juri

Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Viresh Kumar]

On 02-02-16, 16:49, Juri Lelli wrote:
> There are still paths where we call __cpufreq_governor() without holding
> policy->rwsem, but those should be fixed with my cleanups (that I intend
> to refresh and post soon). So, I'm not sure we can safely remove this
> yet.

No, we can't.. Though I haven't seen any races from happening even
after removing it, but it doesn't mean we can't.

The deal is that, the entire sequence of events needs to be guaranteed
to happen in a particular order without any other code performing
similar operations concurrently.

And so we need to preserve the other sites with proper rwsem locking
first.

> So, __cpufreq_governor() becomes effectively a wrapper around
> ->governor() calls and governors are left responsible for implementing
> the state machine with appropriate checks.

Not really. The core can now guarantee that the entire sequence
happens atomically. For example, on governor switch, we need to
guarantee that STOP/EXIT happen without any intervention for the old
governor. Or, INIT/START/LIMITS happen without any intervention for
the new governor, etc..

> Maybe we add a comment somewhere stating exactly how things are meant to
> work?

Hmm.

-- 
viresh

Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Juri Lelli]

On 03/02/16 11:35, Viresh Kumar wrote:
> On 02-02-16, 16:49, Juri Lelli wrote:
> > There are still paths where we call __cpufreq_governor() without holding
> > policy->rwsem, but those should be fixed with my cleanups (that I intend
> > to refresh and post soon). So, I'm not sure we can safely remove this
> > yet.
> 
> No, we can't.. Though I haven't seen any races from happening even
> after removing it, but it doesn't mean we can't.
> 
> The deal is that, the entire sequence of events needs to be guaranteed
> to happen in a particular order without any other code performing
> similar operations concurrently.
> 
> And so we need to preserve the other sites with proper rwsem locking
> first.
> 

Right. I guess it is what I was trying to do with my cleanups, adding
assertions and fixing paths that didn't verify those.

It should be easy to rebase that set (or a part of it) on top of your
and/or Rafael changes. I realize that there are multiple sets of changes
under discussion; so, please tell me how do you, and Rafael, want to
proceed about this.

> > So, __cpufreq_governor() becomes effectively a wrapper around
> > ->governor() calls and governors are left responsible for implementing
> > the state machine with appropriate checks.
> 
> Not really. The core can now guarantee that the entire sequence
> happens atomically. For example, on governor switch, we need to
> guarantee that STOP/EXIT happen without any intervention for the old
> governor. Or, INIT/START/LIMITS happen without any intervention for
> the new governor, etc..
> 

OK, checking for invalid state transitions (for ondemand and
conservative) is still in done cpufreq_governor.c.

> > Maybe we add a comment somewhere stating exactly how things are meant to
> > work?

But, I guess any other governor that will bypass cpufreq_governor.c, it
will also have to implement such checks. I was just proposing to state
this somewhere, so that we don't forget.

Best,

- Juri

Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Viresh Kumar]

On 03-02-16, 11:05, Juri Lelli wrote:
> It should be easy to rebase that set (or a part of it) on top of your
> and/or Rafael changes. I realize that there are multiple sets of changes
> under discussion; so, please tell me how do you, and Rafael, want to
> proceed about this.

Yeah, please wait for a bit for Rafael to apply both the series (if
they pass the litmus test) to PM tree :)

> But, I guess any other governor that will bypass cpufreq_governor.c, it
> will also have to implement such checks. I was just proposing to state
> this somewhere, so that we don't forget.

We can surely add a comment for that.

But I would like to review the state after these patches are applied,
as we may be able to guarantee that from cpufreq-core instead.

-- 
viresh

Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Invalid state-transitions is verified by governor core now

What about the governors that don't use cpufreq_governor_dbs()?

> and there is no need to replicate that in cpufreq core. Also we don't drop
> policy->rwsem anymore, which makes rest of the races go away.
>
> Simplify code a bit now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I won't make this change just yet.  At least not in 4.5 (provided that
the other patches go into it).

Thanks,
Rafael

Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[Juri Lelli]

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Hi Rafael,
> 
> Sorry for doing this, I know you were also looking to fix this in a
> possibly different way. But I thought, it would be better if we fix
> that. We can scrap this version and take yours if that looks better.
> 
> The root cause of all the issues we were facing, was that we were taking
> policy->rwsem while accessing governor sysfs attributes. And that
> happened because we were sharing the show/store calls present in
> cpufreq.c.
> 
> I thought, perhaps the best way to fix it is to give separate sysfs-ops
> to governors. And that's what I did.
> 
> @Juri: I need your help in testing these. My platform doesn't give me
> those lockups (even without these patches) and Juno/Tc2 would fit
> better.
> 
> Can you please run some tests on these?
> 

Sure! Will do in the next few days.

Best,

- Juri

> They are pushed here for easy access (and auto test by build-bot):
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject
> 
> --
> viresh
> 
> Viresh Kumar (5):
>   cpufreq: governor: Kill declare_show_sampling_rate_min()
>   cpufreq: governor: Create separate sysfs-ops
>   cpufreq: governor: Remove unused sysfs attribute macros
>   cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT
>   cpufreq: Get rid of ->governor_enabled and its lock
> 
>  drivers/cpufreq/cpufreq.c              |  29 ----------
>  drivers/cpufreq/cpufreq_conservative.c |  77 ++++++++++---------------
>  drivers/cpufreq/cpufreq_governor.c     |  86 ++++++++++++++++++++--------
>  drivers/cpufreq/cpufreq_governor.h     | 101 +++++++--------------------------
>  drivers/cpufreq/cpufreq_ondemand.c     |  77 ++++++++++---------------
>  include/linux/cpufreq.h                |   5 --
>  6 files changed, 143 insertions(+), 232 deletions(-)
> 
> -- 
> 2.7.0.79.gdc08a19
> 

Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> Sorry for doing this, I know you were also looking to fix this in a
> possibly different way. But I thought, it would be better if we fix
> that. We can scrap this version and take yours if that looks better.

That's not nice to be honest.  At the very least you could have asked
me about the status of my work before sending this.

Fortunately, though, when I started to look deeper into fixing this
problem I thought I didn't like the overall design of things in the
governor land, so I started to change that and my modifications turn
out to be sort of complementary with respect to this patchset.  Of
course, they do conflict, but I can redo my patches on top of these if
that's necessary.  That said I'm going to post them in their current
form anyway, at least to show the direction I want to take going
forward.

> The root cause of all the issues we were facing, was that we were taking
> policy->rwsem while accessing governor sysfs attributes. And that
> happened because we were sharing the show/store calls present in
> cpufreq.c.
>
> I thought, perhaps the best way to fix it is to give separate sysfs-ops
> to governors. And that's what I did.

Yes, that's what I was talking about in the other thread.

My overall impression here is that the code changes make sense.  Some
details need to be improved (like the concurrent ->store for governor
tunables pointed out by Juri).  The patch changelogs suck, though.

If your hope was that this might go into 4.5, there is a small chance
of that happening, but only if it can be made ready this week.
Otherwise, I'd prefer it to be redone on top of my changes.

Let me comment on the individual patches.

Thanks,
Rafael

Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[Viresh Kumar]

On 02-02-16, 21:04, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Hi Rafael,
> >
> > Sorry for doing this, I know you were also looking to fix this in a
> > possibly different way. But I thought, it would be better if we fix
> > that. We can scrap this version and take yours if that looks better.
> 
> That's not nice to be honest.  At the very least you could have asked
> me about the status of my work before sending this.

I started looking into this yesterday morning, while you were away and
finished it before you came back. So, it would have taken more time
and so I just sent them. I don't have any issues (as mentioned
earlier) is discarding them for the work you might have already done.

> Fortunately, though, when I started to look deeper into fixing this
> problem I thought I didn't like the overall design of things in the
> governor land, so I started to change that and my modifications turn
> out to be sort of complementary with respect to this patchset.

That's good.

> Of
> course, they do conflict, but I can redo my patches on top of these if
> that's necessary.

Yeah, even I don't have any issues in rebasing over your work, if I
have to.

> That said I'm going to post them in their current
> form anyway, at least to show the direction I want to take going
> forward.

Sure.

> > I thought, perhaps the best way to fix it is to give separate sysfs-ops
> > to governors. And that's what I did.
> 
> Yes, that's what I was talking about in the other thread.

I must have missed that then :(

> My overall impression here is that the code changes make sense.  Some
> details need to be improved (like the concurrent ->store for governor
> tunables pointed out by Juri). 

> The patch changelogs suck, though.

Like always :(

> If your hope was that this might go into 4.5, there is a small chance
> of that happening, but only if it can be made ready this week.

Will try my best.

> Otherwise, I'd prefer it to be redone on top of my changes.

No worries.

> Let me comment on the individual patches.

Thanks for taking this up Rafael, really appreciate it :)

-- 
viresh

Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups

[Viresh Kumar]

On 03-02-16, 07:52, Viresh Kumar wrote:
> > My overall impression here is that the code changes make sense.  Some
> > details need to be improved (like the concurrent ->store for governor
> > tunables pointed out by Juri). 

The next version is ready with some improvements, will send it once
you are done with this thread.

-- 
viresh