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

Groups > linux.kernel > #1413249 > unrolled thread

[PATCH V3 0/9] cpufreq: Sort policy->freq_table

Back to article view | Back to linux.kernel

Contents

  [PATCH V3 0/9] cpufreq: Sort policy->freq_table Viresh Kumar <viresh.kumar@linaro.org> - 2016-06-03 15:40 +0200
    [PATCH V3 5/9] cpufreq: ia64: Use 'index' only to index into policy->freq_table Viresh Kumar <viresh.kumar@linaro.org> - 2016-06-03 15:40 +0200
    [PATCH V3 9/9] cpufreq: drivers: Free frequency tables after being used Viresh Kumar <viresh.kumar@linaro.org> - 2016-06-03 15:40 +0200
      Re: [PATCH V3 9/9] cpufreq: drivers: Free frequency tables after  being used "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-07 00:50 +0200
    Re: [PATCH V3 0/9] cpufreq: Sort policy->freq_table "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-06 23:10 +0200
      Re: [PATCH V3 0/9] cpufreq: Sort policy->freq_table Viresh Kumar <viresh.kumar@linaro.org> - 2016-06-07 05:50 +0200

#1413249 — [PATCH V3 0/9] cpufreq: Sort policy->freq_table

Hi Rafael,

So all my patches are contained in two series. The first one is:

[PATCH V3 0/8] cpufreq: cleanups and reorganization

which I have sent this morning. It does some cleanup and shall be
applied regardless of this series.

This series improves the performance of cpufreq_frequency_table_target()
routine by storing the policy->freq_table sorted in ascending order. On
one hand it shall improve the performance of current governors for
drivers providing freq-table to cpufreq core and on another hand this
API can be used directly from schedutil governor as well.

As Steve has requested earlier, these APIs are moved to a .h file to
avoid function calls.

Steve's series can use this API now without any performance lag.

The first 7 patches makes sure that current drivers wouldn't break because
of reordering of policy->freq_table and the 8th patch updates cpufreq
core to sort policy->freq_table and make
cpufreq_frequency_table_target() much more efficient.

This series is tested on ARM exynos platform and all the frequencies are
switching properly with both ondemand and schedutil governor.

V2->V3:
- Combined two series as you suggested
- Moved the new APIs and cpufreq_frequency_table_target() to a .h

All the patches are pushed here for testing in case anyone wants to try:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/sorted-freq-table

Thanks

--
viresh

Viresh Kumar (9):
  cpufreq: Use policy->freq_table in ->target_index()
  cpufreq: blackfin: Use 'index' only to index into policy->freq_table
  cpufreq: elanfreq: Use 'index' only to index into policy->freq_table
  cpufreq: exynos: Use 'index' only to index into policy->freq_table
  cpufreq: ia64: Use 'index' only to index into policy->freq_table
  cpufreq: imx: Use 'index' only to index into policy->freq_table
  cpufreq: maple: Use 'index' only to index into policy->freq_table
  cpufreq: Keep policy->freq_table sorted in ascending order
  cpufreq: drivers: Free frequency tables after being used

 MAINTAINERS                            |   1 +
 drivers/cpufreq/acpi-cpufreq.c         |  23 ++---
 drivers/cpufreq/arm_big_little.c       |   2 +-
 drivers/cpufreq/at32ap-cpufreq.c       |   8 +-
 drivers/cpufreq/blackfin-cpufreq.c     |  17 +++-
 drivers/cpufreq/cpufreq-dt.c           |   9 +-
 drivers/cpufreq/cpufreq.c              |  20 ++--
 drivers/cpufreq/cpufreq_ondemand.h     |   1 +
 drivers/cpufreq/cris-artpec3-cpufreq.c |   2 +-
 drivers/cpufreq/cris-etraxfs-cpufreq.c |   2 +-
 drivers/cpufreq/dbx500-cpufreq.c       |   3 +-
 drivers/cpufreq/e_powersaver.c         |  26 +++---
 drivers/cpufreq/elanfreq.c             |   8 +-
 drivers/cpufreq/exynos5440-cpufreq.c   |  13 ++-
 drivers/cpufreq/freq_table.c           | 163 +++++++++++++--------------------
 drivers/cpufreq/ia64-acpi-cpufreq.c    |  16 +++-
 drivers/cpufreq/imx6q-cpufreq.c        |  13 ++-
 drivers/cpufreq/kirkwood-cpufreq.c     |   2 +-
 drivers/cpufreq/loongson1-cpufreq.c    |  10 +-
 drivers/cpufreq/loongson2_cpufreq.c    |   5 +-
 drivers/cpufreq/maple-cpufreq.c        |   6 ++
 drivers/cpufreq/powernv-cpufreq.c      |   1 +
 drivers/cpufreq/s3c24xx-cpufreq.c      |   1 +
 drivers/cpufreq/s5pv210-cpufreq.c      |   1 +
 include/linux/cpufreq.h                |   3 -
 include/linux/cpufreq_table.h          | 139 ++++++++++++++++++++++++++++
 26 files changed, 317 insertions(+), 178 deletions(-)
 create mode 100644 include/linux/cpufreq_table.h

