Message ID | 2174134.tL5yAn4CWt@kreacher |
---|---|
State | New |
Headers | show |
Series | cpufreq: Allow drivers to receive more information from the governor | expand |
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
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!
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?
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.
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); -- viresh
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); > > --
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 */