Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520064 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-11 23:20 +0100
    Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue  to SCHED_FIFO task Saravana Kannan <skannan@codeaurora.org> - 2016-11-12 02:40 +0100
      Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-12 06:30 +0100
        Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-14 06:40 +0100
      Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-13 15:40 +0100
        Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Steve Muckle <smuckle.linux@gmail.com> - 2016-11-13 20:50 +0100
          Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-13 23:50 +0100
            Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-14 07:40 +0100
    Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-12 06:30 +0100
    Re: [PATCH 2/3] cpufreq: schedutil: move slow path from workqueue to  SCHED_FIFO task Steve Muckle <smuckle.linux@gmail.com> - 2016-11-13 21:00 +0100

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

On Fri, Nov 11, 2016 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.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@gmail.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.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 ...

Thanks,
Rafael

[toc] | [next] | [standalone]

#1520144 — 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@linaro.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@gmail.com>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.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

[toc] | [prev] | [next] | [standalone]

#1520183

On 12 November 2016 at 07:01, Saravana Kannan <skannan@codeaurora.org> wrote:
> 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?

I wasn't there but AFAIU, this is the case we have currently for the schedutil
governor. And we (mobile world, Linaro) want to change that it doesn't work
that well for us. So perhaps it is just the opposite of what you stated.

> So, schedutil is going
> to trigger schedutil to bump up the frequency to max, right?

How is that question related to this patch ?

--
viresh

[toc] | [prev] | [next] | [standalone]

#1521328

On 12-11-16, 10:57, Viresh Kumar wrote:
> On 12 November 2016 at 07:01, Saravana Kannan <skannan@codeaurora.org> wrote:
> > 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?
> 
> I wasn't there but AFAIU, this is the case we have currently for the schedutil
> governor. And we (mobile world, Linaro) want to change that it doesn't work
> that well for us. So perhaps it is just the opposite of what you stated.
> 
> > So, schedutil is going
> > to trigger schedutil to bump up the frequency to max, right?
> 
> How is that question related to this patch ?

Trash my last email, I failed to read yours :(

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1520605

On Sat, Nov 12, 2016 at 2:31 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 11/11/2016 02:16 PM, Rafael J. Wysocki wrote:
>>
>> On Fri, Nov 11, 2016 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.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@gmail.com>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.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?

No, it isn't, or at least that is unlikely.

sugov_update_commit() sets sg_policy->work_in_progress before queuing
the IRQ work and it is not cleared until the frequency changes in
sugov_work().

OTOH, sugov_should_update_freq() checks sg_policy->work_in_progress
upfront and returns false when it is set, so the governor won't see
its own worker threads run, unless I'm overlooking something highly
non-obvious.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1520651

On Sun, Nov 13, 2016 at 03:37:18PM +0100, Rafael J. Wysocki wrote:
> > 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?
> 
> No, it isn't, or at least that is unlikely.
> 
> sugov_update_commit() sets sg_policy->work_in_progress before queuing
> the IRQ work and it is not cleared until the frequency changes in
> sugov_work().
> 
> OTOH, sugov_should_update_freq() checks sg_policy->work_in_progress
> upfront and returns false when it is set, so the governor won't see
> its own worker threads run, unless I'm overlooking something highly
> non-obvious.

FWIW my intention with the original version of this patch (which I
neglected to communicate to Viresh) was that it would depend on changing
the frequency policy for RT. I had been using rt_avg. It sounds like
during LPC there were talks of using another metric.

It does appear things would work okay without that but it also seems
a bit fragile. There's the window between when the work_in_progress
gets cleared and the RT kthread yields. I have not thought through the
various scenarios there, what is possible and tested to see if it is
significant enough to impact power-sensitive platforms.

thanks,
Steve

[toc] | [prev] | [next] | [standalone]

#1520718

On Sun, Nov 13, 2016 at 8:47 PM, Steve Muckle <smuckle.linux@gmail.com> wrote:
> On Sun, Nov 13, 2016 at 03:37:18PM +0100, Rafael J. Wysocki wrote:
>> > 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?
>>
>> No, it isn't, or at least that is unlikely.
>>
>> sugov_update_commit() sets sg_policy->work_in_progress before queuing
>> the IRQ work and it is not cleared until the frequency changes in
>> sugov_work().
>>
>> OTOH, sugov_should_update_freq() checks sg_policy->work_in_progress
>> upfront and returns false when it is set, so the governor won't see
>> its own worker threads run, unless I'm overlooking something highly
>> non-obvious.
>
> FWIW my intention with the original version of this patch (which I
> neglected to communicate to Viresh) was that it would depend on changing
> the frequency policy for RT. I had been using rt_avg. It sounds like
> during LPC there were talks of using another metric.
>
> It does appear things would work okay without that but it also seems
> a bit fragile.

Yes, it does.

To a minimum, there should be a comment regarding that in the patches.

> There's the window between when the work_in_progress
> gets cleared and the RT kthread yields. I have not thought through the
> various scenarios there, what is possible and tested to see if it is
> significant enough to impact power-sensitive platforms.

Well, me neither, to be entirely honest. :-)

