linux-kernel - Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to SCHED

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, 11 Nov 2016 17:31:41 -0800
From:   Saravana Kannan <skannan@...eaurora.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
CC:     Viresh Kumar <viresh.kumar@...aro.org>,
        Rafael Wysocki <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Juri Lelli <Juri.Lelli@....com>,
        Robin Randhawa <robin.randhawa@....com>,
        Steve Muckle <smuckle.linux@...il.com>
Subject: Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue
 to SCHED_FIFO task

On 11/11/2016 02:16 PM, Rafael J. Wysocki wrote:
> On Fri, Nov 11, 2016 at 11:22 AM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>> If slow path frequency changes are conducted in a SCHED_OTHER context
>> then they may be delayed for some amount of time, including
>> indefinitely, when real time or deadline activity is taking place.
>>
>> Move the slow path to a real time kernel thread. In the future the
>> thread should be made SCHED_DEADLINE. The RT priority is arbitrarily set
>> to 50 for now.
>>
>> Hackbench results on ARM Exynos, dual core A15 platform for 10
>> iterations:
>>
>> $ hackbench -s 100 -l 100 -g 10 -f 20
>>
>> Before                  After
>> ---------------------------------
>> 1.808                   1.603
>> 1.847                   1.251
>> 2.229                   1.590
>> 1.952                   1.600
>> 1.947                   1.257
>> 1.925                   1.627
>> 2.694                   1.620
>> 1.258                   1.621
>> 1.919                   1.632
>> 1.250                   1.240
>>
>> Average:
>>
>> 1.8829                  1.5041
>>
>> Based on initial work by Steve Muckle.
>>
>> Signed-off-by: Steve Muckle <smuckle.linux@...il.com>
>> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
>> ---
>>   kernel/sched/cpufreq_schedutil.c | 62 ++++++++++++++++++++++++++++++++--------
>>   1 file changed, 50 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index ccb2ab89affb..045ce0a4e6d1 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -12,6 +12,7 @@
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>>   #include <linux/cpufreq.h>
>> +#include <linux/kthread.h>
>>   #include <linux/slab.h>
>>   #include <trace/events/power.h>
>>
>> @@ -35,8 +36,10 @@ struct sugov_policy {
>>
>>          /* The next fields are only needed if fast switch cannot be used. */
>>          struct irq_work irq_work;
>> -       struct work_struct work;
>> +       struct kthread_work work;
>>          struct mutex work_lock;
>> +       struct kthread_worker worker;
>> +       struct task_struct *thread;
>>          bool work_in_progress;
>>
>>          bool need_freq_update;
>> @@ -291,9 +294,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>>          raw_spin_unlock(&sg_policy->update_lock);
>>   }
>>
>> -static void sugov_work(struct work_struct *work)
>> +static void sugov_work(struct kthread_work *work)
>>   {
>> -       struct sugov_policy *sg_policy = container_of(work, struct sugov_policy, work);
>> +       struct sugov_policy *sg_policy =
>> +               container_of(work, struct sugov_policy, work);
>
> Why this change?
>
>>
>>          mutex_lock(&sg_policy->work_lock);
>>          __cpufreq_driver_target(sg_policy->policy, sg_policy->next_freq,
>> @@ -308,7 +312,7 @@ static void sugov_irq_work(struct irq_work *irq_work)
>>          struct sugov_policy *sg_policy;
>>
>>          sg_policy = container_of(irq_work, struct sugov_policy, irq_work);
>> -       schedule_work_on(smp_processor_id(), &sg_policy->work);
>> +       kthread_queue_work(&sg_policy->worker, &sg_policy->work);
>>   }
>>
>>   /************************** sysfs interface ************************/
>> @@ -362,9 +366,23 @@ static struct kobj_type sugov_tunables_ktype = {
>>
>>   static struct cpufreq_governor schedutil_gov;
>>
>> +static void sugov_policy_free(struct sugov_policy *sg_policy)
>> +{
>> +       if (!sg_policy->policy->fast_switch_enabled) {
>> +               kthread_flush_worker(&sg_policy->worker);
>> +               kthread_stop(sg_policy->thread);
>> +       }
>> +
>> +       mutex_destroy(&sg_policy->work_lock);
>> +       kfree(sg_policy);
>> +}
>> +
>>   static struct sugov_policy *sugov_policy_alloc(struct cpufreq_policy *policy)
>>   {
>>          struct sugov_policy *sg_policy;
>> +       struct task_struct *thread;
>> +       struct sched_param param = { .sched_priority = 50 };
>
> I'd define a symbol for the 50.  It's just one extra line of code ...
>

Hold on a sec. I thought during LPC someone (Peter?) made a point that 
when RT thread run, we should bump the frequency to max? So, schedutil 
is going to trigger schedutil to bump up the frequency to max, right?

-Saravana


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