linux-kernel - Re: [PATCH v2 2/2] sched: Make iowait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 May 2017 12:20:21 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Joel Fernandes <joelaf@...gle.com>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2 2/2] sched: Make iowait_boost optional in schedutil

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 <srinivas.pandruvada@...ux.intel.com>
> Cc: Len Brown <lenb@...nel.org>
> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> Cc: Ingo Molnar <mingo@...hat.com> 
> Cc: Peter Zijlstra <peterz@...radead.org> 
> Signed-off-by: Joel Fernandes <joelaf@...gle.com>
> ---
>  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