Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752855AbdGSGTm (ORCPT ); Wed, 19 Jul 2017 02:19:42 -0400 Received: from mail-pf0-f177.google.com ([209.85.192.177]:33572 "EHLO mail-pf0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752760AbdGSGTl (ORCPT ); Wed, 19 Jul 2017 02:19:41 -0400 Date: Wed, 19 Jul 2017 11:49:37 +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: <20170719061937.GB352@vireshk-i7> References: <20170716080407.28492-1-joelaf@google.com> <20170717080441.GM352@vireshk-i7> <20170718054558.GU352@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: 4641 Lines: 122 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 Isn't this the worst behavior we may wish for ? > What will not work is C but as I mentioned in my last email, that > would cause us to delay the iowait boost halving by at most 1 cycle, > is that really an issue considering we are starting from min compared > to max? Note that cases A. and B. are still working. > > Considering the following cases: > (1) min freq is 800MHz, it takes upto 4 cycles to reach 4GHz where the > flag is set. At this point I think its likely we will run for many > more cycles which means keeping the boost active for 1 extra cycle > isn't that big a deal. Even if run for just 5 cycles with boost, that > means only the last cycle will suffer from C not decaying as soon as > possible. Comparing that to the case where in current code we > currently run at max from the first cycle, its not that bad. > > (2) we have a transient type of thing, in this case we're probably not > reaching the full max immediately so even if we delay the decaying, > its still not as bad as what it is currently. > > I think considering that the code is way cleaner than any other > approach - its a small price to pay. Also keeping in mind that this > patch is still an improvement over the current spike, even though as > you said its still a bit spikey, but its still better right? > > Functionally the code is working and I think is also clean, but if you > feel that its still confusing, then I'm open to rewriting it. I am not worried for being boosted for a bit more time, but with the spikes even when we do not want a freq change. > > And so in my initial solution I reversed the order in > > sugov_iowait_boost(). > > Yes, but to fix A. you had to divide by 2 in sugov_set_iowait_boost, > and then multiply by 2 later in sugov_iowait_boost to keep the first > boost at min. That IMO was confusing so my modified patch did it > differently. Yeah, it wasn't great for sure. I hope the following one will work for everyone ? 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; + } } 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; } - sg_cpu->iowait_boost >>= 1; } #ifdef CONFIG_NO_HZ_COMMON -- viresh