Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325473 > unrolled thread

[PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 15:10 +0100
    Re: [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:40 +0100
      Re: [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a  governor-specific tunable Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 03:50 +0100

#1325473 — [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable

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.

[ Rafael: Written changelog ]
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

[toc] | [next] | [standalone]

#1327415

On Wednesday, February 03, 2016 07:32:17 PM Viresh Kumar wrote:
> 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.
> 
> [ Rafael: Written changelog ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I'm having some second thoughts about the utility of this patch to be honest.

I actually would like to move some tunables in the opposite direction.  That is,
from struct od_dbs_tuners and struct cs_dbs_tuners to struct dbs_data.  The
tuners field in that will then become something like gov_tunables (in analogy
with gov_ops in struct common_dbs_data) and it will point to governor-specific
tunables.

The reason why I'd like to do that is to make it easier to get rid of the
super-ugly governor == GOV_CONSERVATIVE etc tests in the common code.

Also I think that governor-specific tunables should be defined in the .c file
for that governor rather than in the common header.

We will need two set of macros for their sysfs attributes then, but that's
not a big deal IMO.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1327424 — Re: [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable

On 05-02-16, 03:31, Rafael J. Wysocki wrote:
> I'm having some second thoughts about the utility of this patch to be honest.
> 
> I actually would like to move some tunables in the opposite direction.  That is,
> from struct od_dbs_tuners and struct cs_dbs_tuners to struct dbs_data.  The
> tuners field in that will then become something like gov_tunables (in analogy
> with gov_ops in struct common_dbs_data) and it will point to governor-specific
> tunables.
> 
> The reason why I'd like to do that is to make it easier to get rid of the
> super-ugly governor == GOV_CONSERVATIVE etc tests in the common code.
> 
> Also I think that governor-specific tunables should be defined in the .c file
> for that governor rather than in the common header.
> 
> We will need two set of macros for their sysfs attributes then, but that's
> not a big deal IMO.

I agree with that, no issues from my side.

But, this patch was actually required to kill the ugly macros. So, if
we are planning to take this series as is, then maybe we can keep it
for now and fix everything together with your patches :)

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web