diff mbox series

[RFC,1/2] cpufreq: Add special-purpose fast-switching callback for drivers

Message ID 2174134.tL5yAn4CWt@kreacher (mailing list archive)
State RFC
Headers show
Series cpufreq: Allow drivers to receive more information from the governor | expand

Commit Message

Rafael J. Wysocki Nov. 30, 2020, 6:37 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

First off, some cpufreq drivers (eg. intel_pstate) can pass hints
beyond the current target frequency to the hardware and there are no
provisions for doing that in the cpufreq framework.  In particular,
today the driver has to assume that it should allow the frequency to
fall below the one requested by the governor (or the required capacity
may not be provided) which may not be the case and which may lead to
excessive energy usage in some scenarios.

Second, the hints passed by these drivers to the hardware neeed not
be in terms of the frequency, so representing the utilization numbers
coming from the scheduler as frequency before passing them to those
drivers is not really useful.

Address the two points above by adding a special-purpose replacement
for the ->fast_switch callback, called ->adjust_perf, allowing the
governor to pass abstract performance level (rather than frequency)
values for the minimum (required) and target (desired) performance
along with the information whether or not the given CPU has been
busy since the last update (which may allow the driver to skip the
update in some cases).

Also update the schedutil governor to use the new callback instead
of ->fast_switch if present.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c        |   41 +++++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h          |   14 +++++++++++++
 include/linux/sched/cpufreq.h    |    5 ++++
 kernel/sched/cpufreq_schedutil.c |   40 ++++++++++++++++++++++++++------------
 4 files changed, 88 insertions(+), 12 deletions(-)

Comments

Doug Smythies Dec. 2, 2020, 3:58 p.m. UTC | #1
Hi Rafael,

On 2020.11.30 10:37 Rafael J. Wysocki wrote:

> First off, some cpufreq drivers (eg. intel_pstate) can pass hints
> beyond the current target frequency to the hardware and there are no
> provisions for doing that in the cpufreq framework.  In particular,
> today the driver has to assume that it should allow the frequency to

Forgot the important "not":

today the driver has to assume that it should allow not the frequency to

> fall below the one requested by the governor (or the required capacity
> may not be provided) which may not be the case and which may lead to
> excessive energy usage in some scenarios.
> 
> Second, the hints passed by these drivers to the hardware neeed not

s/neeed/need

...

O.K. this is good.

The problem with my basic CPU frequency verses load test with the
schedutil governor is that it is always so oscillatory it is pretty
much not possible to conclude anything. So I re-worked the test
to look at Processor Package Power load instead.

In a previous e-mail [1] I had reported the power differences
for one periodic load at one frequency, as a (apparently cherry picked)
example. Quoted:

> schedutil governor:
> acpi-cpufreq: good
> intel_cpufreq hwp: bad    <<<<< Now good, with this patch set.
> intel_cpufreq no hwp: good
> ...
> periodic workflow at 347 hertz.
> ~36% load at 4.60 GHz (where hwp operates)
> ~55% load at 3.2 GHz (where no hwp operates)
>
> intel_cpufreq hwp: 9.6 processor package watts. 45.8 watts on the mains to the computer.
> intel_cpufreq no hwp: ~6 processor package watts. ~41 watts on the mains to the computer. (noisy)

So this time, I only have power/energy data, and a relatively easy way to compress all 12,000
samples into some concise summary is to simply find the average power for the entire experiment:

Legend:
hwp: Kernel 5.10-rc6, HWP enabled; intel_cpufreq; schedutil (always)
rjw: Kernel 5.10-rc6 + this patch set, HWP enabled; intel_cpu-freq; schedutil
no-hwp: Kernel 5.10-rc6, HWP disabled; intel_cpu-freq; schedutil
acpi-cpufreq: Kernel 5.10-rc6, HWP disabled; acpi-cpufreq; schedutil

load work/sleep frequency: 73 Hertz:
hwp: Average: 12.00822 watts
rjw: Average: 10.18089 watts
no-hwp: Average: 10.21947 watts
acpi-cpufreq: Average:  9.06585 watts

load work/sleep frequency: 113 Hertz:

hwp: Average: 12.01056
rjw: Average: 10.12303
no-hwp: Average: 10.08228
acpi-cpufreq: Average:  9.02215

load work/sleep frequency: 211 Hertz:

