Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to SCHED_FIFO task

[Saravana Kannan]

On 11/11/2016 02:16 PM, Rafael J. Wysocki wrote:
> On Fri, Nov 11, 2016 at 11:22 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >   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