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


Groups > linux.kernel > #1688158 > unrolled thread

[PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient

Started byJoel Fernandes <joelaf@google.com>
First post2017-07-16 10:10 +0200
Last post2017-07-18 07:50 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient Joel Fernandes <joelaf@google.com> - 2017-07-16 10:10 +0200
    Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy  efficient Viresh Kumar <viresh.kumar@linaro.org> - 2017-07-17 10:10 +0200
      Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient Joel Fernandes <joelaf@google.com> - 2017-07-17 19:40 +0200
        Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy  efficient Viresh Kumar <viresh.kumar@linaro.org> - 2017-07-18 07:50 +0200

#1688158 — [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient

FromJoel Fernandes <joelaf@google.com>
Date2017-07-16 10:10 +0200
Subject[PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient
Message-ID<u3MMO-4zm-13@gated-at.bofh.it>
Currently the iowait_boost feature in schedutil makes the frequency go to max
on iowait wakeups.  This feature was added to handle a case that Peter
described where the throughput of operations involving continuous I/O requests
[1] is reduced due to running at a lower frequency, however the lower
throughput itself causes utilization to be low and hence causing frequency to
be low hence its "stuck".

Instead of going to max, its also possible to achieve the same effect by
ramping up to max if there are repeated in_iowait wakeups happening. This patch
is an attempt to do that. We start from a lower frequency (policy->mind)
and double the boost for every consecutive iowait update until we reach the
maximum iowait boost frequency (iowait_boost_max).

I ran a synthetic test (continuous O_DIRECT writes in a loop) on an x86 machine
with intel_pstate in passive mode using schedutil. In this test the iowait_boost
value ramped from 800MHz to 4GHz in 60ms. The patch achieves the desired improved
throughput as the existing behavior.

Also while at it, make iowait_boost and iowait_boost_max as unsigned int since
its unit is kHz and this is consistent with struct cpufreq_policy.

[1] https://patchwork.kernel.org/patch/9735885/

Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
This version is based on some ideas from Viresh and Juri in v4.  Viresh, one
difference between the idea we just discussed is, I am scaling up/down the
boost only after consuming it. This has the effect of slightly delaying the
"deboost" but achieves the same boost ramp time. Its more cleaner in the code
IMO to avoid the scaling up and then down on the initial boost. Note that I
also dropped iowait_boost_min and now I'm just starting the initial boost from
policy->min since as I mentioned in the commit above, the ramp of the
iowait_boost value is very quick and for the usecase its intended for, it works
fine. Hope this is acceptable. Thanks.

 kernel/sched/cpufreq_schedutil.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 622eed1b7658..4225bbada88d 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -53,8 +53,9 @@ struct sugov_cpu {
 	struct update_util_data update_util;
 	struct sugov_policy *sg_policy;
 
-	unsigned long iowait_boost;
-	unsigned long iowait_boost_max;
+	bool iowait_boost_pending;
+	unsigned int iowait_boost;
+	unsigned int iowait_boost_max;
 	u64 last_update;
 
 	/* The fields below are only needed when sharing a policy. */
@@ -172,30 +173,43 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
 				   unsigned int flags)
 {
 	if (flags & SCHED_CPUFREQ_IOWAIT) {
-		sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
+		sg_cpu->iowait_boost_pending = true;
+		sg_cpu->iowait_boost = max(sg_cpu->iowait_boost,
+					   sg_cpu->sg_policy->policy->min);
 	} else if (sg_cpu->iowait_boost) {
 		s64 delta_ns = time - sg_cpu->last_update;
 
 		/* Clear iowait_boost if the CPU apprears to have been idle. */
-		if (delta_ns > TICK_NSEC)
+		if (delta_ns > TICK_NSEC) {
 			sg_cpu->iowait_boost = 0;
+			sg_cpu->iowait_boost_pending = false;
+		}
 	}
 }
 
 static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
 			       unsigned long *max)
 {
-	unsigned long boost_util = sg_cpu->iowait_boost;
-	unsigned long boost_max = sg_cpu->iowait_boost_max;
+	unsigned long boost_util, boost_max;
 
-	if (!boost_util)
+	if (!sg_cpu->iowait_boost)
 		return;
 
+	boost_util = sg_cpu->iowait_boost;
+	boost_max = sg_cpu->iowait_boost_max;
+
 	if (*util * boost_max < *max * boost_util) {
 		*util = boost_util;
 		*max = boost_max;
 	}
-	sg_cpu->iowait_boost >>= 1;
+
+	if (sg_cpu->iowait_boost_pending) {
+		sg_cpu->iowait_boost_pending = false;
+		sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
+					   sg_cpu->iowait_boost_max);
+	} else {
+		sg_cpu->iowait_boost >>= 1;
+	}
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
@@ -267,6 +281,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
 		delta_ns = time - j_sg_cpu->last_update;
 		if (delta_ns > TICK_NSEC) {
 			j_sg_cpu->iowait_boost = 0;
+			j_sg_cpu->iowait_boost_pending = false;
 			continue;
 		}
 		if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL)
