| Message ID | 1acfffe798c0371e69ec1171f485499e7b49ed6d.1478858983.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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).
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)
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(-)