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

Groups > linux.kernel > #1520054 > unrolled thread

Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier

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 1/3] cpufreq: schedutil: enable fast switch earlier "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-11 23:00 +0100
    Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-12 06:20 +0100
      Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-13 15:50 +0100
        Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-14 05:20 +0100
          Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-14 12:40 +0100

#1520054 — Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier

On Fri, Nov 11, 2016 at 11:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The fast_switch_enabled flag will be used a bit earlier while converting
> the schedutil governor to use kthread worker.
>
> Prepare for that by moving the call to enable it to the beginning of
> sugov_init().

Fair enough ->

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/sched/cpufreq_schedutil.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 69e06898997d..ccb2ab89affb 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -416,9 +416,13 @@ static int sugov_init(struct cpufreq_policy *policy)
>         if (policy->governor_data)
>                 return -EBUSY;
>
> +       cpufreq_enable_fast_switch(policy);
> +
>         sg_policy = sugov_policy_alloc(policy);
> -       if (!sg_policy)
> -               return -ENOMEM;
> +       if (!sg_policy) {
> +               ret = -ENOMEM;
> +               goto disable_fast_switch;
> +       }
>
>         mutex_lock(&global_tunables_lock);
>
> @@ -456,8 +460,6 @@ static int sugov_init(struct cpufreq_policy *policy)
>
>   out:
>         mutex_unlock(&global_tunables_lock);
> -
> -       cpufreq_enable_fast_switch(policy);
>         return 0;
>
>   fail:
> @@ -468,6 +470,10 @@ static int sugov_init(struct cpufreq_policy *policy)
>         mutex_unlock(&global_tunables_lock);
>
>         sugov_policy_free(sg_policy);
> +
> + disable_fast_switch:
> +       cpufreq_disable_fast_switch(policy);
> +
>         pr_err("initialization failed (error %d)\n", ret);
>         return ret;
>  }
> @@ -478,8 +484,6 @@ static void sugov_exit(struct cpufreq_policy *policy)
>         struct sugov_tunables *tunables = sg_policy->tunables;
>         unsigned int count;
>
> -       cpufreq_disable_fast_switch(policy);
> -

->but why is this change necessary?

sugov_stop() has been called already, so the ordering here shouldn't matter.

>         mutex_lock(&global_tunables_lock);
>
>         count = gov_attr_set_put(&tunables->attr_set, &sg_policy->tunables_hook);
> @@ -490,6 +494,7 @@ static void sugov_exit(struct cpufreq_policy *policy)
>         mutex_unlock(&global_tunables_lock);
>
>         sugov_policy_free(sg_policy);
> +       cpufreq_disable_fast_switch(policy);
>  }
>
>  static int sugov_start(struct cpufreq_policy *policy)
> --

Thanks,
Rafael

[toc] | [next] | [standalone]

#1520178

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

>> @@ -478,8 +484,6 @@ static void sugov_exit(struct cpufreq_policy *policy)
>>         struct sugov_tunables *tunables = sg_policy->tunables;
>>         unsigned int count;
>>
>> -       cpufreq_disable_fast_switch(policy);
>> -
>
> ->but why is this change necessary?
>
> sugov_stop() has been called already, so the ordering here shouldn't matter.

Because sugov_policy_free() would be using the flag fast_switch_enabled.

--
viresh

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

#1520608

On Sat, Nov 12, 2016 at 6:19 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12 November 2016 at 03:28, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
>>> @@ -478,8 +484,6 @@ static void sugov_exit(struct cpufreq_policy *policy)
>>>         struct sugov_tunables *tunables = sg_policy->tunables;
>>>         unsigned int count;
>>>
>>> -       cpufreq_disable_fast_switch(policy);
>>> -
>>
>> ->but why is this change necessary?
>>
>> sugov_stop() has been called already, so the ordering here shouldn't matter.
>
> Because sugov_policy_free() would be using the flag fast_switch_enabled.

That's only going to happen in the next patch, though, right?  It
wouldn't hurt to write that in the changelog too.

Besides, I'm not actually sure if starting/stopping the kthread in
sugov_policy_alloc/free() is a good idea.  It sort of conflates the
allocation of memory with kthread creation.  Any chance to untangle
that?

Thanks,
Rafael

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

#1521283

On 13-11-16, 15:46, Rafael J. Wysocki wrote:
> That's only going to happen in the next patch, though, right?  It
> wouldn't hurt to write that in the changelog too.

Sure.

> Besides, I'm not actually sure if starting/stopping the kthread in
> sugov_policy_alloc/free() is a good idea.  It sort of conflates the
> allocation of memory with kthread creation.  Any chance to untangle
> that?

Hmm, so either I can create two new routines for the thread and call
them along with alloc/free. Or I can rename the alloc/free routines
and keep this patch as is.

-- 
viresh

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

#1521561

On 14-11-16, 09:36, Viresh Kumar wrote:
> On 13-11-16, 15:46, Rafael J. Wysocki wrote:
> > That's only going to happen in the next patch, though, right?  It
> > wouldn't hurt to write that in the changelog too.
> 
> Sure.
> 
> > Besides, I'm not actually sure if starting/stopping the kthread in
> > sugov_policy_alloc/free() is a good idea.  It sort of conflates the
> > allocation of memory with kthread creation.  Any chance to untangle
> > that?
> 
> Hmm, so either I can create two new routines for the thread and call
> them along with alloc/free. Or I can rename the alloc/free routines
> and keep this patch as is.

I have created separate routines in my new version (which I will send
tomorrow).

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web