-- 
2.13.2.932.g7449e964c-goog

[toc] | [next] | [standalone]


#1688744 — Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient

FromViresh Kumar <viresh.kumar@linaro.org>
Date2017-07-17 10:10 +0200
SubjectRe: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient
Message-ID<u49gm-2si-21@gated-at.bofh.it>
In reply to#1688158
On 16-07-17, 01:04, Joel Fernandes wrote:
> Currently the iowait_boost feature in schedutil makes the frequency go to max
> on iowait wakeups.  This feature was added to handle a case that Peter
> described where the throughput of operations involving continuous I/O requests
> [1] is reduced due to running at a lower frequency, however the lower
> throughput itself causes utilization to be low and hence causing frequency to
> be low hence its "stuck".
> 
> Instead of going to max, its also possible to achieve the same effect by
> ramping up to max if there are repeated in_iowait wakeups happening. This patch
> is an attempt to do that. We start from a lower frequency (policy->mind)

s/mind/min/

> and double the boost for every consecutive iowait update until we reach the
> maximum iowait boost frequency (iowait_boost_max).
> 
> I ran a synthetic test (continuous O_DIRECT writes in a loop) on an x86 machine
> with intel_pstate in passive mode using schedutil. In this test the iowait_boost
> value ramped from 800MHz to 4GHz in 60ms. The patch achieves the desired improved
> throughput as the existing behavior.
> 
> Also while at it, make iowait_boost and iowait_boost_max as unsigned int since
> its unit is kHz and this is consistent with struct cpufreq_policy.
> 
> [1] https://patchwork.kernel.org/patch/9735885/
> 
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
> This version is based on some ideas from Viresh and Juri in v4.  Viresh, one
> difference between the idea we just discussed is, I am scaling up/down the
> boost only after consuming it. This has the effect of slightly delaying the
> "deboost" but achieves the same boost ramp time. Its more cleaner in the code
> IMO to avoid the scaling up and then down on the initial boost. Note that I
> also dropped iowait_boost_min and now I'm just starting the initial boost from
> policy->min since as I mentioned in the commit above, the ramp of the
> iowait_boost value is very quick and for the usecase its intended for, it works
> fine. Hope this is acceptable. Thanks.
> 
>  kernel/sched/cpufreq_schedutil.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 622eed1b7658..4225bbada88d 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -53,8 +53,9 @@ struct sugov_cpu {
>  	struct update_util_data update_util;
>  	struct sugov_policy *sg_policy;
>  
> -	unsigned long iowait_boost;
> -	unsigned long iowait_boost_max;
> +	bool iowait_boost_pending;
> +	unsigned int iowait_boost;
> +	unsigned int iowait_boost_max;
>  	u64 last_update;
>  
>  	/* The fields below are only needed when sharing a policy. */
> @@ -172,30 +173,43 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
>  				   unsigned int flags)
>  {
>  	if (flags & SCHED_CPUFREQ_IOWAIT) {
> -		sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> +		sg_cpu->iowait_boost_pending = true;
> +		sg_cpu->iowait_boost = max(sg_cpu->iowait_boost,
> +					   sg_cpu->sg_policy->policy->min);
>  	} else if (sg_cpu->iowait_boost) {
>  		s64 delta_ns = time - sg_cpu->last_update;
>  
>  		/* Clear iowait_boost if the CPU apprears to have been idle. */
> -		if (delta_ns > TICK_NSEC)
> +		if (delta_ns > TICK_NSEC) {
>  			sg_cpu->iowait_boost = 0;
> +			sg_cpu->iowait_boost_pending = false;
> +		}

We don't really need to clear this flag here as we are already making
iowait_boost as 0 and that's what we check while using boost.

>  	}
>  }
>  
>  static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
>  			       unsigned long *max)
>  {
> -	unsigned long boost_util = sg_cpu->iowait_boost;
> -	unsigned long boost_max = sg_cpu->iowait_boost_max;
> +	unsigned long boost_util, boost_max;
>  
> -	if (!boost_util)
> +	if (!sg_cpu->iowait_boost)
>  		return;
>  
> +	boost_util = sg_cpu->iowait_boost;
> +	boost_max = sg_cpu->iowait_boost_max;
> +

The above changes are not required anymore (and were required only
with my patch).

>  	if (*util * boost_max < *max * boost_util) {
>  		*util = boost_util;
>  		*max = boost_max;
>  	}
> -	sg_cpu->iowait_boost >>= 1;
> +
> +	if (sg_cpu->iowait_boost_pending) {
> +		sg_cpu->iowait_boost_pending = false;
> +		sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
> +					   sg_cpu->iowait_boost_max);

Now this has a problem. We will also boost after waiting for
rate_limit_us. And that's why I had proposed the tricky solution in
the first place. I thought we wanted to avoid instant boost only for
the first iteration, but after that we wanted to do it ASAP. Isn't it?

Now that you are using policy->min instead of policy->cur, we can
simplify the solution I proposed and always do 2 * iowait_boost before
getting current util/max in above if loop. i.e. we will start iowait
boost with min * 2 instead of min and that should be fine.

> +	} else {
> +		sg_cpu->iowait_boost >>= 1;
> +	}
>  }
>  
>  #ifdef CONFIG_NO_HZ_COMMON
> @@ -267,6 +281,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
>  		delta_ns = time - j_sg_cpu->last_update;
>  		if (delta_ns > TICK_NSEC) {
>  			j_sg_cpu->iowait_boost = 0;
> +			j_sg_cpu->iowait_boost_pending = false;

Not required here as well.

>  			continue;
>  		}
>  		if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL)
> -- 
> 2.13.2.932.g7449e964c-goog

