MIME-Version: 1.0
In-Reply-To: <20170713033531.GI1679@vireshk-i7>
References: <20170709170826.29396-1-joelaf@google.com> <20170711101432.GB17115@vireshk-i7>
 <CAJWu+oqgN+1V4AF2QowT+fT_5SMQoNMy7NgrO8Zf3eSvAByTCg@mail.gmail.com>
 <20170712050035.GH17115@vireshk-i7> <CAJWu+ory5-qcTcN676tm1GQ=jSuMh7FPn4QOO9hQu33w_QgAmA@mail.gmail.com>
 <20170713033531.GI1679@vireshk-i7>
From: Joel Fernandes <joelaf@google.com>
Date: Wed, 12 Jul 2017 20:52:59 -0700
Message-ID: <CAJWu+or8XwnoA4SM93Cu2nhKoE8UwbdADYqPzxJ-KxOFL8uR-w@mail.gmail.com>
Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Patrick Bellasi <patrick.bellasi@arm.com>,
        Juri Lelli <juri.lelli@arm.com>,
        Andres Oportus <andresoportus@google.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Len Brown <lenb@kernel.org>, "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2215
Lines: 57

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