Date: Tue, 18 Jul 2017 11:15:58 +0530
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Joel Fernandes <joelaf@google.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Juri Lelli <juri.lelli@arm.com>,
        Patrick Bellasi <patrick.bellasi@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>
Subject: Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy
 efficient
Message-ID: <20170718054558.GU352@vireshk-i7>
References: <20170716080407.28492-1-joelaf@google.com>
 <20170717080441.GM352@vireshk-i7>
 <CAJWu+oqU+qVMM4M0k4d7YeUFV-Aq4vZUU3JyWrMy6GQHA1MLQw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJWu+oqU+qVMM4M0k4d7YeUFV-Aq4vZUU3JyWrMy6GQHA1MLQw@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3957
Lines: 97

On 17-07-17, 10:35, Joel Fernandes wrote:
> On Mon, Jul 17, 2017 at 1:04 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 16-07-17, 01:04, Joel Fernandes wrote:

> >> +     if (sg_cpu->iowait_boost_pending) {
> >> +             sg_cpu->iowait_boost_pending = false;
> >> +             sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
> >> +                                        sg_cpu->iowait_boost_max);
> >
> > Now this has a problem. We will also boost after waiting for

s/also/always/

> > rate_limit_us. And that's why I had proposed the tricky solution in
> 
> Not really unless rate_limit_us is < TICK_NSEC? Once TICK_NSEC
> elapses, we would clear the boost in sugov_set_iowait_boost and in
> sugov_next_freq_shared.

You misread it and I know why it happened. And so I have sent a small
patch to make it a bit more readable.

rate_limit_us is associated with "last_freq_update_time", while
iowait-boost is associated with "last_update".

And last_update gets updated way too often.

> > the first place. I thought we wanted to avoid instant boost only for
> > the first iteration, but after that we wanted to do it ASAP. Isn't it?
> >
> > Now that you are using policy->min instead of policy->cur, we can
> > simplify the solution I proposed and always do 2 * iowait_boost before
> 
> No, doubling on the first boost was never discussed or intended in my
> earlier patches. I thought even your patch never did, you were
> dividing by 2, and then scaling it back up by 2 before consuming it to
> preserve the initial boost.
> 
> > getting current util/max in above if loop. i.e. we will start iowait
> > boost with min * 2 instead of min and that should be fine.
> 
> Hmm, but why start from double of min? Why not just min? It doesn't
> make any difference to the intended behavior itself and is also
> consistent with my proposal in RFC v4. Also I feel what you're
> suggesting is more spike prone as well, the idea was to start from the
> minimum and double it as we go, not to double the min the first go.
> That was never intended.
> 
> Also I would rather keep the "set and use and set and use" pattern to
> keep the logic less confusing and clean IMO.
> So we set initial boost in sugov_set_iowait_boost, and then in
> sugov_iowait_boost we use it, and then set the boost for the next time
> around at the end of sugov_iowait_boost (that is we double it). Next
> time sugov_set_iowait_boost wouldn't touch the boost whether iowait
> flag is set or not and we would continue into sugov_iowait_boost to
> consume the boost. This would have a small delay in reducing the
> boost, but that's Ok since its only one cycle of delay, and keeps the
> code clean. I assume the last part is not an issue considering you're
> proposing double of the initial boost anyway ;-)

Okay, let me try to explain the problem first and then you can propose
a solution if required.

Expected Behavior:

(Window refers to a time window of rate_limit_us here)

A. The first window where IOWAIT flag is set, we set boost to min-freq
   and that shall be used for next freq update in
   sugov_iowait_boost().  Any more calls to sugov_set_iowait_boost()
   within this window shouldn't change the behavior.

B. If the next window also has IOWAIT flag set, then
   sugov_iowait_boost() should use iowait*2 for freq update.

C. If a window doesn't have IOWAIT flag set, then sugov_iowait_boost()
   should use iowait/2 in it.


Do they look fine to you?

Now coming to how will system behave with your patch:

A. would be fine. We will follow things properly.

But B. and C. aren't true anymore.

This happened because after the first window we updated iowait_boost
as 2*min unconditionally and the next window will *always* use that,
even if the flag isn't set. And we may end up increasing the frequency
unnecessarily, i.e. the spike where this discussion started.

And so in my initial solution I reversed the order in
sugov_iowait_boost().

-- 
viresh