lkml.org 
[lkml]   [2017]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient
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

\
 
 \ /
  Last update: 2017-07-20 23:18    [W:0.057 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site