-- 
2.7.1.410.g6faf27b

[toc] | [next] | [standalone]

#1413251 — [PATCH V3 5/9] cpufreq: ia64: Use 'index' only to index into policy->freq_table

Later patches would make changes in cpufreq core, after which
policy->freq_table may be reordered by cpufreq core and it wouldn't be
safe anymore to use 'index' for any other local arrays.

To prepare for that, use policy->freq_table[index].driver_data for other
driver specific usage of 'index'. The 'driver_data' fields are set
properly by the driver now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/ia64-acpi-cpufreq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c
index 759612da4fdc..cc8bb1e5ac50 100644
--- a/drivers/cpufreq/ia64-acpi-cpufreq.c
+++ b/drivers/cpufreq/ia64-acpi-cpufreq.c
@@ -210,7 +210,12 @@ acpi_cpufreq_target (
 	struct cpufreq_policy   *policy,
 	unsigned int index)
 {
-	return processor_set_freq(acpi_io_data[policy->cpu], policy, index);
+	/*
+	 * policy->freq_table may be sorted differently, get the index value we
+	 * are concerned about.
+	 */
+	return processor_set_freq(acpi_io_data[policy->cpu], policy,
+				  policy->freq_table[index].driver_data);
 }
 
 static int
@@ -282,6 +287,8 @@ acpi_cpufreq_cpu_init (
 		} else {
 			freq_table[i].frequency = CPUFREQ_TABLE_END;
 		}
+
+		freq_table[i].driver_data = i;
 	}
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
-- 
2.7.1.410.g6faf27b

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

#1413252 — [PATCH V3 9/9] cpufreq: drivers: Free frequency tables after being used

The cpufreq core doesn't use these tables anymore after
cpufreq_table_validate_and_show() has returned.  And so these can be
freed early.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c      |  7 +++----
 drivers/cpufreq/at32ap-cpufreq.c    |  6 +++---
 drivers/cpufreq/cpufreq-dt.c        |  9 ++++-----
 drivers/cpufreq/e_powersaver.c      | 24 ++++++++++++++----------
 drivers/cpufreq/ia64-acpi-cpufreq.c |  7 +++----
 drivers/cpufreq/loongson1-cpufreq.c | 10 +---------
 6 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 364b86119f3f..2e36677e5b36 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -815,8 +815,10 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	perf->state = 0;
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (result)
-		goto err_freqfree;
+		goto err_unreg;
 
 	if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq)
 		pr_warn(FW_WARN "P-state 0 is not max freq\n");
@@ -860,8 +862,6 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	return result;
 
-err_freqfree:
-	kfree(freq_table);
 err_unreg:
 	acpi_processor_unregister_performance(cpu);
 err_free_mask:
@@ -883,7 +883,6 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 	policy->driver_data = NULL;
 	acpi_processor_unregister_performance(data->acpi_perf_cpu);
 	free_cpumask_var(data->freqdomain_cpus);
-	kfree(policy->freq_table);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/cpufreq/at32ap-cpufreq.c b/drivers/cpufreq/at32ap-cpufreq.c
index 9231b1efb70d..c9751572ac8b 100644
--- a/drivers/cpufreq/at32ap-cpufreq.c
+++ b/drivers/cpufreq/at32ap-cpufreq.c
@@ -21,8 +21,6 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 
-static struct cpufreq_frequency_table *freq_table;
-
 static unsigned int	ref_freq;
 static unsigned long	loops_per_jiffy_ref;
 
