Fix and cleanup iowait boost

A more energy efficient update of the IO wait boosting mechanism has been introduced in:

commit a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")

where the boost value is expected to be:

- doubled at each successive wakeup from IO staring from the minimum frequency supported by a CPU

- reset when a CPU is not updated for more then one tick by either disabling the IO wait boost or resetting its value to the minimum frequency if this new update requires an IO boost.

This approach is supposed to "ignore" boosting for sporadic wakeups from IO, while still getting the frequency boosted to the maximum to benefit long sequence of wakeup from IO operations.

However, these assumptions are not always satisfied.
For example, when an IO boosted CPU enters idle for more the one tick and then wakes up after an IO wait, since in sugov_set_iowait_boost() we first check the IOWAIT flag, we keep doubling the iowait boost instead of restarting from the minimum frequency value.

This misbehavior could happen mainly on non-shared frequency domains, thus defeating the energy efficiency optimization, but it can also happen on shared frequency domain systems.

Let fix this issue in sugov_set_iowait_boost() by:
- first check the IO wait boost reset conditions to eventually reset the boost value
- then applying the correct IO boost value if required by the caller

Reported-by Viresh Kumar <[email protected]>

Signed-off-by Patrick Bellasi <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Dietmar Eggemann <[email protected]>

Cc [email protected]

Cc [email protected]

Fixes a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")

---
Changes in v3:
- split the fix into a separated patch (this one)
- added "Fixes" tag (Viresh)

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

kernel/sched/cpufreq_schedutil.c +10 -1

index e13df951aca7..c4063e578e4d 100644
@@ -203,6 +203,16 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
 
 static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
 {
+	/* Clear iowait_boost if the CPU apprears to have been idle. */
+	if (sg_cpu->iowait_boost) {
+		s64 delta_ns = time - sg_cpu->last_update;
+
+		if (delta_ns > TICK_NSEC) {
+			sg_cpu->iowait_boost = 0;
+			sg_cpu->iowait_boost_pending = false;
+		}
+	}
+
 	if (flags & SCHED_CPUFREQ_IOWAIT) {
 		if (sg_cpu->iowait_boost_pending)
 			return;

@@ -216,14 +226,6 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned
 		} else {
 			sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
 		}
-	} else if (sg_cpu->iowait_boost) {

A more energy efficient update of the IO wait boosting mechanism has been introduced in:

commit a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")

where the boost value is expected to be:

- doubled at each successive wakeup from IO staring from the minimum frequency supported by a CPU

- reset when a CPU is not updated for more then one tick by either disabling the IO wait boost or resetting its value to the minimum frequency if this new update requires an IO boost.

This approach is supposed to "ignore" boosting for sporadic wakeups from IO, while still getting the frequency boosted to the maximum to benefit long sequence of wakeup from IO operations.

However, these assumptions are not always satisfied.
For example, when an IO boosted CPU enters idle for more the one tick and then wakes up after an IO wait, since in sugov_set_iowait_boost() we first check the IOWAIT flag, we keep doubling the iowait boost instead of restarting from the minimum frequency value.

This misbehavior could happen mainly on non-shared frequency domains, thus defeating the energy efficiency optimization, but it can also happen on shared frequency domain systems.

Let fix this issue in sugov_set_iowait_boost() by:
- first check the IO wait boost reset conditions to eventually reset the boost value
- then applying the correct IO boost value if required by the caller

Reported-by Viresh Kumar <[email protected]>

Signed-off-by Patrick Bellasi <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Dietmar Eggemann <[email protected]>

Cc [email protected]

Cc [email protected]

Fixes a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient")

---
Changes in v3:
- split the fix into a separated patch (this one)
- added "Fixes" tag (Viresh)

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

kernel/sched/cpufreq_schedutil.c +0 -0

index e13df951aca7..c4063e578e4d 100644
@@ -203,6 +203,16 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
 
 static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
 {

kernel/sched/cpufreq_schedutil.c +15 -0

+static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
+			       unsigned int flags)
+{
+	bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
+
+	/* Reset boost if the CPU appears to have been idle enough */
+	if (sg_cpu->iowait_boost &&
+	    sugov_iowait_reset(sg_cpu, time, set_iowait_boost))
+		return;
+
+	/* Boost only tasks waking up after IO */
+	if (!set_iowait_boost)
+		return;
+
+	/* Ensure boost doubles only one time at each request */

kernel/sched/cpufreq_schedutil.c +18 -0

 	}
+
+	/* First wakeup after IO: start with minimum boost */
+	sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
 }
 
 
