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

Groups > linux.kernel > #1519667 > unrolled thread

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

Started byViresh Kumar <viresh.kumar@linaro.org>
First post2016-11-11 11:30 +0100
Last post2016-11-14 12:40 +0100
Articles 9 — 3 participants

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

  [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-11 11:30 +0100
    Re: [PATCH 1/3] cpufreq: schedutil: enable fast switch earlier Peter Zijlstra <peterz@infradead.org> - 2016-11-11 15:30 +0100
      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 Peter Zijlstra <peterz@infradead.org> - 2016-11-12 00:00 +0100
    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

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

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-11 11:30 +0100
Subject[PATCH 1/3] cpufreq: schedutil: enable fast switch earlier
Message-ID<sChfQ-35W-15@gated-at.bofh.it>
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().

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);
-
 	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)
-- 
2.7.1.410.g6faf27b

[toc] | [next] | [standalone]

#1519823

FromPeter Zijlstra <peterz@infradead.org>
Date2016-11-11 15:30 +0100
Message-ID<sCl05-5tz-5@gated-at.bofh.it>
In reply to#1519667
On Fri, Nov 11, 2016 at 03:52:21PM +0530, Viresh Kumar wrote:
> @@ -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;
>  }

Argh, no indented labels please. Please fix the 3 that snuck in while
you're there.

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

#1520052

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-11-11 23:00 +0100
Message-ID<sCs1z-1sJ-9@gated-at.bofh.it>
In reply to#1519823
On Fri, Nov 11, 2016 at 3:19 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Nov 11, 2016 at 03:52:21PM +0530, Viresh Kumar wrote:
>> @@ -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;
>>  }
>
> Argh, no indented labels please. Please fix the 3 that snuck in while
> you're there.

Well, you didn't tell me you didn't like them. :-)

Anyway, I can fix this up easily enough.

Any other concerns regarding the patch?

Thanks,
Rafael

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

#1520089

FromPeter Zijlstra <peterz@infradead.org>
Date2016-11-12 00:00 +0100
Message-ID<sCsXE-27K-21@gated-at.bofh.it>
In reply to#1520052
On Fri, Nov 11, 2016 at 10:52:27PM +0100, Rafael J. Wysocki wrote:
> On Fri, Nov 11, 2016 at 3:19 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Nov 11, 2016 at 03:52:21PM +0530, Viresh Kumar wrote:
> >> @@ -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;
> >>  }
> >
> > Argh, no indented labels please. Please fix the 3 that snuck in while
> > you're there.
> 
> Well, you didn't tell me you didn't like them. :-)
> 
> Anyway, I can fix this up easily enough.
> 
> Any other concerns regarding the patch?

No, looked fine I think, as did the others.

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

#1520054

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-11-11 23:00 +0100
Message-ID<sCs1A-1sJ-25@gated-at.bofh.it>
In reply to#1519667
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] | [prev] | [next] | [standalone]

#1520178

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-12 06:20 +0100
Message-ID<sCyTo-653-3@gated-at.bofh.it>
In reply to#1520054
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

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-11-13 15:50 +0100
Message-ID<sD4gy-1xq-15@gated-at.bofh.it>
In reply to#1520178
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

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-14 05:20 +0100
Message-ID<sDgUp-1Sy-1@gated-at.bofh.it>
In reply to#1520608
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

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-14 12:40 +0100
Message-ID<sDnMd-6mY-3@gated-at.bofh.it>
In reply to#1521283
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