Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936459AbdGTTtl (ORCPT ); Thu, 20 Jul 2017 15:49:41 -0400 Received: from mail-oi0-f54.google.com ([209.85.218.54]:33159 "EHLO mail-oi0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935971AbdGTTtj (ORCPT ); Thu, 20 Jul 2017 15:49:39 -0400 MIME-Version: 1.0 In-Reply-To: <20170720034148.GI352@vireshk-i7> References: <20170716080407.28492-1-joelaf@google.com> <20170717080441.GM352@vireshk-i7> <20170718054558.GU352@vireshk-i7> <20170719061937.GB352@vireshk-i7> <20170720034148.GI352@vireshk-i7> From: Joel Fernandes Date: Thu, 20 Jul 2017 12:49:37 -0700 Message-ID: Subject: Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient To: Viresh Kumar Cc: LKML , Juri Lelli , Patrick Bellasi , Andres Oportus , Dietmar Eggemann , Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Ingo Molnar , Peter Zijlstra Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4731 Lines: 130 Hi Viresh, On Wed, Jul 19, 2017 at 8:41 PM, Viresh Kumar wrote: > On 19-07-17, 19:38, Joel Fernandes wrote: >> On Tue, Jul 18, 2017 at 11:19 PM, Viresh Kumar 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