Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932812AbdGKOOQ (ORCPT ); Tue, 11 Jul 2017 10:14:16 -0400 Received: from mail-oi0-f42.google.com ([209.85.218.42]:33469 "EHLO mail-oi0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932578AbdGKOOP (ORCPT ); Tue, 11 Jul 2017 10:14:15 -0400 MIME-Version: 1.0 In-Reply-To: <20170711101432.GB17115@vireshk-i7> References: <20170709170826.29396-1-joelaf@google.com> <20170711101432.GB17115@vireshk-i7> From: Joel Fernandes Date: Tue, 11 Jul 2017 07:14:13 -0700 Message-ID: Subject: Re: [PATCH RFC v4] cpufreq: schedutil: Make iowait boost more energy efficient To: Viresh Kumar Cc: LKML , Patrick Bellasi , Juri Lelli , Andres Oportus , Dietmar Eggemann , Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , 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: 4784 Lines: 106 Hi Viresh, On Tue, Jul 11, 2017 at 3:14 AM, Viresh Kumar wrote: > On 09-07-17, 10:08, Joel Fernandes wrote: >> 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; > > I am not sure if boost should start from the min frequency, as the current > frequency will at least be equal to that. Which means that with no boost > initially, we will never increase the frequency for the first IOWAIT flag event. I think the whole point of IOWAIT boost was to solve the issue with a long sequence of repeated I/O requests as described in the commit message. So IIUC there isn't a usecase for that (increase freq. on first request). Also its just for the first couple of requests in my testing and doesn't hurt the performance at all for the intended usecase while still not causing transient spikes. Another approach than setting min in sugov_set_iowait_boost, is, since we have already retrieved the current util, we can check if flags == SCHED_CPUFREQ_IOWAIT, then set initial the iowait_boost such that (iowait_boost / iowait_boost_max) is aleast equal to (util / max) or iowait_boost_min, which ever is lower. This still will not increase frequency on the first request, but will ensure the next one will benefit. Then again I fear running into slightly longer-term transients where 2 iowait requests are enough to boost the frequency to high values. I can try to measure how often this can hurt power for common usecases if you agree its worth exploring. > >> + } >> } 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; >> + >> + /* >> + * Since we don't decay iowait_boost when its consumed during >> + * the previous SCHED_CPUFREQ_IOWAIT update, decay it now. >> + */ >> + if (sg_cpu->prev_iowait_boost) { > > SCHED_CPUFREQ_IOWAIT flag is set only by CFS from the enqueue_task() and in many > cases we call the util hook twice from the same enqueue_task() instance before > returning (2nd one after updating util). And in such cases we will set > iowait_boost as 0 on the second call. > > Have you ever seen two consecutive calls to sugov_set_iowait_boost() with IOWAIT > flag set ? Can we get the ratio of that against the other case where we have > IOWAIT flag set in first call, followed by one or more non-IOWAIT calls and then > IOWAIT again ? > > I am asking because if the calls with IOWAIT flag aren't consecutive then we > will make iowait_boost as 0 in the next non-IOWAIT call. Yes, I've seen that happen in my testing (consecutive iowait). I haven't seen the other case where you have IOWAIT followed by non-IOWAIT for a repeated set of IOWAIT requests. Would you more comfortable if we moved sugov_set_iowait_boost() after the sugov_should_update_freq() ? That way if there are consecutive requests in the same path, then it most likely rate-limiting will prevent such updates. I will also try to collect some stats as you suggested to see if how often if at all this can happen. thanks, -Joel