[PATCH 1/2] cpufreq: Make iowait boost a policy option

[PATCH 1/2] cpufreq: Make iowait boost a policy option

[Joel Fernandes]

Make iowait boost a cpufreq policy option and enable it for intel_pstate
cpufreq driver. Governors like schedutil can use it to determine if
boosting for tasks that wake up with p->in_iowait set is needed.

Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 drivers/cpufreq/intel_pstate.c | 1 +
 include/linux/cpufreq.h        | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index b7de5bd76a31..a3099f099779 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2239,6 +2239,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = INTEL_CPUFREQ_TRANSITION_LATENCY;
 	policy->transition_delay_us = INTEL_CPUFREQ_TRANSITION_DELAY;
+	policy->iowait_boost_enable = 1;
 	/* This reflects the intel_pstate_get_cpu_pstates() setting. */
 	policy->cur = policy->cpuinfo.min_freq;
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index a5ce0bbeadb5..4bb086dbe7ec 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -127,6 +127,9 @@ struct cpufreq_policy {
 	 */
 	unsigned int		transition_delay_us;
 
+	/* Boost switch for tasks with p->in_iowait set */
+	unsigned int		iowait_boost_enable;
+
 	 /* Cached frequency lookup from cpufreq_driver_resolve_freq. */
 	unsigned int cached_target_freq;
 	int cached_resolved_idx;
-- 
2.13.0.303.g4ebf302169-goog

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

[Joel Fernandes]

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@google.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;
 };
 
 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;
+
 	policy->governor_data = sg_policy;
 	sg_policy->tunables = tunables;
 
-- 
2.13.0.303.g4ebf302169-goog

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@google.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).
>
> Signed-off-by: Joel Fernandes <joelaf@google.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;
>  };
>
>  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

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

[Joel Fernandes]

Hi Rafael,

On Thu, May 18, 2017 at 6:08 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
[..]
>>  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 ().

Yes you're right, will fix.

>
> 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.
>

I didn't get your comment. Could you clarify what you meant by
'intel_pstate does iowait boosting in the active mode' ? Is there
another path outside the governor where intel_pstate does iowait
boosting, or are you referring to the hardware behavior?

thanks,

-Joel

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

[Rafael J. Wysocki]

On Fri, May 19, 2017 at 3:17 AM, Joel Fernandes <joelaf@google.com> wrote:
> Hi Rafael,
>
> On Thu, May 18, 2017 at 6:08 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> [..]
>>>  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 ().
>
> Yes you're right, will fix.
>
>>
>> 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.
>>
>
> I didn't get your comment. Could you clarify what you meant by
> 'intel_pstate does iowait boosting in the active mode' ? Is there
> another path outside the governor where intel_pstate does iowait
> boosting,

Yes.

> or are you referring to the hardware behavior?

No.

In the active mode intel_pstate acts as a governor too and it does
iowait boosting unconditionally then.

But that's OK on a second thought, as the policy field will affect the
passive mode only anyway and setting it on init will actually cause
the behavior to be consistent.

Thanks,
Rafael

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

[Viresh Kumar]

On Fri, May 19, 2017 at 12:00 AM, Joel Fernandes <joelaf@google.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@google.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
>

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

[Joel Fernandes]

On Thu, May 18, 2017 at 9:39 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, May 19, 2017 at 12:00 AM, Joel Fernandes <joelaf@google.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?

Yes, I actually have power measurements too which I will include in v2.
This is for an arm64 based Qualcomm SoC.

thanks,

-Joel

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

[Joel Fernandes]

On Thu, May 18, 2017 at 9:39 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
[..]
>> 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;

Yes, Rafael mentioned this for the cpufreq framework part, I will do
it this way in v2.

>
>>  };
>>
>>  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 ?
>

Ah! Yes you're right, this is spurious, I will drop it. Thanks for spotting it.

-Joel

Re: [PATCH 1/2] cpufreq: Make iowait boost a policy option

[Rafael J. Wysocki]

On Thu, May 18, 2017 at 8:30 PM, Joel Fernandes <joelaf@google.com> wrote:
> Make iowait boost a cpufreq policy option and enable it for intel_pstate
> cpufreq driver. Governors like schedutil can use it to determine if
> boosting for tasks that wake up with p->in_iowait set is needed.
>
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 1 +
>  include/linux/cpufreq.h        | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index b7de5bd76a31..a3099f099779 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2239,6 +2239,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy)
>
>         policy->cpuinfo.transition_latency = INTEL_CPUFREQ_TRANSITION_LATENCY;
>         policy->transition_delay_us = INTEL_CPUFREQ_TRANSITION_DELAY;
> +       policy->iowait_boost_enable = 1;
>         /* This reflects the intel_pstate_get_cpu_pstates() setting. */
>         policy->cur = policy->cpuinfo.min_freq;
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index a5ce0bbeadb5..4bb086dbe7ec 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -127,6 +127,9 @@ struct cpufreq_policy {
>          */
>         unsigned int            transition_delay_us;
>
> +       /* Boost switch for tasks with p->in_iowait set */
> +       unsigned int            iowait_boost_enable;

Could that be a bool field?

> +
>          /* Cached frequency lookup from cpufreq_driver_resolve_freq. */
>         unsigned int cached_target_freq;
>         int cached_resolved_idx;
> --
> 2.13.0.303.g4ebf302169-goog
>

Thanks,
Rafael

Re: [PATCH 1/2] cpufreq: Make iowait boost a policy option

[Joel Fernandes]

On Thu, May 18, 2017 at 6:03 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
[..]
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index a5ce0bbeadb5..4bb086dbe7ec 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -127,6 +127,9 @@ struct cpufreq_policy {
>>          */
>>         unsigned int            transition_delay_us;
>>
>> +       /* Boost switch for tasks with p->in_iowait set */
>> +       unsigned int            iowait_boost_enable;
>
> Could that be a bool field?

Yes that would be better, will do.

thanks,

-Joel

Re: [PATCH 1/2] cpufreq: Make iowait boost a policy option

[Viresh Kumar]

On Fri, May 19, 2017 at 12:00 AM, Joel Fernandes <joelaf@google.com> wrote:
> Make iowait boost a cpufreq policy option and enable it for intel_pstate
> cpufreq driver. Governors like schedutil can use it to determine if
> boosting for tasks that wake up with p->in_iowait set is needed.
>
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 1 +
>  include/linux/cpufreq.h        | 3 +++
>  2 files changed, 4 insertions(+)

Hi Joel,

Please refer to MAINTAINERS file to find the list of people/list you need to
send these patches to. You can take help from scripts/get_maintainers.pl
as well.

--
viresh

Re: [PATCH 1/2] cpufreq: Make iowait boost a policy option

[Joel Fernandes]

On Thu, May 18, 2017 at 9:17 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, May 19, 2017 at 12:00 AM, Joel Fernandes <joelaf@google.com> wrote:
>> Make iowait boost a cpufreq policy option and enable it for intel_pstate
>> cpufreq driver. Governors like schedutil can use it to determine if
>> boosting for tasks that wake up with p->in_iowait set is needed.
>>
>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>> ---
>>  drivers/cpufreq/intel_pstate.c | 1 +
>>  include/linux/cpufreq.h        | 3 +++
>>  2 files changed, 4 insertions(+)
>
> Hi Joel,
>
> Please refer to MAINTAINERS file to find the list of people/list you need to
> send these patches to. You can take help from scripts/get_maintainers.pl
> as well.

Ok with me.

thanks,