cpufreq: schedutil: Make iowait boost more energy efficient

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 <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
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(-)

kernel/sched/cpufreq_schedutil.c +5 -3

index 622eed1b7658..4225bbada88d 100644
@@ -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,

kernel/sched/cpufreq_schedutil.c +0 -1

 	}
 }
 
 static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
 			       unsigned long *max)
 {
-	unsigned long boost_util = sg_cpu->iowait_boost;

kernel/sched/cpufreq_schedutil.c +0 -0

 			continue;
 		}
 		if (j_sg_cpu->flags & SCHED_CPUFREQ_RT_DL)

Signature · 1 line

2.13.2.932.g7449e964c-goog

Quoted 10 lines · Joel Fernandes

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/

Quoted 62 lines

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 <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
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(-)

kernel/sched/cpufreq_schedutil.c +6 -3

index 622eed1b7658..4225bbada88d 100644
@@ -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;
 

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 186

Quoted 6 lines

kernel/sched/cpufreq_schedutil.c +3 -1

              /* 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.

Quoted 15 lines

kernel/sched/cpufreq_schedutil.c +2 -3

      }
 }
 
 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;
 

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 200

Quoted 3 lines

kernel/sched/cpufreq_schedutil.c +3 -0

+     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).

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 209

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +5 -1

      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.

Quoted 8 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 209

Quoted 4 lines · Joel Fernandes

kernel/sched/cpufreq_schedutil.c +4 -0

+     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

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 2 lines

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 ;-)

Quoted 9 lines · Joel Fernandes

Quoted 8 lines · Viresh Kumar

Review

Quoted 4 lines · Joel Fernandes

(quoted diff) +4 -0

+     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/

Quoted 5 lines

Quoted 1 line

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.

Quoted 21 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 11 lines

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.

Mmm, seems to make sense to me. :/

Would the following work (on top of Joel's v5)? Rationale being that only in sugov_set_iowait_boost we might bump freq up (if no iowait_boost was set) or start from policy->min. In sugov_iowait_boost (consumer) instead we do the decay (if no boosting was pending).

---
 kernel/sched/cpufreq_schedutil.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

kernel/sched/cpufreq_schedutil.c +0 -0

index 46b2479641cc..b270563c15a5 100644

Quoted 16 lines · Juri Lelli

Mmm, seems to make sense to me. :/

Would the following work (on top of Joel's v5)? Rationale being that only in sugov_set_iowait_boost we might bump freq up (if no iowait_boost was set) or start from policy->min. In sugov_iowait_boost (consumer) instead we do the decay (if no boosting was pending).

---
 kernel/sched/cpufreq_schedutil.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

kernel/sched/cpufreq_schedutil.c +0 -0

index 46b2479641cc..b270563c15a5 100644

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 177

Quoted 9 lines

kernel/sched/cpufreq_schedutil.c +4 -2

@@ -171,8 +171,14 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
 {
      if (flags & SCHED_CPUFREQ_IOWAIT) {
              sg_cpu->iowait_boost_pending = true;
-             sg_cpu->iowait_boost = max(sg_cpu->iowait_boost,
-                                        sg_cpu->sg_policy->policy->min);
+             if (sg_cpu->iowait_boost) {
+                     /* Bump up 2*current_boost until hitting max */
+                     sg_cpu->iowait_boost = max(sg_cpu->iowait_boost << 1,
+                                                sg_cpu->iowait_boost_max);

And we are back at where we started :)

This wouldn't work because sugov_set_iowait_boost() gets called a lot. Maybe 10 times within a rate_limit_us period.

The thumb rule is, never double/half the boost from sugov_set_iowait_boost() :)

Quoted 9 lines · Joel Fernandes

Quoted 8 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 209

Quoted 4 lines · Joel Fernandes

kernel/sched/cpufreq_schedutil.c +4 -0

+     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/

Quoted 5 lines

Quoted 1 line

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.

Quoted 21 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 11 lines

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().

Not really, to me B will still work because in the case the flag is set, we are correctly double boosting in the next cycle.

Taking an example, with B = flag is set and D = flag is not set

F = Fmin (minimum)

iowait flag       B  B    B    D    D    D
resulting boost   F  2*F  4*F  4*F  2*F  F

What will not work is C but as I mentioned in my last email, that would cause us to delay the iowait boost halving by at most 1 cycle, is that really an issue considering we are starting from min compared to max? Note that cases A. and B. are still working.

Considering the following cases:
(1) min freq is 800MHz, it takes upto 4 cycles to reach 4GHz where the flag is set. At this point I think its likely we will run for many more cycles which means keeping the boost active for 1 extra cycle isn't that big a deal. Even if run for just 5 cycles with boost, that means only the last cycle will suffer from C not decaying as soon as possible. Comparing that to the case where in current code we currently run at max from the first cycle, its not that bad.

