Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751073AbdGMDfg (ORCPT ); Wed, 12 Jul 2017 23:35:36 -0400 Received: from mail-pf0-f179.google.com ([209.85.192.179]:36083 "EHLO mail-pf0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbdGMDff (ORCPT ); Wed, 12 Jul 2017 23:35:35 -0400 Date: Thu, 13 Jul 2017 09:05:31 +0530 From: Viresh Kumar To: Joel Fernandes Cc: LKML , Patrick Bellasi , Juri Lelli , Andres Oportus , Dietmar Eggemann , Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient Message-ID: <20170713033531.GI1679@vireshk-i7> References: <20170709170826.29396-1-joelaf@google.com> <20170711101432.GB17115@vireshk-i7> <20170712050035.GH17115@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: 4083 Lines: 104 On 12-07-17, 19:02, Joel Fernandes wrote: > On Tue, Jul 11, 2017 at 10:00 PM, Viresh Kumar wrote: > Hmm, Ok. I can try to do some measurements about consecutive calls > soon and let you know how often it happens. I also noticed its > possible to call twice in the enqueue path itself as well. Yeah, I think I told you that in previous replies. > It probably > affect my patch more because of starting from min than max. Yeah, it will. > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 076a2e31951c..3459f327c94e 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,12 @@ 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; > > + sg_cpu->iowait_boost_pending = true; > > + > > + if (!sg_cpu->iowait_boost) { > > + sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->cur; > > + sg_cpu->iowait_boost >>= 1; > > + } > > Hmm, this doesn't look right to me.. why are we decaying in this path? I am convinced that we need a comment here as what I did wasn't straight forward :) The idea: We wouldn't increase the frequency for the first event with IOWAIT flag set, but on every subsequent event (captured over rate-limit-us window). You may have noticed that I am updating the boost values in sugov_iowait_boost() a bit earlier now and they will affect the current frequency update as well. Because I wanted to do a 2X there unconditionally if iowait_boost_pending is set, I had to make it half for the very first event with IOWAIT flag set. End result: - First event, we stay at current freq. - Second and all consecutive events every rate_limit_us time, 2X - If there is no IOWAIT event in last rate_limit_us, X/2 Makes sense ? > I think nothing else other than setting the pending flag and the > initial iowait_boost value is needed here. > > Also I feel this is more "spike" prone than setting it initially to > min. As Peter was saying, since we apply the boost only if it > increases the frequency and not decreases, starting from min should at > worst just result in ignoring of the boost the first time. Yeah, we can discuss on what should be the default value to start with and I would be fine with "min" if Rafael is, as he proposed the iowait thing to begin with after seeing some real issues on hardware. > > } else if (sg_cpu->iowait_boost) { > > s64 delta_ns = time - sg_cpu->last_update; > > > > @@ -182,17 +188,26 @@ 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; > > + sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1, I was talking about this unconditional 2X earlier. > > + sg_cpu->iowait_boost_max); > > + } else { > > + sg_cpu->iowait_boost >>= 1; > > + } > > And then this path will do the decay correctly when the boost is applied. Yeah, the else part should do good. -- viresh