Here is an update of:
https://lkml.org/lkml/2018/4/10/642
[email protected]
which is now a series since I've split the fix from the refactoring/cleanup
patch, as suggested by Viresh.
Cheers Patrick
Changes in v3:
- renamed the iowait boost functions (Peter)
- moved boost reset into a dedicated function (Peter)
- split the fix into a separated patch (Viresh)
- added "Fixes" tag (Viresh)
Patrick Bellasi (2):
cpufreq: schedutil: Fix iowait boost reset
cpufreq: schedutil: Cleanup and document iowait boost
kernel/sched/cpufreq_schedutil.c | 152 +++++++++++++++++++++++++++------------
1 file changed, 108 insertions(+), 44 deletions(-)
--
2.15.1
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.
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]
---
Changes in v3:
- renamed the iowait boost functions (Peter)
- moved boost reset into a dedicated function (Peter)
- split the fix into a separated patch (Viresh)
---
kernel/sched/cpufreq_schedutil.c | 152 +++++++++++++++++++++++++++------------
1 file changed, 107 insertions(+), 45 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index c4063e578e4d..1a50e1e95f4b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -51,7 +51,7 @@ struct sugov_cpu {
bool iowait_boost_pending;
unsigned int iowait_boost;
unsigned int iowait_boost_max;
- u64 last_update;
+ u64 last_update;
/* The fields below are only needed when sharing a policy: */
unsigned long util_cfs;
@@ -201,45 +201,120 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
return min(util, sg_cpu->max);
}
-static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
+/**
+ * sugov_iowait_reset() - Reset the IO boost status of a CPU.
+ * @sg_cpu: the sugov data for the CPU to boost
+ * @time: the update time from the caller
+ * @set_iowait_boost: true if an IO boost has been requested
+ *
+ * The IO wait boost of a task is disabled after a tick since the last update
+ * of a CPU. If a new IO wait boost is requested after more then a tick, then
+ * we enable the boost starting from the minimum frequency, which improves
+ * energy efficiency by ignoring sporadic wakeups from IO.
+ */
+static bool sugov_iowait_reset(struct sugov_cpu *sg_cpu, u64 time,
+ bool set_iowait_boost)
{
- /* Clear iowait_boost if the CPU apprears to have been idle. */
- if (sg_cpu->iowait_boost) {
- s64 delta_ns = time - sg_cpu->last_update;
+ s64 delta_ns = time - sg_cpu->last_update;
- if (delta_ns > TICK_NSEC) {
- sg_cpu->iowait_boost = 0;
- sg_cpu->iowait_boost_pending = false;
- }
- }
+ /* Reset boost only if a tick has elapsed since last request */
+ if (delta_ns <= TICK_NSEC)
+ return false;
- if (flags & SCHED_CPUFREQ_IOWAIT) {
- if (sg_cpu->iowait_boost_pending)
- return;
+ sg_cpu->iowait_boost = set_iowait_boost
+ ? sg_cpu->sg_policy->policy->min : 0;
+ sg_cpu->iowait_boost_pending = set_iowait_boost;
- sg_cpu->iowait_boost_pending = true;
+ return true;
+}
- 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;
- } else {
- sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
- }
+/**
+ * sugov_iowait_boost() - Updates the IO boost status of a CPU.
+ * @sg_cpu: the sugov data for the CPU to boost
+ * @time: the update time from the caller
+ * @flags: SCHED_CPUFREQ_IOWAIT if the task is waking up after an IO wait
+ *
+ * Each time a task wakes up after an IO operation, the CPU utilization can be
+ * boosted to a certain utilization which doubles at each "frequent and
+ * successive" wakeup from IO, ranging from the utilization of the minimum
+ * OPP to the utilization of the maximum OPP.
+ * To keep doubling, an IO boost has to be requested at least once per tick,
+ * otherwise we restart from the utilization of the minimum OPP.
+ */
+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 */
+ 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;
}
+
+ /* First wakeup after IO: start with minimum boost */
+ sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
}
-static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
- unsigned long *max)
+/**
+ * sugov_iowait_apply() - Apply the IO boost to a CPU.
+ * @sg_cpu: the sugov data for the cpu to boost
+ * @time: the update time from the caller
+ * @util: the utilization to (eventually) boost
+ * @max: the maximum value the utilization can be boosted to
+ *
+ * A CPU running a task which woken up after an IO operation can have its
+ * utilization boosted to speed up the completion of those IO operations.
+ * The IO boost value is increased each time a task wakes up from IO, in
+ * sugov_iowait_apply(), and it's instead decreased by this function,
+ * each time an increase has not been requested (!iowait_boost_pending).
+ *
+ * A CPU which also appears to have been idle for at least one tick has also
+ * its IO boost utilization reset.
+ *
+ * This mechanism is designed to boost high frequently IO waiting tasks, while
+ * being more conservative on tasks which does sporadic IO operations.
+ */
+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;
@@ -247,9 +322,12 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
}
}
+ /*
+ * 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;
-
if (*util * boost_max < *max * boost_util) {
*util = boost_util;
*max = boost_max;
@@ -288,7 +366,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
unsigned int next_f;
bool busy;
- sugov_set_iowait_boost(sg_cpu, time, flags);
+ sugov_iowait_boost(sg_cpu, time, flags);
sg_cpu->last_update = time;
ignore_dl_rate_limit(sg_cpu, sg_policy);
@@ -301,7 +379,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
sugov_get_util(sg_cpu);
max = sg_cpu->max;
util = sugov_aggregate_util(sg_cpu);
- sugov_iowait_boost(sg_cpu, &util, &max);
+ sugov_iowait_apply(sg_cpu, time, &util, &max);
next_f = get_next_freq(sg_policy, util, max);
/*
* Do not reduce the frequency if the CPU has not been idle
@@ -328,28 +406,12 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
for_each_cpu(j, policy->cpus) {
struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j);
unsigned long j_util, j_max;
- s64 delta_ns;
sugov_get_util(j_sg_cpu);
-
- /*
- * If the CFS CPU utilization was last updated before the
- * previous frequency update and the time elapsed between the
- * last update of the CPU utilization and the last frequency
- * update is long enough, reset iowait_boost and util_cfs, as
- * they are now probably stale. However, still consider the
- * CPU contribution if it has some DEADLINE utilization
- * (util_dl).
- */
- 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;
- }
-
j_max = j_sg_cpu->max;
j_util = sugov_aggregate_util(j_sg_cpu);
- sugov_iowait_boost(j_sg_cpu, &j_util, &j_max);
+ sugov_iowait_apply(j_sg_cpu, time, &j_util, &j_max);
+
if (j_util * max > j_max * util) {
util = j_util;
max = j_max;
@@ -368,7 +430,7 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags)
raw_spin_lock(&sg_policy->update_lock);
- sugov_set_iowait_boost(sg_cpu, time, flags);
+ sugov_iowait_boost(sg_cpu, time, flags);
sg_cpu->last_update = time;
ignore_dl_rate_limit(sg_cpu, sg_policy);
--
2.15.1
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(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df951aca7..c4063e578e4d 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -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) {
- s64 delta_ns = time - sg_cpu->last_update;
-
- /* Clear iowait_boost if the CPU apprears to have been idle. */
- if (delta_ns > TICK_NSEC) {
- sg_cpu->iowait_boost = 0;
- sg_cpu->iowait_boost_pending = false;
- }
}
}
--
2.15.1
On 21-05-18, 09:51, Patrick Bellasi wrote:
> 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(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e13df951aca7..c4063e578e4d 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -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) {
> - s64 delta_ns = time - sg_cpu->last_update;
> -
> - /* Clear iowait_boost if the CPU apprears to have been idle. */
> - if (delta_ns > TICK_NSEC) {
> - sg_cpu->iowait_boost = 0;
> - sg_cpu->iowait_boost_pending = false;
> - }
> }
> }
>
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 21-05-18, 09:51, Patrick Bellasi wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> +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 */
> + 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 ?
> }
> +
> + /* 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;
> @@ -247,9 +322,12 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
> }
> }
>
> + /*
> + * 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 ?
> if (*util * boost_max < *max * boost_util) {
> *util = boost_util;
> *max = boost_max;
Otherwise looks good.
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 21-May 15:22, Viresh Kumar wrote:
> On 21-05-18, 09:51, Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > +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 */
> > + 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?
[...]
> > + /*
> > + * 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.
> > if (*util * boost_max < *max * boost_util) {
> > *util = boost_util;
> > *max = boost_max;
>
> Otherwise looks good.
>
> Acked-by: Viresh Kumar <[email protected]>
Cheers
--
#include <best/regards.h>
Patrick Bellasi
On 21-05-18, 11:11, Patrick Bellasi wrote:
> On 21-May 15:22, Viresh Kumar wrote:
> > On 21-05-18, 09:51, Patrick Bellasi wrote:
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > +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 */
> > > + 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?
>
> [...]
>
> > > + /*
> > > + * 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.
Okay for both.
--
viresh
On Mon, May 21, 2018 at 09:51:19AM +0100, Patrick Bellasi wrote:
> 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(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e13df951aca7..c4063e578e4d 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -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;
> + }
> + }
> +
Yes, I guess this effects single policies only. For shared, we are resetting
the iowait boost on the wake-up-from-idle update anyway. So even if it was
doubled, it would be reset.
Looks good to me! thanks,
Reviewed-by: Joel Fernandes (Google) <[email protected]>
On Mon, May 21, 2018 at 09:51:20AM +0100, Patrick Bellasi wrote:
> 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
makes->make
updated->update
> 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.
Reviewed-by: Joel Fernandes (Google) <[email protected]>
thanks,
- Joel
[..]
Acked-by: Peter Zijlstra (Intel) <[email protected]>