(2) we have a transient type of thing, in this case we're probably not reaching the full max immediately so even if we delay the decaying, its still not as bad as what it is currently.

I think considering that the code is way cleaner than any other approach - its a small price to pay. Also keeping in mind that this patch is still an improvement over the current spike, even though as you said its still a bit spikey, but its still better right?

Functionally the code is working and I think is also clean, but if you feel that its still confusing, then I'm open to rewriting it.

Quoted 2 lines

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

Yes, but to fix A. you had to divide by 2 in sugov_set_iowait_boost, and then multiply by 2 later in sugov_iowait_boost to keep the first boost at min. That IMO was confusing so my modified patch did it differently.

Quoted 7 lines · Joel Fernandes

Not really, to me B will still work because in the case the flag is set, we are correctly double boosting in the next cycle.

Taking an example, with B = flag is set and D = flag is not set

F = Fmin (minimum)

Quoted 2 lines

iowait flag       B  B    B    D    D    D
resulting boost   F  2*F  4*F  4*F  2*F  F

What about this ?

iowait flag       B  D    B    D    B    D
resulting boost   F  2*F  F    2*F  F    2*F

Isn't this the worst behavior we may wish for ?

Quoted 18 lines

What will not work is C but as I mentioned in my last email, that would cause us to delay the iowait boost halving by at most 1 cycle, is that really an issue considering we are starting from min compared to max? Note that cases A. and B. are still working.

Considering the following cases:
(1) min freq is 800MHz, it takes upto 4 cycles to reach 4GHz where the flag is set. At this point I think its likely we will run for many more cycles which means keeping the boost active for 1 extra cycle isn't that big a deal. Even if run for just 5 cycles with boost, that means only the last cycle will suffer from C not decaying as soon as possible. Comparing that to the case where in current code we currently run at max from the first cycle, its not that bad.

(2) we have a transient type of thing, in this case we're probably not reaching the full max immediately so even if we delay the decaying, its still not as bad as what it is currently.

Quoted 7 lines

I think considering that the code is way cleaner than any other approach - its a small price to pay. Also keeping in mind that this patch is still an improvement over the current spike, even though as you said its still a bit spikey, but its still better right?

Functionally the code is working and I think is also clean, but if you feel that its still confusing, then I'm open to rewriting it.

I am not worried for being boosted for a bit more time, but with the spikes even when we do not want a freq change.

Quoted 7 lines

Quoted 2 lines

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

Yes, but to fix A. you had to divide by 2 in sugov_set_iowait_boost, and then multiply by 2 later in sugov_iowait_boost to keep the first boost at min. That IMO was confusing so my modified patch did it differently.

Yeah, it wasn't great for sure.

I hope the following one will work for everyone ?

kernel/sched/cpufreq_schedutil.c +2 -1

