lkml.org 
[lkml]   [2017]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [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

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