@@ -51,6 +49,7 @@ static int at32_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int at32_cpufreq_driver_init(struct cpufreq_policy *policy)
 {
+	struct cpufreq_frequency_table *freq_table;
 	unsigned int frequency, rate, min_freq;
 	struct clk *cpuclk;
 	int retval, steps, i;
@@ -99,12 +98,13 @@ static int at32_cpufreq_driver_init(struct cpufreq_policy *policy)
 	freq_table[steps - 1].frequency = CPUFREQ_TABLE_END;
 
 	retval = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (!retval) {
 		printk("cpufreq: AT32AP CPU frequency driver\n");
 		return 0;
 	}
 
-	kfree(freq_table);
 out_err_put_clk:
 	clk_put(cpuclk);
 out_err:
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 3957de801ae8..d46741b69c59 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -253,10 +253,12 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	rcu_read_unlock();
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
+	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+
 	if (ret) {
 		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
 			ret);
-		goto out_free_cpufreq_table;
+		goto out_free_priv;
 	}
 
 	/* Support turbo/boost mode */
@@ -264,7 +266,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		/* This gets disabled by core on driver unregister */
 		ret = cpufreq_enable_boost_support();
 		if (ret)
-			goto out_free_cpufreq_table;
+			goto out_free_priv;
 		cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs;
 	}
 
@@ -276,8 +278,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	return 0;
 
-out_free_cpufreq_table:
-	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 out_free_priv:
 	kfree(priv);
 out_free_opp:
@@ -295,7 +295,6 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	struct private_data *priv = policy->driver_data;
 
 	cpufreq_cooling_unregister(priv->cdev);
-	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
 		dev_pm_opp_put_regulator(priv->cpu_dev);
diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
index a284bddfb067..6c6090492889 100644
--- a/drivers/cpufreq/e_powersaver.c
+++ b/drivers/cpufreq/e_powersaver.c
@@ -38,7 +38,6 @@ struct eps_cpu_data {
 #if IS_ENABLED(CONFIG_ACPI_PROCESSOR)
 	u32 bios_limit;
 #endif
-	struct cpufreq_frequency_table freq_table[];
 };
 
 static struct eps_cpu_data *eps_cpu[NR_CPUS];
@@ -324,11 +323,17 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 		states = 2;
 
 	/* Allocate private data and frequency table for current cpu */
-	centaur = kzalloc(sizeof(*centaur)
-		    + (states + 1) * sizeof(struct cpufreq_frequency_table),
-		    GFP_KERNEL);
+	centaur = kzalloc(sizeof(*centaur), GFP_KERNEL);
 	if (!centaur)
 		return -ENOMEM;
+
+	f_table = kzalloc((states + 1) * sizeof(struct cpufreq_frequency_table),
+			  GFP_KERNEL);
+	if (!f_table) {
+		kfree(centaur);
+		return -ENOMEM;
+	}
+
 	eps_cpu[0] = centaur;
 
 	/* Copy basic values */
@@ -338,7 +343,6 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 #endif
 
 	/* Fill frequency and MSR value table */
-	f_table = &centaur->freq_table[0];
 	if (brand != EPS_BRAND_C7M) {
 		f_table[0].frequency = fsb * min_multiplier;
 		f_table[0].driver_data = (min_multiplier << 8) | min_voltage;
@@ -360,13 +364,13 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = 140000; /* 844mV -> 700mV in ns */
 
-	ret = cpufreq_table_validate_and_show(policy, &centaur->freq_table[0]);
-	if (ret) {
+	ret = cpufreq_table_validate_and_show(policy, f_table);
+	if (ret)
 		kfree(centaur);
-		return ret;
-	}
 
-	return 0;
+	kfree(f_table);
+
+	return ret;
 }
 
 static int eps_cpu_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c
