From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750871AbdESGud (ORCPT ); Fri, 19 May 2017 02:50:33 -0400 Received: from mail-pg0-f49.google.com ([74.125.83.49]:35401 "EHLO mail-pg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750728AbdESGua (ORCPT ); Fri, 19 May 2017 02:50:30 -0400 Date: Fri, 19 May 2017 12:20:21 +0530 From: Viresh Kumar To: Joel Fernandes Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Srinivas Pandruvada , Len Brown , "Rafael J . Wysocki" , Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH v2 2/2] sched: Make iowait_boost optional in schedutil Message-ID: <20170519065021.GK17481@vireshk-i7> References: <20170519062344.27692-1-joelaf@google.com> <20170519062344.27692-3-joelaf@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170519062344.27692-3-joelaf@google.com> 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 On 18-05-17, 23:23, Joel Fernandes wrote: > We should apply the iowait boost only if cpufreq policy has iowait boost > enabled. Also make it a schedutil configuration from sysfs so it can be turned > on/off if needed (by default initialize it to the policy value). > > For systems that don't need/want it enabled, such as those on arm64 based > mobile devices that are battery operated, it saves energy when the cpufreq > driver policy doesn't have it enabled (details below): > > Here are some results for energy measurements collected running a YouTube video > for 30 seconds: > Before: 8.042533 mWh > After: 7.948377 mWh > Energy savings is ~1.2% > > Cc: Srinivas Pandruvada > Cc: Len Brown > Cc: Rafael J. Wysocki > Cc: Viresh Kumar > Cc: Ingo Molnar > Cc: Peter Zijlstra > Signed-off-by: Joel Fernandes > --- > kernel/sched/cpufreq_schedutil.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 76877a62b5fa..0e392b58b9b3 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -24,6 +24,7 @@ > struct sugov_tunables { > struct gov_attr_set attr_set; > unsigned int rate_limit_us; > + bool iowait_boost_enable; I suggested s/iowait_boost_enable/iowait_boost/ and you said okay for that change. > }; > > struct sugov_policy { > @@ -171,6 +172,11 @@ static void sugov_get_util(unsigned long *util, unsigned long *max) > static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, > unsigned int flags) > { > + struct sugov_policy *sg_policy = sg_cpu->sg_policy; > + > + if (!sg_policy->tunables->iowait_boost_enable) > + return; > + > if (flags & SCHED_CPUFREQ_IOWAIT) { > sg_cpu->iowait_boost = sg_cpu->iowait_boost_max; > } else if (sg_cpu->iowait_boost) { > @@ -386,10 +392,34 @@ static ssize_t rate_limit_us_store(struct gov_attr_set *attr_set, const char *bu > return count; > } > > +static ssize_t iowait_boost_enable_show(struct gov_attr_set *attr_set, > + char *buf) > +{ > + struct sugov_tunables *tunables = to_sugov_tunables(attr_set); > + > + return sprintf(buf, "%u\n", tunables->iowait_boost_enable); > +} > + > +static ssize_t iowait_boost_enable_store(struct gov_attr_set *attr_set, > + const char *buf, size_t count) > +{ > + struct sugov_tunables *tunables = to_sugov_tunables(attr_set); > + bool enable; > + > + if (kstrtobool(buf, &enable)) > + return -EINVAL; > + > + tunables->iowait_boost_enable = enable; > + > + return count; > +} > + > static struct governor_attr rate_limit_us = __ATTR_RW(rate_limit_us); > +static struct governor_attr iowait_boost_enable = __ATTR_RW(iowait_boost_enable); > > static struct attribute *sugov_attributes[] = { > &rate_limit_us.attr, > + &iowait_boost_enable.attr, > NULL > }; Do we really need this right now? I mean, are you going to use it this way? It may never get used eventually and may be better to leave the sysfs option for now. > @@ -543,6 +573,8 @@ static int sugov_init(struct cpufreq_policy *policy) > tunables->rate_limit_us *= lat; > } > > + tunables->iowait_boost_enable = policy->iowait_boost_enable; > + > policy->governor_data = sg_policy; > sg_policy->tunables = tunables; -- viresh