+static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
+			       unsigned long *util, unsigned long *max)
 {
 	unsigned int boost_util, boost_max;
 
+	/* No boost currently required */
 	if (!sg_cpu->iowait_boost)
 		return;
 
+	/* Reset boost if the CPU appears to have been idle enough */
+	if (sugov_iowait_reset(sg_cpu, time, false))
+		return;
+
+	/*
+	 * An IO waiting task has just woken up:
+	 * allow to further double the boost value
+	 */
 	if (sg_cpu->iowait_boost_pending) {
 		sg_cpu->iowait_boost_pending = false;
 	} else {
+		/*
+		 * Otherwise: reduce the boost value and disable it when we
+		 * reach the minimum.
+		 */
 		sg_cpu->iowait_boost >>= 1;
 		if (sg_cpu->iowait_boost < sg_cpu->sg_policy->policy->min) {
 			sg_cpu->iowait_boost = 0;

Quoted 21 lines · Patrick Bellasi

kernel/sched/cpufreq_schedutil.c +20 -0

+static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
+			       unsigned int flags)
+{
+	bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
+
+	/* Reset boost if the CPU appears to have been idle enough */
+	if (sg_cpu->iowait_boost &&
+	    sugov_iowait_reset(sg_cpu, time, set_iowait_boost))
+		return;
+
+	/* Boost only tasks waking up after IO */
+	if (!set_iowait_boost)
+		return;
+
+	/* Ensure boost doubles only one time at each request */
+	if (sg_cpu->iowait_boost_pending)
+		return;
+	sg_cpu->iowait_boost_pending = true;
+
+	/* Double the boost at each request */

kernel/sched/cpufreq_schedutil.c static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu) · line 268

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +5 -0

+	if (sg_cpu->iowait_boost) {
+		sg_cpu->iowait_boost <<= 1;
+		if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
+			sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
+		return;

Maybe add "else" part of the if block and drop the "return" statement here ?

kernel/sched/cpufreq_schedutil.c static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util, · line 252

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +4 -1

+	/*
+	 * Apply the current boost value: a CPU is boosted only if its current
+	 * utilization is smaller then the current IO boost level.
+	 */
 	boost_util = sg_cpu->iowait_boost;
 	boost_max = sg_cpu->iowait_boost_max;
-

Maybe keep this blank line as is ?

kernel/sched/cpufreq_schedutil.c static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util, · line 333

Quoted 3 lines

kernel/sched/cpufreq_schedutil.c +0 -0

 	if (*util * boost_max < *max * boost_util) {
 		*util = boost_util;
 		*max = boost_max;

Otherwise looks good.

Acked-by Viresh Kumar <[email protected]>

Quoted 22 lines · Viresh Kumar

Quoted 21 lines · Patrick Bellasi

kernel/sched/cpufreq_schedutil.c +20 -0

+static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
+			       unsigned int flags)
+{
+	bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
+
+	/* Reset boost if the CPU appears to have been idle enough */
+	if (sg_cpu->iowait_boost &&
+	    sugov_iowait_reset(sg_cpu, time, set_iowait_boost))
+		return;
+
+	/* Boost only tasks waking up after IO */
+	if (!set_iowait_boost)
+		return;
+
+	/* Ensure boost doubles only one time at each request */
+	if (sg_cpu->iowait_boost_pending)
+		return;
+	sg_cpu->iowait_boost_pending = true;
+
+	/* Double the boost at each request */

Quoted 8 lines

kernel/sched/cpufreq_schedutil.c static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu) · line 268

Quoted 5 lines

kernel/sched/cpufreq_schedutil.c +5 -0

+	if (sg_cpu->iowait_boost) {
+		sg_cpu->iowait_boost <<= 1;
+		if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
+			sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
+		return;

Maybe add "else" part of the if block and drop the "return" statement here ?

If not "mandatory", I would prefer as it is: I'm running with a small stack size in my mind. :)

This "bail out of a function as soon as possible" template allows me to see immediately that, for example in this function, once we decided to double the boost value there is anything more to do here.

At the same time, the statement below it reads as the function default action.

Does it make any sense?

[...]

Quoted 9 lines

kernel/sched/cpufreq_schedutil.c static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util, · line 252

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +4 -1

+	/*
+	 * Apply the current boost value: a CPU is boosted only if its current
+	 * utilization is smaller then the current IO boost level.
+	 */
 	boost_util = sg_cpu->iowait_boost;
 	boost_max = sg_cpu->iowait_boost_max;
-

Maybe keep this blank line as is ?

I've removed it because the above comment now refers to all these lines together.

The iowait boosting code has been recently updated to add a progressive boosting behavior which allows to be less aggressive in boosting tasks doing only sporadic IO operations, thus being more energy efficient for example on mobile platforms.

The current code is now however a bit convoluted. Some functionalities (e.g. iowait boost reset) are replicated in different paths and their documentation is slightly misaligned.

Let's cleanup the code by consolidating all the IO wait boosting related functionality within within few dedicated functions and better define their role:

- sugov_iowait_boost: set/increase the IO wait boost of a CPU
- sugov_iowait_apply: apply/reduce the IO wait boost of a CPU

Both these two function are used at every sugov updated and they makes

use of a unified IO wait boost reset policy provided by:

- sugov_iowait_reset: reset/disable the IO wait boost of a CPU if a CPU is not updated for more then one tick

This makes possible a cleaner and more self-contained design for the IO wait boosting code since the rest of the sugov update routines, both for single and shared frequency domains, follow the same template:

   /* Configure IO boost, if required */
   sugov_iowait_boost()

   /* Return here if freq change is in progress or throttled */

   /* Collect and aggregate utilization information */
   sugov_get_util()
   sugov_aggregate_util()

   /*
    * Add IO boost, if currently enabled, on top of the aggregated
    * utilization value
    */
   sugov_iowait_apply()

As a extra bonus, let's also add the documentation for the new functions and better align the in-code documentation.