index cc8bb1e5ac50..10e3bfac84d5 100644
--- a/drivers/cpufreq/ia64-acpi-cpufreq.c
+++ b/drivers/cpufreq/ia64-acpi-cpufreq.c
@@ -292,8 +292,10 @@ acpi_cpufreq_cpu_init (
 	}
 
 	result = cpufreq_table_validate_and_show(policy, freq_table);
+	kfree(freq_table);
+
 	if (result) {
-		goto err_freqfree;
+		goto err_unreg;
 	}
 
 	/* notify BIOS that we exist */
@@ -317,8 +319,6 @@ acpi_cpufreq_cpu_init (
 
 	return (result);
 
- err_freqfree:
-	kfree(freq_table);
  err_unreg:
 	acpi_processor_unregister_performance(cpu);
  err_free:
@@ -340,7 +340,6 @@ acpi_cpufreq_cpu_exit (
 	if (data) {
 		acpi_io_data[policy->cpu] = NULL;
 		acpi_processor_unregister_performance(policy->cpu);
-		kfree(policy->freq_table);
 		kfree(data);
 	}
 
diff --git a/drivers/cpufreq/loongson1-cpufreq.c b/drivers/cpufreq/loongson1-cpufreq.c
index be89416e2358..2d35d3cc2ad8 100644
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1-cpufreq.c
@@ -103,18 +103,11 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->clk = cpufreq->clk;
 	ret = cpufreq_generic_init(policy, freq_tbl, 0);
-	if (ret)
-		kfree(freq_tbl);
+	kfree(freq_tbl);
 
 	return ret;
 }
 
-static int ls1x_cpufreq_exit(struct cpufreq_policy *policy)
-{
-	kfree(policy->freq_table);
-	return 0;
-}
-
 static struct cpufreq_driver ls1x_cpufreq_driver = {
 	.name		= "cpufreq-ls1x",
 	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
@@ -122,7 +115,6 @@ static struct cpufreq_driver ls1x_cpufreq_driver = {
 	.target_index	= ls1x_cpufreq_target,
 	.get		= cpufreq_generic_get,
 	.init		= ls1x_cpufreq_init,
-	.exit		= ls1x_cpufreq_exit,
 	.attr		= cpufreq_generic_attr,
 };
 
-- 
2.7.1.410.g6faf27b

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

#1415526 — Re: [PATCH V3 9/9] cpufreq: drivers: Free frequency tables after being used

On Fri, Jun 3, 2016 at 3:35 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The cpufreq core doesn't use these tables anymore after
> cpufreq_table_validate_and_show() has returned.  And so these can be
> freed early.

That doesn't look particularly efficient.

The driver has to allocate memory first and populate it and then the
core needs to allocate more memory again to store the same information
in it, but perhaps in a different order and then the driver can free
the memory allocated before right away.  It looks like there's one
excessive memory allocation here, so maybe the core can simply sort
the frequency table in place and overwrite the invalid entries in the
process?

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

#1415445

On Fri, Jun 3, 2016 at 3:35 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> So all my patches are contained in two series. The first one is:
>
> [PATCH V3 0/8] cpufreq: cleanups and reorganization
>
> which I have sent this morning. It does some cleanup and shall be
> applied regardless of this series.
>
> This series improves the performance of cpufreq_frequency_table_target()
> routine by storing the policy->freq_table sorted in ascending order. On
> one hand it shall improve the performance of current governors for
> drivers providing freq-table to cpufreq core and on another hand this
> API can be used directly from schedutil governor as well.
>
> As Steve has requested earlier, these APIs are moved to a .h file to
> avoid function calls.
>
> Steve's series can use this API now without any performance lag.
>
> The first 7 patches makes sure that current drivers wouldn't break because
> of reordering of policy->freq_table and the 8th patch updates cpufreq
> core to sort policy->freq_table and make
> cpufreq_frequency_table_target() much more efficient.

Is there any particular reason why patches [2-7/9] are separate?

They seem to be making analogous changes in several drivers (and even
the changelog seems to be the same), so why don't you make a single
patch out of them?

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

#1415664

On 06-06-16, 23:09, Rafael J. Wysocki wrote:
> Is there any particular reason why patches [2-7/9] are separate?

I just wanted them to be separately reviewable by platform driver owners.
Nothing else.

Will merge them in next version.

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web