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] [day] [month] [year] [list]
Date:   Tue, 11 Jul 2017 12:02:04 -0700
From:   Saravana Kannan <skannan@...eaurora.org>
To:     Joel Fernandes <joelaf@...gle.com>
CC:     Viresh Kumar <viresh.kumar@...aro.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <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 05/19/2017 09:10 AM, Joel Fernandes wrote:
> Hi Viresh,
>
> On Thu, May 18, 2017 at 11:50 PM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>> 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.
>
> Yes, I somehow only picked up 'bool' from your comment.  I'll drop the
> '_enable' in the next version. Sorry and thanks.
>
>>>   };
>>>
>>>   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.
>
> I felt it is good to leave it to the system designer and have the
> policy set a 'default' value, so incase it isn't touched by the
> designer from userspace, then the policy default is fine, and if the
> designer chooses to change it then he has the option to. This is also
> how we currently set the rate limits for schedutil in android. I don't
> feel strongly about one way or the other and if the general consensus
> is to drop this part then I'm fine. I'm curious to know what others
> think as well though.
>

Acked-by: Saravana Kannan <skannan@...eaurora.org>



-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project