hwp: Average: 12.16067
rjw: Average: 10.24413
no-hwp: Average: 10.12463
acpi-cpufreq: Average:  9.19175

load work/sleep frequency: 347 Hertz:

hwp: Average: 12.34169
rjw: Average: 10.79980
no-hwp: Average: 10.57296
acpi-cpufreq: Average:  9.84709

load work/sleep frequency: 401 Hertz:

hwp: Average: 12.42562
rjw: Average: 11.12465
no-hwp: Average: 11.24203
acpi-cpufreq: Average: 10.78670

[1] https://marc.info/?l=linux-pm&m=159769839401767&w=2

My tests results graphs:
Note: I have to code the web site, or I get hammered by bots.
Note: it is .com only because it was less expensive than .org
73 Hertz:
Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su73/ 
113 Hertz:
Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su113/
211 Hertz:
Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su211/
347 Hertz:
Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su347/
401 Hertz:
Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su401/

... Doug
Rafael J. Wysocki Dec. 2, 2020, 5:39 p.m. UTC | #2
On Wed, Dec 2, 2020 at 4:59 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> Hi Rafael,
>
> On 2020.11.30 10:37 Rafael J. Wysocki wrote:
>
> > First off, some cpufreq drivers (eg. intel_pstate) can pass hints
> > beyond the current target frequency to the hardware and there are no
> > provisions for doing that in the cpufreq framework.  In particular,
> > today the driver has to assume that it should allow the frequency to
>
> Forgot the important "not":

Right, thanks for noticing that!

> today the driver has to assume that it should allow not the frequency to
>
> > fall below the one requested by the governor (or the required capacity
> > may not be provided) which may not be the case and which may lead to
> > excessive energy usage in some scenarios.
> >
> > Second, the hints passed by these drivers to the hardware neeed not
>
> s/neeed/need

Yup, thanks!

> ...
>
> O.K. this is good.
>
> The problem with my basic CPU frequency verses load test with the
> schedutil governor is that it is always so oscillatory it is pretty
> much not possible to conclude anything. So I re-worked the test
> to look at Processor Package Power load instead.
>
> In a previous e-mail [1] I had reported the power differences
> for one periodic load at one frequency, as a (apparently cherry picked)
> example. Quoted:
>
> > schedutil governor:
> > acpi-cpufreq: good
> > intel_cpufreq hwp: bad    <<<<< Now good, with this patch set.

OK, great!

> > intel_cpufreq no hwp: good
> > ...
> > periodic workflow at 347 hertz.
> > ~36% load at 4.60 GHz (where hwp operates)
> > ~55% load at 3.2 GHz (where no hwp operates)
> >
> > intel_cpufreq hwp: 9.6 processor package watts. 45.8 watts on the mains to the computer.
> > intel_cpufreq no hwp: ~6 processor package watts. ~41 watts on the mains to the computer. (noisy)
>
> So this time, I only have power/energy data, and a relatively easy way to compress all 12,000
> samples into some concise summary is to simply find the average power for the entire experiment:
>
> Legend:
> hwp: Kernel 5.10-rc6, HWP enabled; intel_cpufreq; schedutil (always)
> rjw: Kernel 5.10-rc6 + this patch set, HWP enabled; intel_cpu-freq; schedutil
> no-hwp: Kernel 5.10-rc6, HWP disabled; intel_cpu-freq; schedutil
> acpi-cpufreq: Kernel 5.10-rc6, HWP disabled; acpi-cpufreq; schedutil
>
> load work/sleep frequency: 73 Hertz:
> hwp: Average: 12.00822 watts
> rjw: Average: 10.18089 watts
> no-hwp: Average: 10.21947 watts
> acpi-cpufreq: Average:  9.06585 watts
>
> load work/sleep frequency: 113 Hertz:
>
> hwp: Average: 12.01056
> rjw: Average: 10.12303
> no-hwp: Average: 10.08228
> acpi-cpufreq: Average:  9.02215
>
> load work/sleep frequency: 211 Hertz:
>
> hwp: Average: 12.16067
> rjw: Average: 10.24413
> no-hwp: Average: 10.12463
> acpi-cpufreq: Average:  9.19175
>
> load work/sleep frequency: 347 Hertz:
>
> hwp: Average: 12.34169
> rjw: Average: 10.79980
> no-hwp: Average: 10.57296
> acpi-cpufreq: Average:  9.84709
>
> load work/sleep frequency: 401 Hertz:
>
> hwp: Average: 12.42562
> rjw: Average: 11.12465
> no-hwp: Average: 11.24203
> acpi-cpufreq: Average: 10.78670
>
> [1] https://marc.info/?l=linux-pm&m=159769839401767&w=2
>
> My tests results graphs:
> Note: I have to code the web site, or I get hammered by bots.
> Note: it is .com only because it was less expensive than .org
> 73 Hertz:
> Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su73/
> 113 Hertz:
> Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su113/
> 211 Hertz:
> Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su211/
> 347 Hertz:
> Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su347/
> 401 Hertz:
> Double u double u double u dot smythies dot .com/~doug/linux/s18/hwp/k510-rc6/su401/

