diff mbox series

[v3,3/4] cpufreq: schedutil: Simplify iowait boosting

Message ID 2864116.v8IU97Resm@aspire.rjw.lan (mailing list archive)
State Superseded, archived
Headers show
Series cpufreq: intel_pstate: Handle _PPC updates on global turbo disable/enable | expand

Commit Message

Rafael J. Wysocki March 26, 2019, 11:18 a.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is not reason for the minimum iowait boost value in the
schedutil cpufreq governor to depend on the available range of CPU
frequencies.  In fact, that dependency is generally confusing,
because it causes the iowait boost to behave somewhat differently
on CPUs with the same maximum frequency and different minimum
frequencies, for example.

For this reason, replace the min field in struct sugov_cpu
with a constant and choose its values to be 1/8 of
SCHED_CAPACITY_SCALE (for consistency with the intel_pstate
driver's internal governor).

[Note that policy->cpuinfo.max_freq will not be a constant any more
 after a subsequent change, so this change is depended on by it.]

Link: https://lore.kernel.org/lkml/20190305083202.GU32494@hirez.programming.kicks-ass.net/T/#ee20bdc98b7d89f6110c0d00e5c3ee8c2ced93c3d
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 kernel/sched/cpufreq_schedutil.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Quentin Perret March 28, 2019, 9:48 a.m. UTC | #1 
Hi Rafael,

On Tuesday 26 Mar 2019 at 12:18:00 (+0100), Rafael J. Wysocki wrote:
> @@ -13,6 +13,8 @@
>  #include <linux/sched/cpufreq.h>
>  #include <trace/events/power.h>
>  
> +#define IOWAIT_BOOST_MIN	(SCHED_CAPACITY_SCALE / 8)
> +
>  struct sugov_tunables {
>  	struct gov_attr_set	attr_set;
>  	unsigned int		rate_limit_us;
> @@ -51,7 +53,6 @@ struct sugov_cpu {
>  	u64			last_update;
>  
>  	unsigned long		bw_dl;
> -	unsigned long		min;
>  	unsigned long		max;
>  
>  	/* The field below is for single-CPU policies only: */
> @@ -303,7 +304,7 @@ static bool sugov_iowait_reset(struct sua

The comment above this function needs updating I think.

>  	if (delta_ns <= TICK_NSEC)
>  		return false;
>  
> -	sg_cpu->iowait_boost = set_iowait_boost ? sg_cpu->min : 0;
> +	sg_cpu->iowait_boost = set_iowait_boost ? IOWAIT_BOOST_MIN : 0;
>  	sg_cpu->iowait_boost_pending = set_iowait_boost;
>  
>  	return true;
> @@ -349,7 +350,7 @@ static void sugov_iowait_boost(struct su

Ditto.

>  	}
>  
>  	/* First wakeup after IO: start with minimum boost */
> -	sg_cpu->iowait_boost = sg_cpu->min;
> +	sg_cpu->iowait_boost = IOWAIT_BOOST_MIN;
>  }
>  
>  /**
> @@ -389,7 +390,7 @@ static unsigned long sugov_iowait_apply(
>  		 * No boost pending; reduce the boost value.
>  		 */
>  		sg_cpu->iowait_boost >>= 1;
> -		if (sg_cpu->iowait_boost < sg_cpu->min) {
> +		if (sg_cpu->iowait_boost < IOWAIT_BOOST_MIN) {
>  			sg_cpu->iowait_boost = 0;
>  			return util;
>  		}
> @@ -826,9 +827,6 @@ static int sugov_start(struct cpufreq_po
>  		memset(sg_cpu, 0, sizeof(*sg_cpu));
>  		sg_cpu->cpu			= cpu;
>  		sg_cpu->sg_policy		= sg_policy;
> -		sg_cpu->min			=
> -			(SCHED_CAPACITY_SCALE * policy->cpuinfo.min_freq) /
> -			policy->cpuinfo.max_freq;
>  	}
>  
>  	for_each_cpu(cpu, policy->cpus) {
> 

Other than that, I tried a backport of this on a Pixel 3 with Snapdragon
845 (which is relevant because it has tons of OPPs, so starting at 128
makes it ramp up faster) to check the impact on power, but the only
differences appeared to be in the noise margin, so it's all good :)

Full test results available at [1]. Note that I did enable the iowait
boost feature for these tests -- it is disabled by default on P3 ...

Thanks,
Quentin

---
[1] https://nbviewer.jupyter.org/gist/qperret/69c9bde13aad2d783689e78c9ba2d9bc
Rafael J. Wysocki March 28, 2019, 10:24 a.m. UTC | #2 
On Thu, Mar 28, 2019 at 10:48 AM Quentin Perret <quentin.perret@arm.com> wrote:
>
> Hi Rafael,
>
> On Tuesday 26 Mar 2019 at 12:18:00 (+0100), Rafael J. Wysocki wrote:
> > @@ -13,6 +13,8 @@
> >  #include <linux/sched/cpufreq.h>
> >  #include <trace/events/power.h>
> >
> > +#define IOWAIT_BOOST_MIN     (SCHED_CAPACITY_SCALE / 8)
> > +
> >  struct sugov_tunables {
> >       struct gov_attr_set     attr_set;
> >       unsigned int            rate_limit_us;
> > @@ -51,7 +53,6 @@ struct sugov_cpu {
> >       u64                     last_update;
> >
> >       unsigned long           bw_dl;
> > -     unsigned long           min;
> >       unsigned long           max;
> >
> >       /* The field below is for single-CPU policies only: */
> > @@ -303,7 +304,7 @@ static bool sugov_iowait_reset(struct sua
>
> The comment above this function needs updating I think.
>
> >       if (delta_ns <= TICK_NSEC)
> >               return false;
> >
> > -     sg_cpu->iowait_boost = set_iowait_boost ? sg_cpu->min : 0;
> > +     sg_cpu->iowait_boost = set_iowait_boost ? IOWAIT_BOOST_MIN : 0;
> >       sg_cpu->iowait_boost_pending = set_iowait_boost;
> >
> >       return true;
> > @@ -349,7 +350,7 @@ static void sugov_iowait_boost(struct su
>
> Ditto.

I overlooked these two, thanks for pointing that out!

Will send an update momentarily.

> >       }
> >
> >       /* First wakeup after IO: start with minimum boost */
> > -     sg_cpu->iowait_boost = sg_cpu->min;
> > +     sg_cpu->iowait_boost = IOWAIT_BOOST_MIN;
> >  }
> >
> >  /**
> > @@ -389,7 +390,7 @@ static unsigned long sugov_iowait_apply(
> >                * No boost pending; reduce the boost value.
> >                */
> >               sg_cpu->iowait_boost >>= 1;
> > -             if (sg_cpu->iowait_boost < sg_cpu->min) {
> > +             if (sg_cpu->iowait_boost < IOWAIT_BOOST_MIN) {
> >                       sg_cpu->iowait_boost = 0;
> >                       return util;
> >               }
> > @@ -826,9 +827,6 @@ static int sugov_start(struct cpufreq_po
> >               memset(sg_cpu, 0, sizeof(*sg_cpu));
> >               sg_cpu->cpu                     = cpu;
> >               sg_cpu->sg_policy               = sg_policy;
> > -             sg_cpu->min                     =
> > -                     (SCHED_CAPACITY_SCALE * policy->cpuinfo.min_freq) /
> > -                     policy->cpuinfo.max_freq;
> >       }
> >
> >       for_each_cpu(cpu, policy->cpus) {
> >
>
> Other than that, I tried a backport of this on a Pixel 3 with Snapdragon
> 845 (which is relevant because it has tons of OPPs, so starting at 128
> makes it ramp up faster) to check the impact on power, but the only
> differences appeared to be in the noise margin, so it's all good :)

Cool. :-)

> Full test results available at [1]. Note that I did enable the iowait
> boost feature for these tests -- it is disabled by default on P3 ...

Thanks!

> ---
> [1] https://nbviewer.jupyter.org/gist/qperret/69c9bde13aad2d783689e78c9ba2d9bc
diff mbox series

Patch

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -13,6 +13,8 @@ 
 #include <linux/sched/cpufreq.h>
 #include <trace/events/power.h>
 
+#define IOWAIT_BOOST_MIN	(SCHED_CAPACITY_SCALE / 8)
+
 struct sugov_tunables {
 	struct gov_attr_set	attr_set;
 	unsigned int		rate_limit_us;
@@ -51,7 +53,6 @@  struct sugov_cpu {
 	u64			last_update;
 
 	unsigned long		bw_dl;
-	unsigned long		min;
 	unsigned long		max;
 
 	/* The field below is for single-CPU policies only: */
@@ -303,7 +304,7 @@  static bool sugov_iowait_reset(struct su
 	if (delta_ns <= TICK_NSEC)
 		return false;
 
-	sg_cpu->iowait_boost = set_iowait_boost ? sg_cpu->min : 0;
+	sg_cpu->iowait_boost = set_iowait_boost ? IOWAIT_BOOST_MIN : 0;
 	sg_cpu->iowait_boost_pending = set_iowait_boost;
 
 	return true;
@@ -349,7 +350,7 @@  static void sugov_iowait_boost(struct su
 	}
 
 	/* First wakeup after IO: start with minimum boost */
-	sg_cpu->iowait_boost = sg_cpu->min;
+	sg_cpu->iowait_boost = IOWAIT_BOOST_MIN;
 }
 
 /**
@@ -389,7 +390,7 @@  static unsigned long sugov_iowait_apply(
 		 * No boost pending; reduce the boost value.
 		 */
 		sg_cpu->iowait_boost >>= 1;
-		if (sg_cpu->iowait_boost < sg_cpu->min) {
+		if (sg_cpu->iowait_boost < IOWAIT_BOOST_MIN) {
 			sg_cpu->iowait_boost = 0;
 			return util;
 		}
@@ -826,9 +827,6 @@  static int sugov_start(struct cpufreq_po
 		memset(sg_cpu, 0, sizeof(*sg_cpu));
 		sg_cpu->cpu			= cpu;
 		sg_cpu->sg_policy		= sg_policy;
-		sg_cpu->min			=
-			(SCHED_CAPACITY_SCALE * policy->cpuinfo.min_freq) /
-			policy->cpuinfo.max_freq;
 	}
 
 	for_each_cpu(cpu, policy->cpus) {