Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754403AbdGKFMS (ORCPT ); Tue, 11 Jul 2017 01:12:18 -0400 Received: from mail-oi0-f47.google.com ([209.85.218.47]:35525 "EHLO mail-oi0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbdGKFMR (ORCPT ); Tue, 11 Jul 2017 01:12:17 -0400 MIME-Version: 1.0 In-Reply-To: <20170710105500.ef26lvurzg6ptbhq@e106622-lin> References: <20170709170826.29396-1-joelaf@google.com> <20170710105500.ef26lvurzg6ptbhq@e106622-lin> From: Joel Fernandes Date: Mon, 10 Jul 2017 22:12:15 -0700 Message-ID: Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient To: Juri Lelli Cc: LKML , Patrick Bellasi , Andres Oportus , Dietmar Eggemann , Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Viresh Kumar , 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: 5722 Lines: 147 Hi Juri, On Mon, Jul 10, 2017 at 3:55 AM, Juri Lelli wrote: > 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 [..] >> >> 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. Ok, will do. >> + >> + /* >> + * 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. :) > Ok :) >> + 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(). > Actually the "decay" was already being done before (without this patch), I am just preserving the same old behavior where we do decay. Perhaps your proposal can be a separate match? Or did I miss something else subtle here? Thanks, -Joel