-- 
viresh

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


#1689298

FromJoel Fernandes <joelaf@google.com>
Date2017-07-17 19:40 +0200
Message-ID<u4i9X-82a-13@gated-at.bofh.it>
In reply to#1688744
Hi Viresh,

On Mon, Jul 17, 2017 at 1:04 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16-07-17, 01:04, Joel Fernandes wrote:
>> Currently the iowait_boost feature in schedutil makes the frequency go to max
>> on iowait wakeups.  This feature was added to handle a case that Peter
>> described where the throughput of operations involving continuous I/O requests
>> [1] is reduced due to running at a lower frequency, however the lower
>> throughput itself causes utilization to be low and hence causing frequency to
>> be low hence its "stuck".
>>
>> Instead of going to max, its also possible to achieve the same effect by
>> ramping up to max if there are repeated in_iowait wakeups happening. This patch
>> is an attempt to do that. We start from a lower frequency (policy->mind)
>
> s/mind/min/
>
>> and double the boost for every consecutive iowait update until we reach the
>> maximum iowait boost frequency (iowait_boost_max).
>>
>> I ran a synthetic test (continuous O_DIRECT writes in a loop) on an x86 machine
>> with intel_pstate in passive mode using schedutil. In this test the iowait_boost
>> value ramped from 800MHz to 4GHz in 60ms. The patch achieves the desired improved
>> throughput as the existing behavior.
>>
>> Also while at it, make iowait_boost and iowait_boost_max as unsigned int since
>> its unit is kHz and this is consistent with struct cpufreq_policy.
>>
>> [1] https://patchwork.kernel.org/patch/9735885/
>>
>> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Suggested-by: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>> ---
>> This version is based on some ideas from Viresh and Juri in v4.  Viresh, one
>> difference between the idea we just discussed is, I am scaling up/down the
>> boost only after consuming it. This has the effect of slightly delaying the
>> "deboost" but achieves the same boost ramp time. Its more cleaner in the code
>> IMO to avoid the scaling up and then down on the initial boost. Note that I
>> also dropped iowait_boost_min and now I'm just starting the initial boost from
>> policy->min since as I mentioned in the commit above, the ramp of the
>> iowait_boost value is very quick and for the usecase its intended for, it works
>> fine. Hope this is acceptable. Thanks.
>>
>>  kernel/sched/cpufreq_schedutil.c | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 622eed1b7658..4225bbada88d 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -53,8 +53,9 @@ struct sugov_cpu {
>>       struct update_util_data update_util;
>>       struct sugov_policy *sg_policy;
>>
>> -     unsigned long iowait_boost;
>> -     unsigned long iowait_boost_max;
>> +     bool iowait_boost_pending;
>> +     unsigned int iowait_boost;
>> +     unsigned int iowait_boost_max;
>>       u64 last_update;
>>
>>       /* The fields below are only needed when sharing a policy. */
>> @@ -172,30 +173,43 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
>>                                  unsigned int flags)
>>  {
>>       if (flags & SCHED_CPUFREQ_IOWAIT) {
>> -             sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
>> +             sg_cpu->iowait_boost_pending = true;
>> +             sg_cpu->iowait_boost = max(sg_cpu->iowait_boost,
>> +                                        sg_cpu->sg_policy->policy->min);
>>       } else if (sg_cpu->iowait_boost) {
>>               s64 delta_ns = time - sg_cpu->last_update;
>>
>>               /* Clear iowait_boost if the CPU apprears to have been idle. */
>> -             if (delta_ns > TICK_NSEC)
>> +             if (delta_ns > TICK_NSEC) {
>>                       sg_cpu->iowait_boost = 0;
>> +                     sg_cpu->iowait_boost_pending = false;
>> +             }
>
> We don't really need to clear this flag here as we are already making
> iowait_boost as 0 and that's what we check while using boost.

Hmm, I would rather clear this flag here and in the loop in
sugov_next_freq_shared since it keeps the flag in sync with what's
happening and is less confusing IMHO.

>
>>       }
>>  }
>>
>>  static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
>>                              unsigned long *max)
>>  {
>> -     unsigned long boost_util = sg_cpu->iowait_boost;
>> -     unsigned long boost_max = sg_cpu->iowait_boost_max;
>> +     unsigned long boost_util, boost_max;
>>
>> -     if (!boost_util)
>> +     if (!sg_cpu->iowait_boost)
>>               return;
>>
>> +     boost_util = sg_cpu->iowait_boost;
>> +     boost_max = sg_cpu->iowait_boost_max;
>> +
>
> The above changes are not required anymore (and were required only
> with my patch).

Yep, I'll drop it.

>>       if (*util * boost_max < *max * boost_util) {
>>               *util = boost_util;
>>               *max = boost_max;
>>       }
>> -     sg_cpu->iowait_boost >>= 1;
>> +
>> +     if (sg_cpu->iowait_boost_pending) {
>> +             sg_cpu->iowait_boost_pending = false;
>> +             sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
>> +                                        sg_cpu->iowait_boost_max);
>
> Now this has a problem. We will also boost after waiting for
> rate_limit_us. And that's why I had proposed the tricky solution in

Not really unless rate_limit_us is < TICK_NSEC? Once TICK_NSEC
elapses, we would clear the boost in sugov_set_iowait_boost and in
sugov_next_freq_shared.

> the first place. I thought we wanted to avoid instant boost only for
> the first iteration, but after that we wanted to do it ASAP. Isn't it?
>
> Now that you are using policy->min instead of policy->cur, we can
> simplify the solution I proposed and always do 2 * iowait_boost before

No, doubling on the first boost was never discussed or intended in my
earlier patches. I thought even your patch never did, you were
dividing by 2, and then scaling it back up by 2 before consuming it to
preserve the initial boost.

> getting current util/max in above if loop. i.e. we will start iowait
> boost with min * 2 instead of min and that should be fine.

Hmm, but why start from double of min? Why not just min? It doesn't
make any difference to the intended behavior itself and is also
consistent with my proposal in RFC v4. Also I feel what you're
suggesting is more spike prone as well, the idea was to start from the
minimum and double it as we go, not to double the min the first go.
That was never intended.

Also I would rather keep the "set and use and set and use" pattern to
keep the logic less confusing and clean IMO.
So we set initial boost in sugov_set_iowait_boost, and then in
sugov_iowait_boost we use it, and then set the boost for the next time
around at the end of sugov_iowait_boost (that is we double it). Next
time sugov_set_iowait_boost wouldn't touch the boost whether iowait
flag is set or not and we would continue into sugov_iowait_boost to
consume the boost. This would have a small delay in reducing the
boost, but that's Ok since its only one cycle of delay, and keeps the
code clean. I assume the last part is not an issue considering you're
proposing double of the initial boost anyway ;-)