Thanks for the data, this is encouraging!
Peter Zijlstra Dec. 3, 2020, 12:41 p.m. UTC | #3
On Mon, Nov 30, 2020 at 07:37:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> First off, some cpufreq drivers (eg. intel_pstate) can pass hints
> beyond the current target frequency to the hardware and there are no

Everything CPPC, which is quite a bit these days.


> +	/*
> +	 * ->fast_switch() replacement for drivers that use an internal
> +	 * representation of performance levels and can pass hints other than
> +	 * the target performance level to the hardware.
> +	 */
> +	void		(*adjust_perf)(unsigned int cpu, bool busy,
> +				       unsigned long min_perf,
> +				       unsigned long target_perf,
> +				       unsigned long capacity);
>  

I'm not sure @busy makes sense, that's more a hack because @util had a
dip and should remain inside schedutil.


> @@ -454,6 +455,25 @@ static void sugov_update_single(struct u
>  	util = sugov_get_util(sg_cpu);
>  	max = sg_cpu->max;
>  	util = sugov_iowait_apply(sg_cpu, time, util, max);
> +
> +	/*
> +	 * This code runs under rq->lock for the target CPU, so it won't run
> +	 * concurrently on two different CPUs for the same target and it is not
> +	 * necessary to acquire the lock in the fast switch case.
> +	 */
> +	if (sg_policy->direct_fast_switch) {
> +		/*
> +		 * In this case, any optimizations that can be done are up to
> +		 * the driver.
> +		 */
> +		cpufreq_driver_adjust_perf(sg_cpu->cpu,
> +					   sugov_cpu_is_busy(sg_cpu),
> +					   map_util_perf(sg_cpu->bw_dl),
> +					   map_util_perf(util), max);
> +		sg_policy->last_freq_update_time = time;
> +		return;
> +	}

Instead of adding more branches, would it makes sense to simply set a
whole different util_hook in this case?
Rafael J. Wysocki Dec. 3, 2020, 2:44 p.m. UTC | #4
On Thu, Dec 3, 2020 at 1:42 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Nov 30, 2020 at 07:37:01PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > First off, some cpufreq drivers (eg. intel_pstate) can pass hints
> > beyond the current target frequency to the hardware and there are no
>
> Everything CPPC, which is quite a bit these days.

Right, but that is still "some". :-) I can add it to the list of
examples, though.

> > +     /*
> > +      * ->fast_switch() replacement for drivers that use an internal
> > +      * representation of performance levels and can pass hints other than
> > +      * the target performance level to the hardware.
> > +      */
> > +     void            (*adjust_perf)(unsigned int cpu, bool busy,
> > +                                    unsigned long min_perf,
> > +                                    unsigned long target_perf,
> > +                                    unsigned long capacity);
> >
>
> I'm not sure @busy makes sense, that's more a hack because @util had a
> dip and should remain inside schedutil.

So I did it this way, because schedutil would need to store the old
value of target_perf for this and intel_pstate already does that.

But if a new util_hook is used in this case, the existing space in
sg_policy may be used for that.

