On Wed, Jul 12, 2017 at 8:35 PM, Viresh Kumar <viresh.ku...@linaro.org> wrote:
[..]
>
>> > diff --git a/kernel/sched/cpufreq_schedutil.c
>> > b/kernel/sched/cpufreq_schedutil.c
>> > index 076a2e31951c..3459f327c94e 100644
>> > --- a/kernel/sched/cpufreq_schedutil.c
>> > +++ b/kernel/sched/cpufreq_schedutil.c
>> > @@ -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,12 @@ 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;
>> > +
>> > + if (!sg_cpu->iowait_boost) {
>> > + sg_cpu->iowait_boost =
>> > sg_cpu->sg_policy->policy->cur;
>> > + sg_cpu->iowait_boost >>= 1;
>> > + }
>>
>> Hmm, this doesn't look right to me.. why are we decaying in this path?
>
> I am convinced that we need a comment here as what I did wasn't
> straight forward :)
>
> The idea: We wouldn't increase the frequency for the first event with
> IOWAIT flag set, but on every subsequent event (captured over
> rate-limit-us window). You may have noticed that I am updating the
> boost values in sugov_iowait_boost() a bit earlier now and they will
> affect the current frequency update as well.
>
> Because I wanted to do a 2X there unconditionally if
> iowait_boost_pending is set, I had to make it half for the very first
> event with IOWAIT flag set.
>
> End result:
> - First event, we stay at current freq.
> - Second and all consecutive events every rate_limit_us time, 2X
> - If there is no IOWAIT event in last rate_limit_us, X/2
>
> Makes sense ?
>
Yes, that makes sense, its a bit subtle but I get what you're doing
now and I agree with it. Its also cleaner than my original patch :-)
and yeah definitely needs a comment ;-)
thanks,
-Joel