That said, there is a limited number of call sites for
update_curr_rt(), where SCHED_CPUFREQ_RT is passed to cpufreq
governors: dequeue_task_rt(), put_prev_task_rt(), pick_next_task_rt(),
and task_tick_rt().  I'm not sure how pick_next_task_rt() can be
relevant here at all, though, and task_tick_rt() would need to be
running exactly during the window mentioned above, so it probably is
negligible either, at least on the average.

From the quick look at the scheduler core, put_prev_task() is mostly
called for running tasks, so that case doesn't look like something to
worry about too, although it would need to be looked through in
detail.  The dequeue part I'm totally unsure about.

In any case, the clearing of work_in_progress might still be deferred
by queuing a regular (non-RT) work item to do that from the kthread
work (that will guarantee "hiding" the kthread work from the
governor), but admittedly that would be a sledgehammer of sorts (and
it might defeat the purpose of the whole exercise) ...

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1521350

On 13-11-16, 23:44, Rafael J. Wysocki wrote:
> To a minimum, there should be a comment regarding that in the patches.

Wanted to get the comment written properly before sending that in the patch. Can
you please rectify this based on what you are looking for ?

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index c6bc60078e21..e43f4fd42fb4 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -313,6 +313,20 @@ 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);
+
+       /*
+        * For Real Time and Deadline tasks, schedutil governor shoots the
+        * frequency to maximum. And special care must be taken to ensure that
+        * this kthread doesn't result in that.
+        *
+        * This is (mostly) guaranteed by the work_in_progress flag. The flag is
+        * updated only at the end of the sugov_work() and before that schedutil
+        * rejects all other frequency scaling requests.
+        *
+        * Though there is a very rare case where the RT thread yields right
+        * after the work_in_progress flag is cleared. The effects of that are
+        * neglected for now.
+        */
        kthread_queue_work(&sg_policy->worker, &sg_policy->work);
 }
 
> In any case, the clearing of work_in_progress might still be deferred
> by queuing a regular (non-RT) work item to do that from the kthread
> work (that will guarantee "hiding" the kthread work from the
> governor), but admittedly that would be a sledgehammer of sorts (and
> it might defeat the purpose of the whole exercise) ...

I agree.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1520180

On 12 November 2016 at 03:46, Rafael J. Wysocki <rafael@kernel.org> wrote:

>> +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?

Mistake ..

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

Sure.

As I asked in the cover letter, will you be fine if I send the same patch
for ondemand/conservative governors ?

[toc] | [prev] | [next] | [standalone]

#1520653

On Fri, Nov 11, 2016 at 11:16:59PM +0100, Rafael J. Wysocki wrote:
> > +       struct sched_param param = { .sched_priority = 50 };
> 
> I'd define a symbol for the 50.  It's just one extra line of code ...

A minor point for sure, but in general what's the motivation for
defining symbols for things which are only used once? It makes it harder
to untangle and learn a piece of code IMO, having to jump to the
definitions of them to see what they are.

thanks,
Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web