> > @@ -454,6 +455,25 @@ static void sugov_update_single(struct u
> >       util = sugov_get_util(sg_cpu);
> >       max = sg_cpu->max;
> >       util = sugov_iowait_apply(sg_cpu, time, util, max);
> > +
> > +     /*
> > +      * This code runs under rq->lock for the target CPU, so it won't run
> > +      * concurrently on two different CPUs for the same target and it is not
> > +      * necessary to acquire the lock in the fast switch case.
> > +      */
> > +     if (sg_policy->direct_fast_switch) {
> > +             /*
> > +              * In this case, any optimizations that can be done are up to
> > +              * the driver.
> > +              */
> > +             cpufreq_driver_adjust_perf(sg_cpu->cpu,
> > +                                        sugov_cpu_is_busy(sg_cpu),
> > +                                        map_util_perf(sg_cpu->bw_dl),
> > +                                        map_util_perf(util), max);
> > +             sg_policy->last_freq_update_time = time;
> > +             return;
> > +     }
>
> Instead of adding more branches, would it makes sense to simply set a
> whole different util_hook in this case?

Looks doable without too much code duplication.  Lemme try.
Viresh Kumar Dec. 7, 2020, 7:46 a.m. UTC | #5
On 30-11-20, 19:37, Rafael J. Wysocki wrote:
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -320,6 +320,15 @@ struct cpufreq_driver {
>  					unsigned int index);
>  	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
>  				       unsigned int target_freq);
> +	/*
> +	 * ->fast_switch() replacement for drivers that use an internal
> +	 * representation of performance levels and can pass hints other than
> +	 * the target performance level to the hardware.
> +	 */
> +	void		(*adjust_perf)(unsigned int cpu, bool busy,

Maybe this should still take policy as an argument (like other calls)
instead of CPU, even if it is going to be used for single-cpu per
policy case for now.

> +				       unsigned long min_perf,
> +				       unsigned long target_perf,
> +				       unsigned long capacity);
Rafael J. Wysocki Dec. 7, 2020, 1:40 p.m. UTC | #6
On Mon, Dec 7, 2020 at 8:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 30-11-20, 19:37, Rafael J. Wysocki wrote:
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -320,6 +320,15 @@ struct cpufreq_driver {
> >                                       unsigned int index);
> >       unsigned int    (*fast_switch)(struct cpufreq_policy *policy,
> >                                      unsigned int target_freq);
> > +     /*
> > +      * ->fast_switch() replacement for drivers that use an internal
> > +      * representation of performance levels and can pass hints other than
> > +      * the target performance level to the hardware.
> > +      */
> > +     void            (*adjust_perf)(unsigned int cpu, bool busy,
>
> Maybe this should still take policy as an argument (like other calls)
> instead of CPU, even if it is going to be used for single-cpu per
> policy case for now.

That can be changed in the future if need be.

Otherwise this path doesn't need to look at the policy object at all
and I'd rather keep it this way.

>
> > +                                    unsigned long min_perf,
> > +                                    unsigned long target_perf,
> > +                                    unsigned long capacity);
>
> --
diff mbox series

Patch

Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -320,6 +320,15 @@  struct cpufreq_driver {
 					unsigned int index);
 	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
 				       unsigned int target_freq);
+	/*
+	 * ->fast_switch() replacement for drivers that use an internal
+	 * representation of performance levels and can pass hints other than
+	 * the target performance level to the hardware.
+	 */
+	void		(*adjust_perf)(unsigned int cpu, bool busy,
+				       unsigned long min_perf,
+				       unsigned long target_perf,
+				       unsigned long capacity);
 
 	/*
 	 * Caches and returns the lowest driver-supported frequency greater than
@@ -588,6 +597,11 @@  struct cpufreq_governor {
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq);
+void cpufreq_driver_adjust_perf(unsigned int cpu, bool busy,
+				unsigned long min_perf,
+				unsigned long target_perf,
+				unsigned long capacity);
+bool cpufreq_driver_has_adjust_perf(void);
 int cpufreq_driver_target(struct cpufreq_policy *policy,
 				 unsigned int target_freq,
 				 unsigned int relation);
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2094,6 +2094,47 @@  unsigned int cpufreq_driver_fast_switch(
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
 
+/**
+ * cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
+ * @cpu: Target CPU.
+ * @busy: Whether or not @CPU has been busy since the previous update.
+ * @min_perf: Minimum (required) performance level (units of @capacity).
+ * @target_perf: Terget (desired) performance level (units of @capacity).
+ * @capacity: Capacity of the target CPU.
+ *
+ * Carry out a fast performance level switch of @cpu without sleeping.
+ *
+ * The driver's ->adjust_perf() callback invoked by this function must be
+ * suitable for being called from within RCU-sched read-side critical sections
+ * and it is expected to select a suitable performance level equal to or above
+ * @min_perf and preferably equal to or below @target_perf.
+ *
+ * This function must not be called if policy->fast_switch_enabled is unset.
+ *
+ * Governors calling this function must guarantee that it will never be invoked
+ * twice in parallel for the same CPU and that it will never be called in
+ * parallel with either ->target() or ->target_index() or ->fast_switch() for
+ * the same CPU.
+ */
+void cpufreq_driver_adjust_perf(unsigned int cpu, bool busy,
+				 unsigned long min_perf,
+				 unsigned long target_perf,
+				 unsigned long capacity)
+{
+	cpufreq_driver->adjust_perf(cpu, busy, min_perf, target_perf, capacity);
+}
+
+/**
+ * cpufreq_driver_has_adjust_perf - Check "direct fast switch" callback.
+ *
+ * Return 'true' if the ->adjust_perf callback is present for the
+ * current driver or 'false' otherwise.
+ */
+bool cpufreq_driver_has_adjust_perf(void)
+{
+	return !!cpufreq_driver->adjust_perf;
+}
+
 /* Must set freqs->new to intermediate frequency */
 static int __target_intermediate(struct cpufreq_policy *policy,
 				 struct cpufreq_freqs *freqs, int index)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -40,6 +40,7 @@  struct sugov_policy {
 	struct task_struct	*thread;
 	bool			work_in_progress;
 
+	bool			direct_fast_switch;
 	bool			limits_changed;
 	bool			need_freq_update;
 };
