linux-kernel - Re: [PATCH 2/2] sched: Use iowait boost policy option in schedutil

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]

Message-ID: <CAOh2x=kHpv7=_YKqMjoVcfBCUyU4QdMsZ7=eb2NQjt4Qyc16-w@mail.gmail.com>
Date:   Fri, 19 May 2017 10:09:47 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Joel Fernandes <joelaf@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 2/2] sched: Use iowait boost policy option in schedutil

On Fri, May 19, 2017 at 12:00 AM, Joel Fernandes <joelaf@...gle.com> wrote:
> If cpufreq policy has iowait boost enabled, use it. Also make it a schedutil
> configuration from sysfs so it can be turned on/off if needed (by default use
> the policy value).

As I understand it, you want to make iowait boost optional. It may be worth
mentioning why we want to do that in the commit somewhere. I am quite sure
you don't want for some of the ARM platforms. Can you please elaborate?

PeterZ once indicated that he is against adding any tunables for the schedutil
governor. This is another one we have now.

> Signed-off-by: Joel Fernandes <joelaf@...gle.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 76877a62b5fa..6915925bc947 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;
> +       unsigned int iowait_boost_enable;

Maybe just:

bool iowait_boost;

>  };
>
>  struct sugov_policy {
> @@ -47,6 +48,7 @@ struct sugov_policy {
>         bool work_in_progress;
>
>         bool need_freq_update;
> +       unsigned int iowait_boost_enable;

I don't see this being used at all. Am I missing something ?

>  };
>
>  struct sugov_cpu {
> @@ -171,6 +173,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 +393,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);
> +       unsigned int enable;
> +
> +       if (kstrtouint(buf, 10, &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
>  };
>
> @@ -543,6 +574,9 @@ static int sugov_init(struct cpufreq_policy *policy)
>                         tunables->rate_limit_us *= lat;
>         }
>
> +       if (policy->iowait_boost_enable)
> +               tunables->iowait_boost_enable = policy->iowait_boost_enable;
> +
>         policy->governor_data = sg_policy;
>         sg_policy->tunables = tunables;
>
> --
> 2.13.0.303.g4ebf302169-goog
>