From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753331AbeEUReB (ORCPT ); Mon, 21 May 2018 13:34:01 -0400 Received: from mail-pl0-f65.google.com ([209.85.160.65]:34946 "EHLO mail-pl0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753303AbeEURd6 (ORCPT ); Mon, 21 May 2018 13:33:58 -0400 X-Google-Smtp-Source: AB8JxZrdepHHx9E8/GWoutcXFS9/9BzvOGppX8KgU7pQG9dhzVW/+gyl7V1H/fUPSkVb4+ZH1pW2LQ== Date: Mon, 21 May 2018 10:33:56 -0700 From: Joel Fernandes To: Patrick Bellasi Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Ingo Molnar , Peter Zijlstra , "Rafael J . Wysocki" , Viresh Kumar , Vincent Guittot , Dietmar Eggemann , Juri Lelli , kernel-team@android.com Subject: Re: [PATCH v3 1/2] cpufreq: schedutil: Fix iowait boost reset Message-ID: <20180521173356.GB21678@joelaf.mtv.corp.google.com> References: <20180521085120.7902-1-patrick.bellasi@arm.com> <20180521085120.7902-2-patrick.bellasi@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180521085120.7902-2-patrick.bellasi@arm.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 21, 2018 at 09:51:19AM +0100, Patrick Bellasi wrote: > A more energy efficient update of the IO wait boosting mechanism has > been introduced in: > > commit a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient") > > where the boost value is expected to be: > > - doubled at each successive wakeup from IO > staring from the minimum frequency supported by a CPU > > - reset when a CPU is not updated for more then one tick > by either disabling the IO wait boost or resetting its value to the > minimum frequency if this new update requires an IO boost. > > This approach is supposed to "ignore" boosting for sporadic wakeups from > IO, while still getting the frequency boosted to the maximum to benefit > long sequence of wakeup from IO operations. > > However, these assumptions are not always satisfied. > For example, when an IO boosted CPU enters idle for more the one tick > and then wakes up after an IO wait, since in sugov_set_iowait_boost() we > first check the IOWAIT flag, we keep doubling the iowait boost instead > of restarting from the minimum frequency value. > > This misbehavior could happen mainly on non-shared frequency domains, > thus defeating the energy efficiency optimization, but it can also > happen on shared frequency domain systems. > > Let fix this issue in sugov_set_iowait_boost() by: > - first check the IO wait boost reset conditions > to eventually reset the boost value > - then applying the correct IO boost value > if required by the caller > > Reported-by: Viresh Kumar > Signed-off-by: Patrick Bellasi > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Rafael J. Wysocki > Cc: Viresh Kumar > Cc: Joel Fernandes > Cc: Juri Lelli > Cc: Dietmar Eggemann > Cc: linux-kernel@vger.kernel.org > Cc: linux-pm@vger.kernel.org > Fixes: a5a0809bc58e ("cpufreq: schedutil: Make iowait boost more energy efficient") > > --- > Changes in v3: > - split the fix into a separated patch (this one) > - added "Fixes" tag (Viresh) > --- > kernel/sched/cpufreq_schedutil.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index e13df951aca7..c4063e578e4d 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -203,6 +203,16 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu) > > static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags) > { > + /* Clear iowait_boost if the CPU apprears to have been idle. */ > + if (sg_cpu->iowait_boost) { > + s64 delta_ns = time - sg_cpu->last_update; > + > + if (delta_ns > TICK_NSEC) { > + sg_cpu->iowait_boost = 0; > + sg_cpu->iowait_boost_pending = false; > + } > + } > + Yes, I guess this effects single policies only. For shared, we are resetting the iowait boost on the wake-up-from-idle update anyway. So even if it was doubled, it would be reset. Looks good to me! thanks, Reviewed-by: Joel Fernandes (Google)