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

[Viresh Kumar <viresh.kumar@linaro.org>]

On 19-07-17, 19:38, Joel Fernandes wrote:
> On Tue, Jul 18, 2017 at 11:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 18-07-17, 21:39, Joel Fernandes wrote:
> >> Not really, to me B will still work because in the case the flag is
> >> set, we are correctly double boosting in the next cycle.
> >>
> >> Taking an example, with B = flag is set and D = flag is not set
> >>
> >> F = Fmin (minimum)
> >>
> >> iowait flag       B  B    B    D    D    D
> >> resulting boost   F  2*F  4*F  4*F  2*F  F
> >
> > What about this ?
> >
> > iowait flag       B  D    B    D    B    D
> > resulting boost   F  2*F  F    2*F  F    2*F
> 
> Yes I guess so but this oscillation can still happen in current code I think.

How ?

> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 45fcf21ad685..ceac5f72d8da 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,17 @@ 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;
> > +               if (sg_cpu->iowait_boost_pending)
> > +                       return;
> > +
> > +               sg_cpu->iowait_boost_pending = true;
> > +
> > +               if (sg_cpu->iowait_boost) {
> > +                       sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
> > +                                                  sg_cpu->iowait_boost_max);
> > +               } else {
> > +                       sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
> > +               }
> 
> I would prefer this to be:
> 
>       if (sg_cpu->iowait_boost >= policy->min) {
>           // double it
>       } else {
>           // set it to min
>       }
> 
> This is for the case when boost happens all the way, then its capped
> at max, but when its decayed back, its not exactly decayed to Fmin but
> lower than it, so in that case when boost next time we start from
> Fmin.

Actually you can add another patch first which makes iowait_boost as 0
when it goes below min as that problem exists today as well.

And this patch would be fine then as is ?

> >         } else if (sg_cpu->iowait_boost) {
> >                 s64 delta_ns = time - sg_cpu->last_update;
> >
> > @@ -182,17 +193,23 @@ 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;
> > +       else
> > +               sg_cpu->iowait_boost >>= 1;
> > +
> > +       boost_util = sg_cpu->iowait_boost;
> > +       boost_max = sg_cpu->iowait_boost_max;
> > +
> >         if (*util * boost_max < *max * boost_util) {
> >                 *util = boost_util;
> >                 *max = boost_max;
> 
> This looks good to me and is kind of what I had in mind. I can spend
> some time testing it soon. Just to be clear if I were to repost this
> patch after testing, should I have your authorship and my tested-by or
> do you prefer something else?

You can keep your authorship I wouldn't mind. Maybe a suggested-by at
max would be fine.

-- 
viresh