All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/2] cpufreq: Use sorted frequency tables
@ 2016-06-01 10:39 Viresh Kumar
  2016-06-01 10:39 ` [PATCH V2 1/2] cpufreq: Store sorted frequency table Viresh Kumar
  2016-06-01 10:39 ` [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target() Viresh Kumar
  0 siblings, 2 replies; 7+ messages in thread
From: Viresh Kumar @ 2016-06-01 10:39 UTC (permalink / raw)
  To: Rafael Wysocki; +Cc: linaro-kernel, linux-pm, smuckle, Viresh Kumar

Hi Guys,

This work in inspired by some of the concerns raised by Steve in one of
his patchset.

Currently, the cpufreq drivers aren't required to provide a sorted list
of frequencies to the cpufreq-core and so traversing that list match a
target frequency is very inefficient.

This is not bearable by, for example, the fast-switch path of schedutil
governor and so we have moved the traversing logic local to the
acpi-cpufreq driver for now. That is better handled in the core, but it
has to be efficient first.

OTOH, even for traditional governors without a fast-switch path, it
would be much better to be able to traverse this table quickly.

The ideal solution would be to keep a single *sorted* freq-table in
struct cpufreq_policy. But there are few dependencies due to which it
can't be done today (Hint: cpufreq drivers are abusing the 'index'
passed to them, to refer to multiple arrays).

And so for now, lets create a separate table local to the cpufreq-core
*only*.

This modifies the existing API cpufreq_frequency_table_target() to use
the sorted table.

Lightly tested on Exynos board, frequencies were getting selected as
expected.

V1->V2:
- This optimizes cpufreq_frequency_table_target() instead of new APIs.
- Is rebased over bleeding-edge + following cleanup series.
  [PATCH 0/8] cpufreq: cleanups and reorganization

--
viresh

Viresh Kumar (2):
  cpufreq: Store sorted frequency table
  cpufreq: Optimize cpufreq_frequency_table_target()

 drivers/cpufreq/acpi-cpufreq.c    |  15 +--
 drivers/cpufreq/cpufreq.c         |  23 ++--
 drivers/cpufreq/freq_table.c      | 247 ++++++++++++++++++++++++++------------
 drivers/cpufreq/s3c24xx-cpufreq.c |  13 +-
 include/linux/cpufreq.h           |  23 +++-
 5 files changed, 220 insertions(+), 101 deletions(-)

-- 
2.7.1.410.g6faf27b


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH V2 1/2] cpufreq: Store sorted frequency table
  2016-06-01 10:39 [PATCH V2 0/2] cpufreq: Use sorted frequency tables Viresh Kumar
@ 2016-06-01 10:39 ` Viresh Kumar
  2016-06-01 10:39 ` [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target() Viresh Kumar
  1 sibling, 0 replies; 7+ messages in thread
From: Viresh Kumar @ 2016-06-01 10:39 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar
  Cc: linaro-kernel, linux-pm, smuckle, linux-kernel

The drivers aren't required to provide a sorted frequency table today,
and its not optimal to work on an unsorted freq table.

To simplify and improve code performance, create a frequency table
within core and keep it sorted in ascending order.

Note that this should eventually replace policy->freq_table itself,
which isn't done currently as few drivers will break due to that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c    | 23 ++++++++++-----
 drivers/cpufreq/freq_table.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h      |  3 ++
 3 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 07c933c6c29a..799e03daa4a3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1137,6 +1137,16 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy, bool notify)
 	kfree(policy);
 }
 
+static void cpufreq_policy_exit(struct cpufreq_policy *policy)
+{
+	if (!cpufreq_driver->exit)
+		return;
+
+	cpufreq_driver->exit(policy);
+	free_sorted_freq_table(policy);
+	policy->freq_table = NULL;
+}
+
 static int cpufreq_online(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
@@ -1178,6 +1188,10 @@ static int cpufreq_online(unsigned int cpu)
 		goto out_free_policy;
 	}
 
