Re: [PATCH v3 2/2] cpufreq: schedutil: Cleanup and document iowait boost

Viresh Kumar
2018-05-21T09:53:04Z

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 <viresh.kumar@linaro.org>

--
viresh