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

[Rafael J. Wysocki]

On Thu, May 18, 2017 at 8:30 PM, Joel Fernandes <joelaf@xxxxxxxxxx> 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).
>
> Signed-off-by: Joel Fernandes <joelaf@xxxxxxxxxx>
> ---
>  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;
>  };
>
>  struct sugov_policy {
> @@ -47,6 +48,7 @@ struct sugov_policy {
>         bool work_in_progress;
>
>         bool need_freq_update;
> +       unsigned int iowait_boost_enable;
>  };
>
>  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;

Well, that's just

tunables->iowait_boost_enable = policy->iowait_boost_enable;

so I'm not sure about the role of the if ().

Also, do we only need the policy field as an initial value here?
That's sort of confusing, because intel_pstate does iowait boosting in
the active mode too and then it is unconditional.

> +
>         policy->governor_data = sg_policy;
>         sg_policy->tunables = tunables;
>
> --
> 2.13.0.303.g4ebf302169-goog

Thanks,
Rafael