Date:   Thu, 20 Jul 2017 12:49:37 -0700
From:   Joel Fernandes <joelaf@...gle.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Juri Lelli <juri.lelli@....com>,
        Patrick Bellasi <patrick.bellasi@....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 v5] cpufreq: schedutil: Make iowait boost more energy efficient

Hi Viresh,

On Wed, Jul 19, 2017 at 8:41 PM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> On 19-07-17, 19:38, Joel Fernandes wrote:
>> On Tue, Jul 18, 2017 at 11:19 PM, Viresh Kumar <viresh.kumar@...aro.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 ?

Yes you're right, its not an issue with current code.

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

Yes I think that's fine, I thought about it some more and I think this
can be an issue in a scenario where

iowait_boost_max < policy->min  but:

(iowait_boost / iowait_boost_max) > (rq->cfs.avg.util_avg /
arch_scale_cpu_capacity)

This is probably not a common case in current real world cases but if
iowait_boost_max is say way less than arch_scale_cpu_capacity for some
reason in the future, then it can be an issue I think. I'll post a
patch for it.

>
>> >         } 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.
>

Cool, will do. Thanks a lot Viresh.


thanks,

-Joel

Powered by blists - more mailing lists