Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933551AbdGTDlz (ORCPT ); Wed, 19 Jul 2017 23:41:55 -0400 Received: from mail-pg0-f51.google.com ([74.125.83.51]:34309 "EHLO mail-pg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933218AbdGTDlx (ORCPT ); Wed, 19 Jul 2017 23:41:53 -0400 Date: Thu, 20 Jul 2017 09:11:48 +0530 From: Viresh Kumar To: Joel Fernandes Cc: LKML , Juri Lelli , Patrick Bellasi , Andres Oportus , Dietmar Eggemann , Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH RFC v5] cpufreq: schedutil: Make iowait boost more energy efficient Message-ID: <20170720034148.GI352@vireshk-i7> References: <20170716080407.28492-1-joelaf@google.com> <20170717080441.GM352@vireshk-i7> <20170718054558.GU352@vireshk-i7> <20170719061937.GB352@vireshk-i7> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3981 Lines: 106 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 ? > > 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