thanks,

-Joel

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


#1689732 — Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient

FromViresh Kumar <viresh.kumar@linaro.org>
Date2017-07-18 07:50 +0200
SubjectRe: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient
Message-ID<u4tyq-6Oe-27@gated-at.bofh.it>
In reply to#1689298
On 17-07-17, 10:35, Joel Fernandes wrote:
> On Mon, Jul 17, 2017 at 1:04 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 16-07-17, 01:04, Joel Fernandes wrote:

> >> +     if (sg_cpu->iowait_boost_pending) {
> >> +             sg_cpu->iowait_boost_pending = false;
> >> +             sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
> >> +                                        sg_cpu->iowait_boost_max);
> >
> > Now this has a problem. We will also boost after waiting for

s/also/always/

> > rate_limit_us. And that's why I had proposed the tricky solution in
> 
> Not really unless rate_limit_us is < TICK_NSEC? Once TICK_NSEC
> elapses, we would clear the boost in sugov_set_iowait_boost and in
> sugov_next_freq_shared.

You misread it and I know why it happened. And so I have sent a small
patch to make it a bit more readable.

rate_limit_us is associated with "last_freq_update_time", while
iowait-boost is associated with "last_update".

And last_update gets updated way too often.

> > the first place. I thought we wanted to avoid instant boost only for
> > the first iteration, but after that we wanted to do it ASAP. Isn't it?
> >
> > Now that you are using policy->min instead of policy->cur, we can
> > simplify the solution I proposed and always do 2 * iowait_boost before
> 
> No, doubling on the first boost was never discussed or intended in my
> earlier patches. I thought even your patch never did, you were
> dividing by 2, and then scaling it back up by 2 before consuming it to
> preserve the initial boost.
> 
> > getting current util/max in above if loop. i.e. we will start iowait
> > boost with min * 2 instead of min and that should be fine.
> 
> Hmm, but why start from double of min? Why not just min? It doesn't
> make any difference to the intended behavior itself and is also
> consistent with my proposal in RFC v4. Also I feel what you're
> suggesting is more spike prone as well, the idea was to start from the
> minimum and double it as we go, not to double the min the first go.
> That was never intended.
> 
> Also I would rather keep the "set and use and set and use" pattern to
> keep the logic less confusing and clean IMO.
> So we set initial boost in sugov_set_iowait_boost, and then in
> sugov_iowait_boost we use it, and then set the boost for the next time
> around at the end of sugov_iowait_boost (that is we double it). Next
> time sugov_set_iowait_boost wouldn't touch the boost whether iowait
> flag is set or not and we would continue into sugov_iowait_boost to
> consume the boost. This would have a small delay in reducing the
> boost, but that's Ok since its only one cycle of delay, and keeps the
> code clean. I assume the last part is not an issue considering you're
> proposing double of the initial boost anyway ;-)

Okay, let me try to explain the problem first and then you can propose
a solution if required.

Expected Behavior:

(Window refers to a time window of rate_limit_us here)

A. The first window where IOWAIT flag is set, we set boost to min-freq
   and that shall be used for next freq update in
   sugov_iowait_boost().  Any more calls to sugov_set_iowait_boost()
   within this window shouldn't change the behavior.

B. If the next window also has IOWAIT flag set, then
   sugov_iowait_boost() should use iowait*2 for freq update.

C. If a window doesn't have IOWAIT flag set, then sugov_iowait_boost()
   should use iowait/2 in it.


Do they look fine to you?

Now coming to how will system behave with your patch:

A. would be fine. We will follow things properly.

But B. and C. aren't true anymore.

This happened because after the first window we updated iowait_boost
as 2*min unconditionally and the next window will *always* use that,
even if the flag isn't set. And we may end up increasing the frequency
unnecessarily, i.e. the spike where this discussion started.

And so in my initial solution I reversed the order in
sugov_iowait_boost().

-- 
viresh

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web