* [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change
@ 2017-07-12 2:24 Saravana Kannan
2017-07-12 5:24 ` Viresh Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Saravana Kannan @ 2017-07-12 2:24 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar; +Cc: linux-pm, linux-kernel
Currently, the governor calculates the next frequency, set the current CPU
frequency (policy->cur). It also assumes the current CPU frequency doesn't
change if the next frequency isn't calculated again and hence caches the
"current frequency".
However, this isn't true when CPU min/max frequency limits are changed. So,
there's room for the CPU frequency to get stuck at the wrong level if the
calculated next frequency doesn't change across multiple limits updates.
Fix this by updating the cached "current frequency" when limits changes the
current CPU frequency.
Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
kernel/sched/cpufreq_schedutil.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 076a2e3..fe0b2fb 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -226,6 +226,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
busy = sugov_cpu_is_busy(sg_cpu);
+ raw_spin_lock(&sg_policy->update_lock);
if (flags & SCHED_CPUFREQ_RT_DL) {
next_f = policy->cpuinfo.max_freq;
} else {
@@ -240,6 +241,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
next_f = sg_policy->next_freq;
}
sugov_update_commit(sg_policy, time, next_f);
+ raw_spin_unlock(&sg_policy->update_lock);
}
static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
@@ -637,10 +639,14 @@ static void sugov_stop(struct cpufreq_policy *policy)
static void sugov_limits(struct cpufreq_policy *policy)
{
struct sugov_policy *sg_policy = policy->governor_data;
+ unsigned long flags;
if (!policy->fast_switch_enabled) {
mutex_lock(&sg_policy->work_lock);
cpufreq_policy_apply_limits(policy);
+ raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
+ sg_policy->next_freq = policy->cur;
+ raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
mutex_unlock(&sg_policy->work_lock);
}
--
~/caf-sig.txt
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change
2017-07-12 2:24 [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change Saravana Kannan
@ 2017-07-12 5:24 ` Viresh Kumar
2017-07-12 17:44 ` Saravana Kannan
2017-07-14 1:52 ` Saravana Kannan
0 siblings, 2 replies; 5+ messages in thread
From: Viresh Kumar @ 2017-07-12 5:24 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel
On 11-07-17, 19:24, Saravana Kannan wrote:
> Currently, the governor calculates the next frequency, set the current CPU
> frequency (policy->cur). It also assumes the current CPU frequency doesn't
> change if the next frequency isn't calculated again and hence caches the
> "current frequency".
>
> However, this isn't true when CPU min/max frequency limits are changed. So,
> there's room for the CPU frequency to get stuck at the wrong level if the
> calculated next frequency doesn't change across multiple limits updates.
>
> Fix this by updating the cached "current frequency" when limits changes the
> current CPU frequency.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
> kernel/sched/cpufreq_schedutil.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 076a2e3..fe0b2fb 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -226,6 +226,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>
> busy = sugov_cpu_is_busy(sg_cpu);
>
> + raw_spin_lock(&sg_policy->update_lock);
> if (flags & SCHED_CPUFREQ_RT_DL) {
> next_f = policy->cpuinfo.max_freq;
> } else {
> @@ -240,6 +241,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> next_f = sg_policy->next_freq;
> }
> sugov_update_commit(sg_policy, time, next_f);
> + raw_spin_unlock(&sg_policy->update_lock);
We wouldn't allow locking here until the time we can :)
> }
>
> static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
> @@ -637,10 +639,14 @@ static void sugov_stop(struct cpufreq_policy *policy)
> static void sugov_limits(struct cpufreq_policy *policy)
> {
> struct sugov_policy *sg_policy = policy->governor_data;
> + unsigned long flags;
>
> if (!policy->fast_switch_enabled) {
> mutex_lock(&sg_policy->work_lock);
> cpufreq_policy_apply_limits(policy);
> + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
> + sg_policy->next_freq = policy->cur;
> + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
> mutex_unlock(&sg_policy->work_lock);
> }
Did you miss the following part which is after the closing } here ?
sg_policy->need_freq_update = true;
As this should already take care of the problem you are worried about. Or did I
misunderstood your problem completely ?
--
viresh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change
2017-07-12 5:24 ` Viresh Kumar
@ 2017-07-12 17:44 ` Saravana Kannan
2017-07-14 1:52 ` Saravana Kannan
1 sibling, 0 replies; 5+ messages in thread
From: Saravana Kannan @ 2017-07-12 17:44 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel
On 07/11/2017 10:24 PM, Viresh Kumar wrote:
> On 11-07-17, 19:24, Saravana Kannan wrote:
>> Currently, the governor calculates the next frequency, set the current CPU
>> frequency (policy->cur). It also assumes the current CPU frequency doesn't
>> change if the next frequency isn't calculated again and hence caches the
>> "current frequency".
>>
>> However, this isn't true when CPU min/max frequency limits are changed. So,
>> there's room for the CPU frequency to get stuck at the wrong level if the
>> calculated next frequency doesn't change across multiple limits updates.
>>
>> Fix this by updating the cached "current frequency" when limits changes the
>> current CPU frequency.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>> kernel/sched/cpufreq_schedutil.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 076a2e3..fe0b2fb 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -226,6 +226,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>>
>> busy = sugov_cpu_is_busy(sg_cpu);
>>
>> + raw_spin_lock(&sg_policy->update_lock);
>> if (flags & SCHED_CPUFREQ_RT_DL) {
>> next_f = policy->cpuinfo.max_freq;
>> } else {
>> @@ -240,6 +241,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>> next_f = sg_policy->next_freq;
>> }
>> sugov_update_commit(sg_policy, time, next_f);
>> + raw_spin_unlock(&sg_policy->update_lock);
>
> We wouldn't allow locking here until the time we can :)
>
>> }
>>
>> static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
>> @@ -637,10 +639,14 @@ static void sugov_stop(struct cpufreq_policy *policy)
>> static void sugov_limits(struct cpufreq_policy *policy)
>> {
>> struct sugov_policy *sg_policy = policy->governor_data;
>> + unsigned long flags;
>>
>> if (!policy->fast_switch_enabled) {
>> mutex_lock(&sg_policy->work_lock);
>> cpufreq_policy_apply_limits(policy);
>> + raw_spin_lock_irqsave(&sg_policy->update_lock, flags);
>> + sg_policy->next_freq = policy->cur;
>> + raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
>> mutex_unlock(&sg_policy->work_lock);
>> }
>
> Did you miss the following part which is after the closing } here ?
>
> sg_policy->need_freq_update = true;
>
> As this should already take care of the problem you are worried about. Or did I
> misunderstood your problem completely ?
>
Yup, I did. Thanks! Ignore patch please.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change
2017-07-12 5:24 ` Viresh Kumar
2017-07-12 17:44 ` Saravana Kannan
@ 2017-07-14 1:52 ` Saravana Kannan
2017-07-14 4:05 ` Viresh Kumar
1 sibling, 1 reply; 5+ messages in thread
From: Saravana Kannan @ 2017-07-14 1:52 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel
On 07/11/2017 10:24 PM, Viresh Kumar wrote:
> On 11-07-17, 19:24, Saravana Kannan wrote:
>> Currently, the governor calculates the next frequency, set the current CPU
>> frequency (policy->cur). It also assumes the current CPU frequency doesn't
>> change if the next frequency isn't calculated again and hence caches the
>> "current frequency".
>>
>> However, this isn't true when CPU min/max frequency limits are changed. So,
>> there's room for the CPU frequency to get stuck at the wrong level if the
>> calculated next frequency doesn't change across multiple limits updates.
>>
>> Fix this by updating the cached "current frequency" when limits changes the
>> current CPU frequency.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>> kernel/sched/cpufreq_schedutil.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 076a2e3..fe0b2fb 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -226,6 +226,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>>
>> busy = sugov_cpu_is_busy(sg_cpu);
>>
>> + raw_spin_lock(&sg_policy->update_lock);
>> if (flags & SCHED_CPUFREQ_RT_DL) {
>> next_f = policy->cpuinfo.max_freq;
>> } else {
>> @@ -240,6 +241,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>> next_f = sg_policy->next_freq;
>> }
>> sugov_update_commit(sg_policy, time, next_f);
>> + raw_spin_unlock(&sg_policy->update_lock);
>
> We wouldn't allow locking here until the time we can :)
>
Actually, can you clarify why you can't allow locking here?
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change
2017-07-14 1:52 ` Saravana Kannan
@ 2017-07-14 4:05 ` Viresh Kumar
0 siblings, 0 replies; 5+ messages in thread
From: Viresh Kumar @ 2017-07-14 4:05 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel
On 13-07-17, 18:52, Saravana Kannan wrote:
> On 07/11/2017 10:24 PM, Viresh Kumar wrote:
> >On 11-07-17, 19:24, Saravana Kannan wrote:
> >>Currently, the governor calculates the next frequency, set the current CPU
> >>frequency (policy->cur). It also assumes the current CPU frequency doesn't
> >>change if the next frequency isn't calculated again and hence caches the
> >>"current frequency".
> >>
> >>However, this isn't true when CPU min/max frequency limits are changed. So,
> >>there's room for the CPU frequency to get stuck at the wrong level if the
> >>calculated next frequency doesn't change across multiple limits updates.
> >>
> >>Fix this by updating the cached "current frequency" when limits changes the
> >>current CPU frequency.
> >>
> >>Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> >>---
> >> kernel/sched/cpufreq_schedutil.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >>diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> >>index 076a2e3..fe0b2fb 100644
> >>--- a/kernel/sched/cpufreq_schedutil.c
> >>+++ b/kernel/sched/cpufreq_schedutil.c
> >>@@ -226,6 +226,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> >>
> >> busy = sugov_cpu_is_busy(sg_cpu);
> >>
> >>+ raw_spin_lock(&sg_policy->update_lock);
> >> if (flags & SCHED_CPUFREQ_RT_DL) {
> >> next_f = policy->cpuinfo.max_freq;
> >> } else {
> >>@@ -240,6 +241,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> >> next_f = sg_policy->next_freq;
> >> }
> >> sugov_update_commit(sg_policy, time, next_f);
> >>+ raw_spin_unlock(&sg_policy->update_lock);
> >
> >We wouldn't allow locking here until the time we can :)
> >
>
> Actually, can you clarify why you can't allow locking here?
Because we want this to be quick and there is no need of locking here
as this is getting used only by one CPU. Nothing else :)
--
viresh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-07-14 4:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-12 2:24 [PATCH] cpufreq: schedutil: Update cached "current frequency" when limits change Saravana Kannan
2017-07-12 5:24 ` Viresh Kumar
2017-07-12 17:44 ` Saravana Kannan
2017-07-14 1:52 ` Saravana Kannan
2017-07-14 4:05 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).