Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932821AbdGLFAl (ORCPT ); Wed, 12 Jul 2017 01:00:41 -0400 Received: from mail-pg0-f48.google.com ([74.125.83.48]:33853 "EHLO mail-pg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750764AbdGLFAk (ORCPT ); Wed, 12 Jul 2017 01:00:40 -0400 Date: Wed, 12 Jul 2017 10:30:35 +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: <20170712050035.GH17115@vireshk-i7> References: <20170709170826.29396-1-joelaf@google.com> <20170711101432.GB17115@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: 4901 Lines: 122 On 11-07-17, 07:14, Joel Fernandes wrote: > 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). Right. So we can take example that Peter gave earlier. Task runs .1 ms and waits for IO for 1 ms (at max speed). But there is high possibility that the util update handler gets called within that 1 ms (from non-enqueue paths) and because you chose to reduce iowait boost from sugov_set_iowait_boost() in your commit, we can easily end up ignoring iowait boosting. > 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. We can have bad enough timing where the util handler gets called right in that 1 ms of IOWAIT period and we will never boost. > 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. So my concerns weren't only about the initial min value, but also that you reduce the freq from sugov_set_iowait_boost(). We can discuss what the ideal value to start with can be. > This still will not increase > frequency on the first request, but will ensure the next one will > benefit. If there is no non-enqueue path request lands. > Yes, I've seen that happen in my testing (consecutive iowait). The CFS scheduler can send a util update request every 1 ms for util updates and I am not sure why isn't that happening in your case. How much is the time between two consecutive IOWAIT requests in your case ? Maybe it is too less (Ofcourse it isn't in your control :). But if we take Peter's example, then it will surely have a non-enqueue path request between two IOWAIT requests. > 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 may make us ignore all IOWAIT requests that happen between rate_limit_us time. And that would be bad IMHO. > 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. Completely untested, but what about something like this ? This should get rid of the spikes you were getting. 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; + } } 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, + sg_cpu->iowait_boost_max); + } else { + sg_cpu->iowait_boost >>= 1; + } + + boost_util = sg_cpu->iowait_boost; + boost_max = sg_cpu->iowait_boost_max; + if (*util * boost_max < *max * boost_util) { *util = boost_util; *max = boost_max; } - sg_cpu->iowait_boost >>= 1; } #ifdef CONFIG_NO_HZ_COMMON -- viresh