@@ -454,6 +455,25 @@  static void sugov_update_single(struct u
 	util = sugov_get_util(sg_cpu);
 	max = sg_cpu->max;
 	util = sugov_iowait_apply(sg_cpu, time, util, max);
+
+	/*
+	 * This code runs under rq->lock for the target CPU, so it won't run
+	 * concurrently on two different CPUs for the same target and it is not
+	 * necessary to acquire the lock in the fast switch case.
+	 */
+	if (sg_policy->direct_fast_switch) {
+		/*
+		 * In this case, any optimizations that can be done are up to
+		 * the driver.
+		 */
+		cpufreq_driver_adjust_perf(sg_cpu->cpu,
+					   sugov_cpu_is_busy(sg_cpu),
+					   map_util_perf(sg_cpu->bw_dl),
+					   map_util_perf(util), max);
+		sg_policy->last_freq_update_time = time;
+		return;
+	}
+
 	next_f = get_next_freq(sg_policy, util, max);
 	/*
 	 * Do not reduce the frequency if the CPU has not been idle
@@ -466,11 +486,6 @@  static void sugov_update_single(struct u
 		sg_policy->cached_raw_freq = cached_freq;
 	}
 
-	/*
-	 * This code runs under rq->lock for the target CPU, so it won't run
-	 * concurrently on two different CPUs for the same target and it is not
-	 * necessary to acquire the lock in the fast switch case.
-	 */
 	if (sg_policy->policy->fast_switch_enabled) {
 		sugov_fast_switch(sg_policy, time, next_f);
 	} else {
@@ -655,10 +670,6 @@  static int sugov_kthread_create(struct s
 	struct cpufreq_policy *policy = sg_policy->policy;
 	int ret;
 
-	/* kthread only required for slow path */
-	if (policy->fast_switch_enabled)
-		return 0;
-
 	kthread_init_work(&sg_policy->work, sugov_work);
 	kthread_init_worker(&sg_policy->worker);
 	thread = kthread_create(kthread_worker_fn, &sg_policy->worker,
@@ -736,9 +747,14 @@  static int sugov_init(struct cpufreq_pol
 		goto disable_fast_switch;
 	}
 
-	ret = sugov_kthread_create(sg_policy);
-	if (ret)
-		goto free_sg_policy;
+	if (policy->fast_switch_enabled) {
+		sg_policy->direct_fast_switch = cpufreq_driver_has_adjust_perf();
+	} else {
+		/* kthread only required for slow path */
+		ret = sugov_kthread_create(sg_policy);
+		if (ret)
+			goto free_sg_policy;
+	}
 
 	mutex_lock(&global_tunables_lock);
 
Index: linux-pm/include/linux/sched/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/sched/cpufreq.h
+++ linux-pm/include/linux/sched/cpufreq.h
@@ -28,6 +28,11 @@  static inline unsigned long map_util_fre
 {
 	return (freq + (freq >> 2)) * util / cap;
 }
+
+static inline unsigned long map_util_perf(unsigned long util)
+{
+	return util + (util >> 2);
+}
 #endif /* CONFIG_CPU_FREQ */
 
 #endif /* _LINUX_SCHED_CPUFREQ_H */