index 45fcf21ad685..ceac5f72d8da 100644
@@ -53,6 +53,7 @@ struct sugov_cpu {
        struct update_util_data update_util;
        struct sugov_policy *sg_policy;
 
+       bool iowait_boost_pending;
        unsigned long iowait_boost;
        unsigned long iowait_boost_max;
        u64 last_update;

@@ -169,7 +170,17 @@ 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;
+               if (sg_cpu->iowait_boost_pending)

kernel/sched/cpufreq_schedutil.c +3 -3

        } else if (sg_cpu->iowait_boost) {
                s64 delta_ns = time - sg_cpu->last_update;
 

@@ -182,17 +193,23 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
 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;
 
+       if (sg_cpu->iowait_boost_pending)

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Joel Fernandes

Not really, to me B will still work because in the case the flag is set, we are correctly double boosting in the next cycle.

Taking an example, with B = flag is set and D = flag is not set

F = Fmin (minimum)

iowait flag       B  B    B    D    D    D
resulting boost   F  2*F  4*F  4*F  2*F  F

Quoted 4 lines

What about this ?

iowait flag       B  D    B    D    B    D
resulting boost   F  2*F  F    2*F  F    2*F

Yes I guess so but this oscillation can still happen in current code I think.

Quoted 18 lines

kernel/sched/cpufreq_schedutil.c +2 -1

index 45fcf21ad685..ceac5f72d8da 100644
@@ -53,6 +53,7 @@ struct sugov_cpu {
        struct update_util_data update_util;
        struct sugov_policy *sg_policy;
 
+       bool iowait_boost_pending;
        unsigned long iowait_boost;
        unsigned long iowait_boost_max;
        u64 last_update;

@@ -169,7 +170,17 @@ 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;
+               if (sg_cpu->iowait_boost_pending)

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 183

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +10 -0

+                       return;
+
+               sg_cpu->iowait_boost_pending = true;
+
+               if (sg_cpu->iowait_boost) {
+                       sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
+                                                  sg_cpu->iowait_boost_max);
+               } else {
+                       sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
+               }

I would prefer this to be:

      if (sg_cpu->iowait_boost >= policy->min) {
          // double it
      } else {
          // set it to min
      }

This is for the case when boost happens all the way, then its capped at max, but when its decayed back, its not exactly decayed to Fmin but lower than it, so in that case when boost next time we start from Fmin.

Quoted 15 lines

kernel/sched/cpufreq_schedutil.c +2 -3

        } else if (sg_cpu->iowait_boost) {
                s64 delta_ns = time - sg_cpu->last_update;
 

@@ -182,17 +193,23 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
 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;
 

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 211

Quoted 11 lines

kernel/sched/cpufreq_schedutil.c +8 -0

+       if (sg_cpu->iowait_boost_pending)
+               sg_cpu->iowait_boost_pending = false;
+       else
+               sg_cpu->iowait_boost >>= 1;
+
+       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;

This looks good to me and is kind of what I had in mind. I can spend some time testing it soon. Just to be clear if I were to repost this patch after testing, should I have your authorship and my tested-by or do you prefer something else?

Quoted 12 lines · Joel Fernandes

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Joel Fernandes

Not really, to me B will still work because in the case the flag is set, we are correctly double boosting in the next cycle.

Taking an example, with B = flag is set and D = flag is not set

F = Fmin (minimum)

iowait flag       B  B    B    D    D    D
resulting boost   F  2*F  4*F  4*F  2*F  F

Quoted 6 lines

Quoted 4 lines

What about this ?

iowait flag       B  D    B    D    B    D
resulting boost   F  2*F  F    2*F  F    2*F

Yes I guess so but this oscillation can still happen in current code I think.

How ?

Quoted 37 lines

Quoted 18 lines

kernel/sched/cpufreq_schedutil.c +2 -1

index 45fcf21ad685..ceac5f72d8da 100644
@@ -53,6 +53,7 @@ struct sugov_cpu {
        struct update_util_data update_util;
        struct sugov_policy *sg_policy;
 
+       bool iowait_boost_pending;
        unsigned long iowait_boost;
        unsigned long iowait_boost_max;
        u64 last_update;

@@ -169,7 +170,17 @@ 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;
+               if (sg_cpu->iowait_boost_pending)

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 183

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +10 -0

+                       return;
+
+               sg_cpu->iowait_boost_pending = true;
+
+               if (sg_cpu->iowait_boost) {
+                       sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
+                                                  sg_cpu->iowait_boost_max);
+               } else {
+                       sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
+               }

I would prefer this to be:

      if (sg_cpu->iowait_boost >= policy->min) {
          // double it
      } else {
          // set it to min
      }

Quoted 4 lines

This is for the case when boost happens all the way, then its capped at max, but when its decayed back, its not exactly decayed to Fmin but lower than it, so in that case when boost next time we start from Fmin.

Actually you can add another patch first which makes iowait_boost as 0 when it goes below min as that problem exists today as well.

And this patch would be fine then as is ?

Quoted 16 lines

Quoted 16 lines

kernel/sched/cpufreq_schedutil.c +2 -3

        } else if (sg_cpu->iowait_boost) {
                s64 delta_ns = time - sg_cpu->last_update;
 

@@ -182,17 +193,23 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
 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;
 

Quoted 16 lines

kernel/sched/cpufreq_schedutil.c static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, · line 211

Quoted 11 lines

kernel/sched/cpufreq_schedutil.c +8 -0

+       if (sg_cpu->iowait_boost_pending)
+               sg_cpu->iowait_boost_pending = false;
+       else
+               sg_cpu->iowait_boost >>= 1;
+
+       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;

This looks good to me and is kind of what I had in mind. I can spend some time testing it soon. Just to be clear if I were to repost this patch after testing, should I have your authorship and my tested-by or do you prefer something else?

You can keep your authorship I wouldn't mind. Maybe a suggested-by at max would be fine.

Yes I think that's fine, I thought about it some more and I think this can be an issue in a scenario where

iowait_boost_max < policy->min but:

Quoted 4 lines · Joel Fernandes

Yes I think that's fine, I thought about it some more and I think this can be an issue in a scenario where

iowait_boost_max < policy->min but:

We will never have this case as boost-max is set to cpuinfo.max_freq.