Messages in this thread Patch in this message | | | From | Joel Fernandes <> | Date | Thu, 20 Jul 2017 12:49:37 -0700 | 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@linaro.org> wrote: > 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 ?
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
| |