Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient

[Joel Fernandes]

On Wed, Jul 12, 2017 at 8:35 PM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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