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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170713033531.GI1679@vireshk-i7>
Date:   Thu, 13 Jul 2017 09:05:31 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Joel Fernandes <joelaf@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Patrick Bellasi <patrick.bellasi@....com>,
        Juri Lelli <juri.lelli@....com>,
        Andres Oportus <andresoportus@...gle.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy
 efficient

On 12-07-17, 19:02, Joel Fernandes wrote:
> On Tue, Jul 11, 2017 at 10:00 PM, Viresh Kumar <viresh.kumar@...aro.org> wrote:

> Hmm, Ok. I can try to do some measurements about consecutive calls
> soon and let you know how often it happens. I also noticed its
> possible to call twice in the enqueue path itself as well.

Yeah, I think I told you that in previous replies.

> It probably
> affect my patch more because of starting from min than max.

Yeah, it will.

> > 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 ?

> I think nothing else other than setting the pending flag and the
> initial iowait_boost value is needed here.
> 
> Also I feel this is more "spike" prone than setting it initially to
> min. As Peter was saying, since we apply the boost only if it
> increases the frequency and not decreases, starting from min should at
> worst just result in ignoring of the boost the first time.

Yeah, we can discuss on what should be the default value to start with
and I would be fine with "min" if Rafael is, as he proposed the iowait
thing to begin with after seeing some real issues on hardware.

> >         } else if (sg_cpu->iowait_boost) {
> >                 s64 delta_ns = time - sg_cpu->last_update;
> >
> > @@ -182,17 +188,26 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> >  static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,
> >                                unsigned long *max)
> >  {
> > -       unsigned long boost_util = sg_cpu->iowait_boost;
> > -       unsigned long boost_max = sg_cpu->iowait_boost_max;
> > +       unsigned long boost_util, boost_max;
> >
> > -       if (!boost_util)
> > +       if (!sg_cpu->iowait_boost)
> >                 return;
> >
> > +       if (sg_cpu->iowait_boost_pending) {
> > +               sg_cpu->iowait_boost_pending = false;
> > +               sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,

I was talking about this unconditional 2X earlier.

> > +                                          sg_cpu->iowait_boost_max);
> > +       } else {
> > +               sg_cpu->iowait_boost >>= 1;
> > +       }
> 
> And then this path will do the decay correctly when the boost is applied.

Yeah, the else part should do good.

-- 
viresh