+	ret = create_sorted_freq_table(policy);
+	if (ret)
+		goto out_exit_policy;
+
 	down_write(&policy->rwsem);
 
 	if (new_policy) {
@@ -1291,9 +1305,7 @@ static int cpufreq_online(unsigned int cpu)
 
 out_exit_policy:
 	up_write(&policy->rwsem);
-
-	if (cpufreq_driver->exit)
-		cpufreq_driver->exit(policy);
+	cpufreq_policy_exit(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, !new_policy);
 	return ret;
@@ -1378,10 +1390,7 @@ static void cpufreq_offline(unsigned int cpu)
 	 * since this is a core component, and is essential for the
 	 * subsequent light-weight ->init() to succeed.
 	 */
-	if (cpufreq_driver->exit) {
-		cpufreq_driver->exit(policy);
-		policy->freq_table = NULL;
-	}
+	cpufreq_policy_exit(policy);
 
 unlock:
 	up_write(&policy->rwsem);
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index eac8bcbdaad1..d536136c8e1f 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -13,6 +13,7 @@
 
 #include <linux/cpufreq.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 /*********************************************************************
  *                     FREQUENCY TABLE HELPERS                       *
@@ -297,6 +298,72 @@ struct freq_attr *cpufreq_generic_attr[] = {
 };
 EXPORT_SYMBOL_GPL(cpufreq_generic_attr);
 
+static int next_larger(struct cpufreq_policy *policy, unsigned int freq,
+		       struct cpufreq_frequency_table *table)
+{
+	struct cpufreq_frequency_table *pos;
+	unsigned int next_freq = ~0;
+	int index = -EINVAL;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency <= freq)
+			continue;
+
+		if (next_freq > pos->frequency) {
+			next_freq = pos->frequency;
+			index = pos - table;
+		}
+	}
+
+	return index;
+}
+
+void free_sorted_freq_table(struct cpufreq_policy *policy)
+{
+	kfree(policy->sorted_freq_table);
+	policy->sorted_freq_table = NULL;
+}
+
+int create_sorted_freq_table(struct cpufreq_policy *policy)
+{
+	struct cpufreq_frequency_table *pos, *new_table;
+	unsigned int freq, index, i, count = 0;
+	struct cpufreq_frequency_table *table = policy->freq_table;
+
+	if (!table)
+		return 0;
+
+	cpufreq_for_each_valid_entry(pos, table)
+		count++;
+
+	/* Extra entry for CPUFREQ_TABLE_END */
+	count++;
+
+	new_table = kmalloc_array(count, sizeof(*new_table), GFP_KERNEL);
+	if (!new_table)
+		return -ENOMEM;
+
+	for (i = 0, freq = 0; i < count - 1; i++) {
+		index = next_larger(policy, freq, table);
+		if (index == -EINVAL)
+			break;
+
+		/*
+		 * driver_data of the sorted table points to the index of the
+		 * unsorted table.
+		 */
+		new_table[i].driver_data = index;
+		new_table[i].frequency = table[index].frequency;
+
+		freq = table[index].frequency;
+	}
+
+	new_table[i].frequency = CPUFREQ_TABLE_END;
+	policy->sorted_freq_table = new_table;
+
+	return 0;
+}
+
 int cpufreq_table_validate_and_show(struct cpufreq_policy *policy,
 				      struct cpufreq_frequency_table *table)
 {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c378776628b4..5e23eed2252c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -87,6 +87,7 @@ struct cpufreq_policy {
 
 	struct cpufreq_user_policy user_policy;
 	struct cpufreq_frequency_table	*freq_table;
+	struct cpufreq_frequency_table	*sorted_freq_table;
 
 	struct list_head        policy_list;
 	struct kobject		kobj;
@@ -592,6 +593,8 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
 				    struct cpufreq_frequency_table *table);
+int create_sorted_freq_table(struct cpufreq_policy *policy);
+void free_sorted_freq_table(struct cpufreq_policy *policy);
 
 int cpufreq_frequency_table_verify(struct cpufreq_policy *policy,
 				   struct cpufreq_frequency_table *table);
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target()
  2016-06-01 10:39 [PATCH V2 0/2] cpufreq: Use sorted frequency tables Viresh Kumar
  2016-06-01 10:39 ` [PATCH V2 1/2] cpufreq: Store sorted frequency table Viresh Kumar
@ 2016-06-01 10:39 ` Viresh Kumar
  2016-06-01 19:46   ` Steve Muckle
  1 sibling, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2016-06-01 10:39 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar
  Cc: linaro-kernel, linux-pm, smuckle, linux-kernel

cpufreq core keeps another table of sorted frequencies now and that can
be used to find a match quickly, instead of traversing the unsorted list
in an inefficient way.

Create helper routines for separate relation types to optimize it
further.

Ideally the new routine cpufreq_find_target_index() is required to be
exported, but s3c24xx was abusing the earlier API and have to be
supported for now. Added a FIXME for it.

Tested on Exynos board with both ondemand and schedutil governor and
confirmed with help of various print messages that we are eventually
switching to the desired frequency based on a target frequency.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c    |  15 ++--
 drivers/cpufreq/freq_table.c      | 180 ++++++++++++++++++++++----------------
 drivers/cpufreq/s3c24xx-cpufreq.c |  13 ++-
 include/linux/cpufreq.h           |  20 ++++-
 4 files changed, 134 insertions(+), 94 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 32a15052f363..4f9f7504a17c 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -468,20 +468,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 	struct acpi_cpufreq_data *data = policy->driver_data;
 	struct acpi_processor_performance *perf;
 	struct cpufreq_frequency_table *entry;
-	unsigned int next_perf_state, next_freq, freq;
+	unsigned int next_perf_state, next_freq, index;
 
 	/*
 	 * Find the closest frequency above target_freq.
-	 *
-	 * The table is sorted in the reverse order with respect to the
-	 * frequency and all of the entries are valid (see the initialization).
 	 */
-	entry = policy->freq_table;
-	do {
-		entry++;
-		freq = entry->frequency;
-	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
-	entry--;
+	index = cpufreq_frequency_table_target(policy, target_freq,
+					       CPUFREQ_RELATION_L);
+
+	entry = &policy->freq_table[index];
 	next_freq = entry->frequency;
 	next_perf_state = entry->driver_data;
 
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index d536136c8e1f..15c4a2462c68 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -114,99 +114,131 @@ int cpufreq_generic_frequency_table_verify(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
-int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-				    unsigned int target_freq,
-				    unsigned int relation)
+static int cpufreq_find_target_index_l(struct cpufreq_policy *policy,
+				       struct cpufreq_frequency_table *table,
+				       unsigned int target_freq)
 {
-	struct cpufreq_frequency_table optimal = {
-		.driver_data = ~0,
-		.frequency = 0,
-	};
-	struct cpufreq_frequency_table suboptimal = {
-		.driver_data = ~0,
-		.frequency = 0,
-	};
-	struct cpufreq_frequency_table *pos;
-	struct cpufreq_frequency_table *table = policy->freq_table;
-	unsigned int freq, diff, i = 0;
-	int index;
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
 
-	pr_debug("request for target %u kHz (relation: %u) for cpu %u\n",
-					target_freq, relation, policy->cpu);
+	cpufreq_for_each_valid_entry(pos, table) {
+		freq = pos->frequency;
 
-	switch (relation) {
-	case CPUFREQ_RELATION_H:
-		suboptimal.frequency = ~0;
-		break;
-	case CPUFREQ_RELATION_L:
-	case CPUFREQ_RELATION_C:
-		optimal.frequency = ~0;
+		if ((freq < policy->min) || (freq > policy->max))
+			continue;
+
+		if (freq >= target_freq)
+			return pos - table;
+
+		best = pos;
+	}
+
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+static int cpufreq_find_target_index_h(struct cpufreq_policy *policy,
+				       struct cpufreq_frequency_table *table,
+				       unsigned int target_freq)
+{
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		freq = pos->frequency;
+
+		if ((freq < policy->min) || (freq > policy->max))
+			continue;
+
+		if (freq == target_freq)
+			return pos - table;
+
+		if (freq < target_freq) {
+			best = pos;
+			continue;
+		}
+
+		/* No freq found below target_freq */
+		if (!best)
+			best = pos;
 		break;
 	}
 
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+static int cpufreq_find_target_index_c(struct cpufreq_policy *policy,
+				       struct cpufreq_frequency_table *table,
+				       unsigned int target_freq)
+{
+	struct cpufreq_frequency_table *pos, *best = NULL;
+	unsigned int freq;
+
 	cpufreq_for_each_valid_entry(pos, table) {
 		freq = pos->frequency;
 
-		i = pos - table;
 		if ((freq < policy->min) || (freq > policy->max))
 			continue;
-		if (freq == target_freq) {
-			optimal.driver_data = i;
-			break;
+
+		if (freq == target_freq)
+			return pos - table;
+
+		if (freq < target_freq) {
+			best = pos;
+			continue;
 		}
-		switch (relation) {
-		case CPUFREQ_RELATION_H:
-			if (freq < target_freq) {
-				if (freq >= optimal.frequency) {
-					optimal.frequency = freq;
-					optimal.driver_data = i;
-				}
-			} else {
-				if (freq <= suboptimal.frequency) {
-					suboptimal.frequency = freq;
-					suboptimal.driver_data = i;
-				}
-			}
-			break;
-		case CPUFREQ_RELATION_L:
-			if (freq > target_freq) {
-				if (freq <= optimal.frequency) {
-					optimal.frequency = freq;
-					optimal.driver_data = i;
-				}
-			} else {
-				if (freq >= suboptimal.frequency) {
-					suboptimal.frequency = freq;
-					suboptimal.driver_data = i;
-				}
-			}
-			break;
-		case CPUFREQ_RELATION_C:
-			diff = abs(freq - target_freq);
-			if (diff < optimal.frequency ||
-			    (diff == optimal.frequency &&
-			     freq > table[optimal.driver_data].frequency)) {
-				optimal.frequency = diff;
-				optimal.driver_data = i;
-			}
+
+		/* No freq found below target_freq */
+		if (!best) {
+			best = pos;
 			break;
 		}
+
+		/* Choose the closest freq */
+		if (target_freq - best->frequency > freq - target_freq)
+			best = pos;
+
+		break;
+	}
+
+	if (best)
+		return best - table;
+
+	return -EINVAL;
+}
+
+int cpufreq_find_target_index(struct cpufreq_policy *policy,
+			      struct cpufreq_frequency_table *table,
+			      unsigned int target_freq, unsigned int relation)
+{
+	int index;
+
+	switch (relation) {
+	case CPUFREQ_RELATION_L:
+		index = cpufreq_find_target_index_l(policy, table, target_freq);
+		break;
+	case CPUFREQ_RELATION_H:
+		index = cpufreq_find_target_index_h(policy, table, target_freq);
+		break;
+	case CPUFREQ_RELATION_C:
+		index = cpufreq_find_target_index_c(policy, table, target_freq);
+		break;
+	default:
+		pr_err("%s: Invalid relation: %d\n", __func__, relation);
+		return -EINVAL;
 	}
-	if (optimal.driver_data > i) {
-		if (suboptimal.driver_data > i) {
-			WARN(1, "Invalid frequency table: %d\n", policy->cpu);
-			return 0;
-		}
 
-		index = suboptimal.driver_data;
-	} else
-		index = optimal.driver_data;
+	if (index == -EINVAL)
+		WARN(1, "Invalid frequency table: %d\n", policy->cpu);
 
-	pr_debug("target index is %u, freq is:%u kHz\n", index,
-		 table[index].frequency);
 	return index;
 }
-EXPORT_SYMBOL_GPL(cpufreq_frequency_table_target);
+EXPORT_SYMBOL_GPL(cpufreq_find_target_index);
 
 int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
 		unsigned int freq)
diff --git a/drivers/cpufreq/s3c24xx-cpufreq.c b/drivers/cpufreq/s3c24xx-cpufreq.c
index 7b596fa38ad2..c9c2b4151b9f 100644
--- a/drivers/cpufreq/s3c24xx-cpufreq.c
+++ b/drivers/cpufreq/s3c24xx-cpufreq.c
@@ -318,14 +318,13 @@ static int s3c_cpufreq_target(struct cpufreq_policy *policy,
 		tmp_policy.min = policy->min * 1000;
 		tmp_policy.max = policy->max * 1000;
 		tmp_policy.cpu = policy->cpu;
-		tmp_policy.freq_table = pll_reg;
 
-		/* cpufreq_frequency_table_target returns the index
-		 * of the table entry, not the value of
-		 * the table entry's index field. */
-
-		index = cpufreq_frequency_table_target(&tmp_policy, target_freq,
-						       relation);
+		/*
+		 * FIXME: We should be providing this freq-table to the cpufreq
+		 * core instead of managing it ourselves here.
+		 */
+		index = cpufreq_find_target_index(&tmp_policy, pll_reg,
+						  target_freq, relation);
 		pll = pll_reg + index;
 
 		s3c_freq_dbg("%s: target %u => %u\n",
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 5e23eed2252c..5aabec611e87 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -600,14 +600,28 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy *policy,
 				   struct cpufreq_frequency_table *table);
 int cpufreq_generic_frequency_table_verify(struct cpufreq_policy *policy);
 
-int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-				   unsigned int target_freq,
-				   unsigned int relation);
+int cpufreq_find_target_index(struct cpufreq_policy *policy,
+			      struct cpufreq_frequency_table *table,
+			      unsigned int target_freq, unsigned int relation);
 int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
 		unsigned int freq);
 
 ssize_t cpufreq_show_cpus(const struct cpumask *mask, char *buf);
 
+/*
+ * Search in the sorted freq table local to the core and return index of
+ * policy->freq_table.
+ */
+static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
+						 unsigned int target_freq,
+						 unsigned int relation)
+{
+	int index = cpufreq_find_target_index(policy, policy->sorted_freq_table,
+					      target_freq, relation);
+
+	return policy->sorted_freq_table[index].driver_data;
+}
+
 #ifdef CONFIG_CPU_FREQ
 int cpufreq_boost_trigger_state(int state);
 int cpufreq_boost_enabled(void);
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target()
  2016-06-01 10:39 ` [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target() Viresh Kumar
@ 2016-06-01 19:46   ` Steve Muckle
  2016-06-02  1:29     ` Viresh Kumar
  0 siblings, 1 reply; 7+ messages in thread
From: Steve Muckle @ 2016-06-01 19:46 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, linaro-kernel, linux-pm, linux-kernel,
	Peter Zijlstra, Ingo Molnar

On Wed, Jun 01, 2016 at 04:09:55PM +0530, Viresh Kumar wrote:
> cpufreq core keeps another table of sorted frequencies now and that can
> be used to find a match quickly, instead of traversing the unsorted list
> in an inefficient way.
> 
> Create helper routines for separate relation types to optimize it
> further.
> 
> Ideally the new routine cpufreq_find_target_index() is required to be
> exported, but s3c24xx was abusing the earlier API and have to be
> supported for now. Added a FIXME for it.
> 
> Tested on Exynos board with both ondemand and schedutil governor and
> confirmed with help of various print messages that we are eventually
> switching to the desired frequency based on a target frequency.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/acpi-cpufreq.c    |  15 ++--
>  drivers/cpufreq/freq_table.c      | 180 ++++++++++++++++++++++----------------
>  drivers/cpufreq/s3c24xx-cpufreq.c |  13 ++-
>  include/linux/cpufreq.h           |  20 ++++-
>  4 files changed, 134 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 32a15052f363..4f9f7504a17c 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -468,20 +468,15 @@ unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
>  	struct acpi_cpufreq_data *data = policy->driver_data;
>  	struct acpi_processor_performance *perf;
>  	struct cpufreq_frequency_table *entry;
> -	unsigned int next_perf_state, next_freq, freq;
> +	unsigned int next_perf_state, next_freq, index;
>  
>  	/*
>  	 * Find the closest frequency above target_freq.
> -	 *
> -	 * The table is sorted in the reverse order with respect to the
> -	 * frequency and all of the entries are valid (see the initialization).
>  	 */
> -	entry = policy->freq_table;
> -	do {
> -		entry++;
> -		freq = entry->frequency;
> -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> -	entry--;
> +	index = cpufreq_frequency_table_target(policy, target_freq,
> +					       CPUFREQ_RELATION_L);

This adds a function call to the fast path...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target()
  2016-06-01 19:46   ` Steve Muckle
@ 2016-06-02  1:29     ` Viresh Kumar
  2016-06-02 18:28       ` Steve Muckle
  0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2016-06-02  1:29 UTC (permalink / raw)
  To: Steve Muckle
  Cc: Rafael Wysocki, linaro-kernel, linux-pm, linux-kernel,
	Peter Zijlstra, Ingo Molnar

On 01-06-16, 12:46, Steve Muckle wrote:
> >  	/*
> >  	 * Find the closest frequency above target_freq.
> > -	 *
> > -	 * The table is sorted in the reverse order with respect to the
> > -	 * frequency and all of the entries are valid (see the initialization).
> >  	 */
> > -	entry = policy->freq_table;
> > -	do {
> > -		entry++;
> > -		freq = entry->frequency;
> > -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> > -	entry--;
> > +	index = cpufreq_frequency_table_target(policy, target_freq,
> > +					       CPUFREQ_RELATION_L);
> 
> This adds a function call to the fast path...

I understand that, but I am not sure how far should we go to avoid
that. Open coding routines to save on that isn't a good idea surely.

I have at least kept this routine in cpufreq.h to avoid a call, but
eventually we will have at least a call somewhere within it. :(

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target()
  2016-06-02  1:29     ` Viresh Kumar
@ 2016-06-02 18:28       ` Steve Muckle
  2016-06-03  3:16         ` Viresh Kumar
  0 siblings, 1 reply; 7+ messages in thread
From: Steve Muckle @ 2016-06-02 18:28 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Steve Muckle, Rafael Wysocki, linaro-kernel, linux-pm,
	linux-kernel, Peter Zijlstra, Ingo Molnar

On Thu, Jun 02, 2016 at 06:59:04AM +0530, Viresh Kumar wrote:
> On 01-06-16, 12:46, Steve Muckle wrote:
> > >  	/*
> > >  	 * Find the closest frequency above target_freq.
> > > -	 *
> > > -	 * The table is sorted in the reverse order with respect to the
> > > -	 * frequency and all of the entries are valid (see the initialization).
> > >  	 */
> > > -	entry = policy->freq_table;
> > > -	do {
> > > -		entry++;
> > > -		freq = entry->frequency;
> > > -	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> > > -	entry--;
> > > +	index = cpufreq_frequency_table_target(policy, target_freq,
> > > +					       CPUFREQ_RELATION_L);
> > 
> > This adds a function call to the fast path...
> 
> I understand that, but I am not sure how far should we go to avoid
> that. Open coding routines to save on that isn't a good idea surely.
> 
> I have at least kept this routine in cpufreq.h to avoid a call, but
> eventually we will have at least a call somewhere within it. :(

Shouldn't we be able to avoid extra function calls through the use of
macros/inlines? Otherwise this is making things slower for schedutil
than it is today. 

Actually cpufreq_driver_resolve_freq() shouldn't require any calls from
schedutil when a freq_table is available - the whole thing could be run
inline.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target()
  2016-06-02 18:28       ` Steve Muckle
@ 2016-06-03  3:16         ` Viresh Kumar
  0 siblings, 0 replies; 7+ messages in thread
From: Viresh Kumar @ 2016-06-03  3:16 UTC (permalink / raw)
  To: Steve Muckle
  Cc: Rafael Wysocki, linaro-kernel, linux-pm, linux-kernel,
	Peter Zijlstra, Ingo Molnar

On 02-06-16, 11:28, Steve Muckle wrote:
> Shouldn't we be able to avoid extra function calls through the use of
> macros/inlines? Otherwise this is making things slower for schedutil
> than it is today. 
> 
> Actually cpufreq_driver_resolve_freq() shouldn't require any calls from
> schedutil when a freq_table is available - the whole thing could be run
> inline.

I will see what I can do on that. Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-06-03  3:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-01 10:39 [PATCH V2 0/2] cpufreq: Use sorted frequency tables Viresh Kumar
2016-06-01 10:39 ` [PATCH V2 1/2] cpufreq: Store sorted frequency table Viresh Kumar
2016-06-01 10:39 ` [PATCH V2 2/2] cpufreq: Optimize cpufreq_frequency_table_target() Viresh Kumar
2016-06-01 19:46   ` Steve Muckle
2016-06-02  1:29     ` Viresh Kumar
2016-06-02 18:28       ` Steve Muckle
2016-06-03  3:16         ` Viresh Kumar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.