[v1,2/4] cpufreq: schedutil: Adjust utilization instead of frequency
diff mbox series

Message ID 1916732.tSaCp9PeQq@kreacher
State New, archived
Headers show
Series
  • cpufreq: Allow drivers to receive more information from the governor
Related show

Commit Message

Rafael J. Wysocki Dec. 7, 2020, 4:29 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When avoiding reduction of the frequency after the target CPU has
been busy since the previous frequency update, adjust the utilization
instead of adjusting the frequency, because doing so is more prudent
(it is done to counter a possible utilization deficit after all) and
it will allow some code to be shared after a subsequent change.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 kernel/sched/cpufreq_schedutil.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Viresh Kumar Dec. 8, 2020, 8:51 a.m. UTC | #1
On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When avoiding reduction of the frequency after the target CPU has
> been busy since the previous frequency update, adjust the utilization
> instead of adjusting the frequency, because doing so is more prudent
> (it is done to counter a possible utilization deficit after all) and
> it will allow some code to be shared after a subsequent change.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  kernel/sched/cpufreq_schedutil.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -437,7 +437,7 @@ static void sugov_update_single(struct u
>  {
>  	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
>  	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> -	unsigned int cached_freq = sg_policy->cached_raw_freq;
> +	unsigned long prev_util = sg_cpu->util;
>  	unsigned int next_f;
>  
>  	sugov_iowait_boost(sg_cpu, time, flags);
> @@ -451,17 +451,14 @@ static void sugov_update_single(struct u
>  	sugov_get_util(sg_cpu);
>  	sugov_iowait_apply(sg_cpu, time);
>  
> -	next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
>  	/*
>  	 * Do not reduce the frequency if the CPU has not been idle
>  	 * recently, as the reduction is likely to be premature then.
>  	 */
> -	if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> -		next_f = sg_policy->next_freq;
> +	if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
> +		sg_cpu->util = prev_util;
>  
> -		/* Restore cached freq as next_freq has changed */
> -		sg_policy->cached_raw_freq = cached_freq;
> -	}
> +	next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);

I don't think we can replace freq comparison by util, or at least it will give
us a different final frequency and the behavior is changed.

Lets take an example, lets say current freq is 1 GHz and max is 1024.

Round 1: Lets say util is 1000

next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz

Round 2: Lets say util has come down to 900 here,

before the patch:

next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz

after the patch:

next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz

Or did I make a mistake here ?
Rafael J. Wysocki Dec. 8, 2020, 5:01 p.m. UTC | #2
On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > When avoiding reduction of the frequency after the target CPU has
> > been busy since the previous frequency update, adjust the utilization
> > instead of adjusting the frequency, because doing so is more prudent
> > (it is done to counter a possible utilization deficit after all) and
> > it will allow some code to be shared after a subsequent change.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  kernel/sched/cpufreq_schedutil.c |   11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > @@ -437,7 +437,7 @@ static void sugov_update_single(struct u
> >  {
> >       struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> >       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > -     unsigned int cached_freq = sg_policy->cached_raw_freq;
> > +     unsigned long prev_util = sg_cpu->util;
> >       unsigned int next_f;
> >
> >       sugov_iowait_boost(sg_cpu, time, flags);
> > @@ -451,17 +451,14 @@ static void sugov_update_single(struct u
> >       sugov_get_util(sg_cpu);
> >       sugov_iowait_apply(sg_cpu, time);
> >
> > -     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> >       /*
> >        * Do not reduce the frequency if the CPU has not been idle
> >        * recently, as the reduction is likely to be premature then.
> >        */
> > -     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > -             next_f = sg_policy->next_freq;
> > +     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
> > +             sg_cpu->util = prev_util;
> >
> > -             /* Restore cached freq as next_freq has changed */
> > -             sg_policy->cached_raw_freq = cached_freq;
> > -     }
> > +     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
>
> I don't think we can replace freq comparison by util, or at least it will give
> us a different final frequency and the behavior is changed.
>
> Lets take an example, lets say current freq is 1 GHz and max is 1024.
>
> Round 1: Lets say util is 1000
>
> next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz
>
> Round 2: Lets say util has come down to 900 here,
>
> before the patch:
>
> next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz
>
> after the patch:
>
> next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz
>
> Or did I make a mistake here ?

I think so, if my understanding is correct.

Without the patch, next_f will be reset to the previous value
(sq_policy->next_freq) if the CPU has been busy and the (new) next_f
is less than that value.

So the "new" next_f before the patch is 1.31 GHz, but because it is
less than the previous value (1.45 GHz), it will be reset to that
value, unless I'm missing something.

Overall, the patch doesn't change the logic AFAICS and because the
util->freq mapping is linear, all of the inequalities map exactly from
one to the other (both ways).
Viresh Kumar Dec. 9, 2020, 5:16 a.m. UTC | #3
On 08-12-20, 18:01, Rafael J. Wysocki wrote:
> On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > When avoiding reduction of the frequency after the target CPU has
> > > been busy since the previous frequency update, adjust the utilization
> > > instead of adjusting the frequency, because doing so is more prudent
> > > (it is done to counter a possible utilization deficit after all) and
> > > it will allow some code to be shared after a subsequent change.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  kernel/sched/cpufreq_schedutil.c |   11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > @@ -437,7 +437,7 @@ static void sugov_update_single(struct u
> > >  {
> > >       struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> > >       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > > -     unsigned int cached_freq = sg_policy->cached_raw_freq;
> > > +     unsigned long prev_util = sg_cpu->util;
> > >       unsigned int next_f;
> > >
> > >       sugov_iowait_boost(sg_cpu, time, flags);
> > > @@ -451,17 +451,14 @@ static void sugov_update_single(struct u
> > >       sugov_get_util(sg_cpu);
> > >       sugov_iowait_apply(sg_cpu, time);
> > >
> > > -     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> > >       /*
> > >        * Do not reduce the frequency if the CPU has not been idle
> > >        * recently, as the reduction is likely to be premature then.
> > >        */
> > > -     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > -             next_f = sg_policy->next_freq;
> > > +     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
> > > +             sg_cpu->util = prev_util;
> > >
> > > -             /* Restore cached freq as next_freq has changed */
> > > -             sg_policy->cached_raw_freq = cached_freq;
> > > -     }
> > > +     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> >
> > I don't think we can replace freq comparison by util, or at least it will give
> > us a different final frequency and the behavior is changed.
> >
> > Lets take an example, lets say current freq is 1 GHz and max is 1024.
> >
> > Round 1: Lets say util is 1000
> >
> > next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz
> >
> > Round 2: Lets say util has come down to 900 here,
> >
> > before the patch:
> >
> > next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz
> >
> > after the patch:
> >
> > next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz
> >
> > Or did I make a mistake here ?
> 
> I think so, if my understanding is correct.
> 
> Without the patch, next_f will be reset to the previous value
> (sq_policy->next_freq) if the CPU has been busy and the (new) next_f
> is less than that value.
> 
> So the "new" next_f before the patch is 1.31 GHz, but because it is
> less than the previous value (1.45 GHz), it will be reset to that
> value, unless I'm missing something.

The prev frequency here was 1.2 GHz (after Round 1). 1.45 GHz is the
value we get after this patch, as we take the earlier utilization
(1000) into account instead of 900.

> Overall, the patch doesn't change the logic AFAICS and because the
> util->freq mapping is linear, all of the inequalities map exactly from
> one to the other (both ways).
Rafael J. Wysocki Dec. 9, 2020, 3:32 p.m. UTC | #4
On Wed, Dec 9, 2020 at 6:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-12-20, 18:01, Rafael J. Wysocki wrote:
> > On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > When avoiding reduction of the frequency after the target CPU has
> > > > been busy since the previous frequency update, adjust the utilization
> > > > instead of adjusting the frequency, because doing so is more prudent
> > > > (it is done to counter a possible utilization deficit after all) and
> > > > it will allow some code to be shared after a subsequent change.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  kernel/sched/cpufreq_schedutil.c |   11 ++++-------
> > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > >
> > > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > > ===================================================================
> > > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > > @@ -437,7 +437,7 @@ static void sugov_update_single(struct u
> > > >  {
> > > >       struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> > > >       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > > > -     unsigned int cached_freq = sg_policy->cached_raw_freq;
> > > > +     unsigned long prev_util = sg_cpu->util;
> > > >       unsigned int next_f;
> > > >
> > > >       sugov_iowait_boost(sg_cpu, time, flags);
> > > > @@ -451,17 +451,14 @@ static void sugov_update_single(struct u
> > > >       sugov_get_util(sg_cpu);
> > > >       sugov_iowait_apply(sg_cpu, time);
> > > >
> > > > -     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> > > >       /*
> > > >        * Do not reduce the frequency if the CPU has not been idle
> > > >        * recently, as the reduction is likely to be premature then.
> > > >        */
> > > > -     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > > -             next_f = sg_policy->next_freq;
> > > > +     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
> > > > +             sg_cpu->util = prev_util;
> > > >
> > > > -             /* Restore cached freq as next_freq has changed */
> > > > -             sg_policy->cached_raw_freq = cached_freq;
> > > > -     }
> > > > +     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> > >
> > > I don't think we can replace freq comparison by util, or at least it will give
> > > us a different final frequency and the behavior is changed.
> > >
> > > Lets take an example, lets say current freq is 1 GHz and max is 1024.

Ah, so that's in the freq-dependent case.

In the freq-invariant case next_f doesn't depend on the current frequency.

> > > Round 1: Lets say util is 1000
> > >
> > > next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz
> > >
> > > Round 2: Lets say util has come down to 900 here,
> > >
> > > before the patch:
> > >
> > > next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz
> > >
> > > after the patch:
> > >
> > > next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz
> > >
> > > Or did I make a mistake here ?
> >
> > I think so, if my understanding is correct.
> >
> > Without the patch, next_f will be reset to the previous value
> > (sq_policy->next_freq) if the CPU has been busy and the (new) next_f
> > is less than that value.
> >
> > So the "new" next_f before the patch is 1.31 GHz, but because it is
> > less than the previous value (1.45 GHz), it will be reset to that
> > value, unless I'm missing something.
>
> The prev frequency here was 1.2 GHz (after Round 1). 1.45 GHz is the
> value we get after this patch, as we take the earlier utilization
> (1000) into account instead of 900.

So I have misunderstood your example.

In the non-invariant case (which is or shortly will be relevant for
everybody interested) cpuinfo.max_freq goes into the calculation
instead of the current frequency and the mapping between util and freq
is linear.  In the freq-dependent case it is not linear, of course.

So I guess the concern is that this changes the behavior in the
freq-dependent case which may not be desirable.

Fair enough, but I'm not sure if that is enough of a reason to avoid
sharing the code between the "perf" and "freq" paths.
Viresh Kumar Dec. 14, 2020, 11:07 a.m. UTC | #5
On 09-12-20, 16:32, Rafael J. Wysocki wrote:
> So I have misunderstood your example.
> 
> In the non-invariant case (which is or shortly will be relevant for
> everybody interested) cpuinfo.max_freq goes into the calculation
> instead of the current frequency and the mapping between util and freq
> is linear.  In the freq-dependent case it is not linear, of course.
> 
> So I guess the concern is that this changes the behavior in the
> freq-dependent case which may not be desirable.

Right and we end up increasing the frequency here..

> Fair enough, but I'm not sure if that is enough of a reason to avoid
> sharing the code between the "perf" and "freq" paths.

Sure, I am not against sharing the code path, but all we need is
something like this here:

     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
             sg_cpu->util = prev_util;
     else
             next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);

i.e. we don't need to call get_next_freq() in this case at all.

Patch
diff mbox series

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -437,7 +437,7 @@  static void sugov_update_single(struct u
 {
 	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
 	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
-	unsigned int cached_freq = sg_policy->cached_raw_freq;
+	unsigned long prev_util = sg_cpu->util;
 	unsigned int next_f;
 
 	sugov_iowait_boost(sg_cpu, time, flags);
@@ -451,17 +451,14 @@  static void sugov_update_single(struct u
 	sugov_get_util(sg_cpu);
 	sugov_iowait_apply(sg_cpu, time);
 
-	next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
 	/*
 	 * Do not reduce the frequency if the CPU has not been idle
 	 * recently, as the reduction is likely to be premature then.
 	 */
-	if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
-		next_f = sg_policy->next_freq;
+	if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
+		sg_cpu->util = prev_util;
 
-		/* Restore cached freq as next_freq has changed */
-		sg_policy->cached_raw_freq = cached_freq;
-	}
+	next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
 
 	/*
 	 * This code runs under rq->lock for the target CPU, so it won't run