Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753697AbdGJKzG (ORCPT ); Mon, 10 Jul 2017 06:55:06 -0400 Received: from foss.arm.com ([217.140.101.70]:34184 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753598AbdGJKzF (ORCPT ); Mon, 10 Jul 2017 06:55:05 -0400 Date: Mon, 10 Jul 2017 11:55:00 +0100 From: Juri Lelli To: Joel Fernandes Cc: linux-kernel@vger.kernel.org, patrick.bellasi@arm.com, andresoportus@google.com, dietmar.eggemann@arm.com, Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Viresh Kumar , Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient Message-ID: <20170710105500.ef26lvurzg6ptbhq@e106622-lin> References: <20170709170826.29396-1-joelaf@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170709170826.29396-1-joelaf@google.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4977 Lines: 138 Hi Joel, On 09/07/17 10:08, Joel Fernandes wrote: > Currently the iowait_boost feature in schedutil makes the frequency go to max. > This feature was added to handle a case that Peter described where the > throughput of operations involving continuous I/O requests [1] is reduced due > to running at a lower frequency, however the lower throughput itself causes > utilization to be low and hence causing frequency to be low hence its "stuck". > > Instead of going to max, its also possible to achieve the same effect by > ramping up to max if there are repeated in_iowait wakeups happening. This patch > is an attempt to do that. We start from a lower frequency (iowait_boost_min) > and double the boost for every consecutive iowait update until we reach the > maximum iowait boost frequency (iowait_boost_max). > > I ran a synthetic test on an x86 machine with intel_pstate in passive mode > using schedutil. The patch achieves the desired effect as the existing > behavior. Also tested on ARM64 platform and see that there the transient iowait > requests aren't causing frequency spikes. > > [1] https://patchwork.kernel.org/patch/9735885/ > > Cc: Srinivas Pandruvada > Cc: Len Brown > Cc: Rafael J. Wysocki > Cc: Viresh Kumar > Cc: Ingo Molnar > Cc: Peter Zijlstra > Suggested-by: Peter Zijlstra > Signed-off-by: Joel Fernandes > --- > Changes since v1: > - not using tunables to plainly turn off iowait boost anymore > > kernel/sched/cpufreq_schedutil.c | 52 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 45 insertions(+), 7 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 622eed1b7658..4d9e8b96bed1 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -53,7 +53,9 @@ struct sugov_cpu { > struct update_util_data update_util; > struct sugov_policy *sg_policy; > > + bool prev_iowait_boost; > unsigned long iowait_boost; > + unsigned long iowait_boost_min; > unsigned long iowait_boost_max; > u64 last_update; > > @@ -168,22 +170,47 @@ static void sugov_get_util(unsigned long *util, unsigned long *max) > *max = cfs_max; > } > > +static void sugov_decay_iowait_boost(struct sugov_cpu *sg_cpu) > +{ > + sg_cpu->iowait_boost >>= 1; > + > + if (sg_cpu->iowait_boost < sg_cpu->iowait_boost_min) > + sg_cpu->iowait_boost = 0; > +} > + > 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; > + /* Remember for next time that we did an iowait boost */ > + sg_cpu->prev_iowait_boost = true; > + if (sg_cpu->iowait_boost) { > + sg_cpu->iowait_boost <<= 1; > + sg_cpu->iowait_boost = min(sg_cpu->iowait_boost, > + sg_cpu->iowait_boost_max); > + } else { > + sg_cpu->iowait_boost = sg_cpu->iowait_boost_min; > + } > } else if (sg_cpu->iowait_boost) { > s64 delta_ns = time - sg_cpu->last_update; > > /* Clear iowait_boost if the CPU apprears to have been idle. */ > if (delta_ns > TICK_NSEC) > sg_cpu->iowait_boost = 0; In this case we might just also want to reset prev_iowait_boost unconditionally and return. > + > + /* > + * Since we don't decay iowait_boost when its consumed during > + * the previous SCHED_CPUFREQ_IOWAIT update, decay it now. > + */ Code below seems pretty self-explaning to me. :) > + if (sg_cpu->prev_iowait_boost) { > + sugov_decay_iowait_boost(sg_cpu); > + sg_cpu->prev_iowait_boost = false; > + } > } > } > > static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util, > - unsigned long *max) > + unsigned long *max, unsigned int flags) > { > unsigned long boost_util = sg_cpu->iowait_boost; > unsigned long boost_max = sg_cpu->iowait_boost_max; > @@ -195,7 +222,16 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util, > *util = boost_util; > *max = boost_max; > } > - sg_cpu->iowait_boost >>= 1; > + > + /* > + * Incase iowait boost just happened on this CPU, don't reduce it right > + * away since then the iowait boost will never increase on subsequent > + * in_iowait wakeups. > + */ > + if (flags & SCHED_CPUFREQ_IOWAIT && this_cpu_ptr(&sugov_cpu) == sg_cpu) > + return; > + > + sugov_decay_iowait_boost(sg_cpu); Mmm, do we need to decay even when we are computing frequency requests for a domain? Considering it a per-cpu thing, isn't enough that it gets bumped up or decayed only when a CPU does an update (by using the above from sugov_update_shared)? If we go this way I think we will only need to reset prev_iowait_boost if delta_ns > TICK_NSEC during the for_each_cpu() loop of sugov_next_ freq_shared(). Thanks, - Juri