The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
occasions:
- In sugov_start(), when the schedutil governor is started for a group
of CPUs.
- And whenever we need to force a freq update before rate-limit
duration, which happens when:
- there is an update in cpufreq policy limits.
- Or when the utilization of DL scheduling class increases.
In return, get_next_freq() doesn't return a cached next_freq value but
instead recalculates the next frequency. This has some side effects
though and may significantly delay a required increase in frequency.
In sugov_update_single() we try to avoid decreasing frequency if the CPU
has not been idle recently. Consider this scenario, the available range
of frequencies for a CPU are from 800 MHz to 2.5 GHz and current
frequency is 800 MHz. From one of the call paths
sg_policy->need_freq_update is set to true and hence
sg_policy->next_freq is set to UINT_MAX. Now if the CPU had been busy,
next_f will always be less than UINT_MAX, whatever the value of next_f
is. And so even when we wanted to increase the frequency, we will
overwrite next_f with UINT_MAX and will not change the frequency
eventually. This will continue until the time CPU stays busy. This isn't
cross checked with any specific test cases, but rather based on general
code review.
Fix that by not resetting the sg_policy->need_freq_update flag from
sugov_should_update_freq() but get_next_freq() and we wouldn't need to
overwrite sg_policy->next_freq anymore.
Cc: 4.12+ <[email protected]> # 4.12+
Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely")
Signed-off-by: Viresh Kumar <[email protected]>
---
kernel/sched/cpufreq_schedutil.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index d2c6083304b4..daaca23697dc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -95,15 +95,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
if (sg_policy->work_in_progress)
return false;
- if (unlikely(sg_policy->need_freq_update)) {
- sg_policy->need_freq_update = false;
- /*
- * This happens when limits change, so forget the previous
- * next_freq value and force an update.
- */
- sg_policy->next_freq = UINT_MAX;
+ if (unlikely(sg_policy->need_freq_update))
return true;
- }
delta_ns = time - sg_policy->last_freq_update_time;
@@ -165,8 +158,10 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
freq = (freq + (freq >> 2)) * util / max;
- if (freq == sg_policy->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
+ if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
return sg_policy->next_freq;
+
+ sg_policy->need_freq_update = false;
sg_policy->cached_raw_freq = freq;
return cpufreq_driver_resolve_freq(policy, freq);
}
@@ -670,7 +665,7 @@ static int sugov_start(struct cpufreq_policy *policy)
sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
sg_policy->last_freq_update_time = 0;
- sg_policy->next_freq = UINT_MAX;
+ sg_policy->next_freq = 0;
sg_policy->work_in_progress = false;
sg_policy->need_freq_update = false;
sg_policy->cached_raw_freq = 0;
--
2.15.0.194.g9af6a3dea062
On Tue, May 8, 2018 at 8:42 AM, Viresh Kumar <[email protected]> wrote:
> The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
> occasions:
> - In sugov_start(), when the schedutil governor is started for a group
> of CPUs.
> - And whenever we need to force a freq update before rate-limit
> duration, which happens when:
> - there is an update in cpufreq policy limits.
> - Or when the utilization of DL scheduling class increases.
>
> In return, get_next_freq() doesn't return a cached next_freq value but
> instead recalculates the next frequency. This has some side effects
> though and may significantly delay a required increase in frequency.
>
> In sugov_update_single() we try to avoid decreasing frequency if the CPU
> has not been idle recently. Consider this scenario, the available range
> of frequencies for a CPU are from 800 MHz to 2.5 GHz and current
> frequency is 800 MHz. From one of the call paths
> sg_policy->need_freq_update is set to true and hence
> sg_policy->next_freq is set to UINT_MAX. Now if the CPU had been busy,
> next_f will always be less than UINT_MAX, whatever the value of next_f
> is. And so even when we wanted to increase the frequency, we will
> overwrite next_f with UINT_MAX and will not change the frequency
> eventually. This will continue until the time CPU stays busy. This isn't
> cross checked with any specific test cases, but rather based on general
> code review.
>
> Fix that by not resetting the sg_policy->need_freq_update flag from
> sugov_should_update_freq() but get_next_freq() and we wouldn't need to
> overwrite sg_policy->next_freq anymore.
>
> Cc: 4.12+ <[email protected]> # 4.12+
> Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely")
The rest of the chantelog is totally disconnected from this commit.
So the problem is that sugov_update_single() doesn't check the special
UNIT_MAX value before assigning sg_policy->next_freq to next_f. Fair
enough.
I don't see why the patch is the right fix for that, however.
Thanks,
Rafael
On 08-05-18, 22:54, Rafael J. Wysocki wrote:
> On Tue, May 8, 2018 at 8:42 AM, Viresh Kumar <[email protected]> wrote:
> > The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
> > occasions:
> > - In sugov_start(), when the schedutil governor is started for a group
> > of CPUs.
> > - And whenever we need to force a freq update before rate-limit
> > duration, which happens when:
> > - there is an update in cpufreq policy limits.
> > - Or when the utilization of DL scheduling class increases.
> >
> > In return, get_next_freq() doesn't return a cached next_freq value but
> > instead recalculates the next frequency. This has some side effects
> > though and may significantly delay a required increase in frequency.
> >
> > In sugov_update_single() we try to avoid decreasing frequency if the CPU
> > has not been idle recently. Consider this scenario, the available range
> > of frequencies for a CPU are from 800 MHz to 2.5 GHz and current
> > frequency is 800 MHz. From one of the call paths
> > sg_policy->need_freq_update is set to true and hence
> > sg_policy->next_freq is set to UINT_MAX. Now if the CPU had been busy,
> > next_f will always be less than UINT_MAX, whatever the value of next_f
> > is. And so even when we wanted to increase the frequency, we will
> > overwrite next_f with UINT_MAX and will not change the frequency
> > eventually. This will continue until the time CPU stays busy. This isn't
> > cross checked with any specific test cases, but rather based on general
> > code review.
> >
> > Fix that by not resetting the sg_policy->need_freq_update flag from
> > sugov_should_update_freq() but get_next_freq() and we wouldn't need to
> > overwrite sg_policy->next_freq anymore.
> >
> > Cc: 4.12+ <[email protected]> # 4.12+
> > Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely")
>
> The rest of the chantelog is totally disconnected from this commit.
I added the "Fixes" tag because this is exactly the commit after which
this problem started, isn't it?
> So the problem is that sugov_update_single() doesn't check the special
> UNIT_MAX value before assigning sg_policy->next_freq to next_f. Fair
> enough.
>
> I don't see why the patch is the right fix for that, however.
I thought not overwriting next_freq makes things much simpler and easy
to review. What else do you have in mind to solve this problem ?
--
viresh
On Wed, May 9, 2018 at 10:41 AM, Viresh Kumar <[email protected]> wrote:
> On 08-05-18, 22:54, Rafael J. Wysocki wrote:
>> On Tue, May 8, 2018 at 8:42 AM, Viresh Kumar <[email protected]> wrote:
>> > The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
>> > occasions:
>> > - In sugov_start(), when the schedutil governor is started for a group
>> > of CPUs.
>> > - And whenever we need to force a freq update before rate-limit
>> > duration, which happens when:
>> > - there is an update in cpufreq policy limits.
>> > - Or when the utilization of DL scheduling class increases.
>> >
>> > In return, get_next_freq() doesn't return a cached next_freq value but
>> > instead recalculates the next frequency. This has some side effects
>> > though and may significantly delay a required increase in frequency.
>> >
>> > In sugov_update_single() we try to avoid decreasing frequency if the CPU
>> > has not been idle recently. Consider this scenario, the available range
>> > of frequencies for a CPU are from 800 MHz to 2.5 GHz and current
>> > frequency is 800 MHz. From one of the call paths
>> > sg_policy->need_freq_update is set to true and hence
>> > sg_policy->next_freq is set to UINT_MAX. Now if the CPU had been busy,
>> > next_f will always be less than UINT_MAX, whatever the value of next_f
>> > is. And so even when we wanted to increase the frequency, we will
>> > overwrite next_f with UINT_MAX and will not change the frequency
>> > eventually. This will continue until the time CPU stays busy. This isn't
>> > cross checked with any specific test cases, but rather based on general
>> > code review.
>> >
>> > Fix that by not resetting the sg_policy->need_freq_update flag from
>> > sugov_should_update_freq() but get_next_freq() and we wouldn't need to
>> > overwrite sg_policy->next_freq anymore.
>> >
>> > Cc: 4.12+ <[email protected]> # 4.12+
>> > Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely")
>>
>> The rest of the chantelog is totally disconnected from this commit.
>
> I added the "Fixes" tag because this is exactly the commit after which
> this problem started, isn't it?
>
>> So the problem is that sugov_update_single() doesn't check the special
>> UNIT_MAX value before assigning sg_policy->next_freq to next_f. Fair
>> enough.
>>
>> I don't see why the patch is the right fix for that, however.
>
> I thought not overwriting next_freq makes things much simpler and easy
> to review.
I'm kind of concerned about updating the limits via sysfs in which
case the cached next frequency may be out of range, so it's better to
invalidate it right away then.
> What else do you have in mind to solve this problem ?
Something like the below?
---
kernel/sched/cpufreq_schedutil.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -305,7 +305,8 @@ static void sugov_update_single(struct u
* Do not reduce the frequency if the CPU has not been idle
* recently, as the reduction is likely to be premature then.
*/
- if (busy && next_f < sg_policy->next_freq) {
+ if (busy && next_f < sg_policy->next_freq &&
+ sg_policy->next_freq != UINT_MAX) {
next_f = sg_policy->next_freq;
/* Reset cached freq as next_freq has changed */
On 09-05-18, 10:56, Rafael J. Wysocki wrote:
> I'm kind of concerned about updating the limits via sysfs in which
> case the cached next frequency may be out of range, so it's better to
> invalidate it right away then.
That should not be a problem as __cpufreq_driver_target() will anyway
clamp the target frequency to be within limits, whatever the cached
value of next_freq is.
And we aren't invalidating the cached next freq immediately currently
as well, as we are waiting until the next time the util update handler
is called to set sg_policy->next_freq to UINT_MAX.
> > What else do you have in mind to solve this problem ?
>
> Something like the below?
>
> ---
> kernel/sched/cpufreq_schedutil.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
> * Do not reduce the frequency if the CPU has not been idle
> * recently, as the reduction is likely to be premature then.
> */
> - if (busy && next_f < sg_policy->next_freq) {
> + if (busy && next_f < sg_policy->next_freq &&
> + sg_policy->next_freq != UINT_MAX) {
> next_f = sg_policy->next_freq;
>
> /* Reset cached freq as next_freq has changed */
This will fix the problem we have identified currently, but adding a
special meaning to next_freq == UINT_MAX invites more hidden corner
cases like the one we just found. IMHO, using next_freq only for the
*real* frequency values makes its usage more transparent and readable.
And we already have the need_freq_update flag which we can use for
this special purpose, as is done in my patch.
--
viresh
On Wed, May 9, 2018 at 11:15 AM, Viresh Kumar <[email protected]> wrote:
> On 09-05-18, 10:56, Rafael J. Wysocki wrote:
>> I'm kind of concerned about updating the limits via sysfs in which
>> case the cached next frequency may be out of range, so it's better to
>> invalidate it right away then.
>
> That should not be a problem as __cpufreq_driver_target() will anyway
> clamp the target frequency to be within limits, whatever the cached
> value of next_freq is.
The fast switch case doesn't use it, though.
> And we aren't invalidating the cached next freq immediately currently
> as well, as we are waiting until the next time the util update handler
> is called to set sg_policy->next_freq to UINT_MAX.
>
>> > What else do you have in mind to solve this problem ?
>>
>> Something like the below?
>>
>> ---
>> kernel/sched/cpufreq_schedutil.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
>> ===================================================================
>> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
>> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
>> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
>> * Do not reduce the frequency if the CPU has not been idle
>> * recently, as the reduction is likely to be premature then.
>> */
>> - if (busy && next_f < sg_policy->next_freq) {
>> + if (busy && next_f < sg_policy->next_freq &&
>> + sg_policy->next_freq != UINT_MAX) {
>> next_f = sg_policy->next_freq;
>>
>> /* Reset cached freq as next_freq has changed */
>
> This will fix the problem we have identified currently, but adding a
> special meaning to next_freq == UINT_MAX invites more hidden corner
> cases like the one we just found. IMHO, using next_freq only for the
> *real* frequency values makes its usage more transparent and readable.
> And we already have the need_freq_update flag which we can use for
> this special purpose, as is done in my patch.
So I prefer to do the above as a -stable fix and make the UNIT_MAX
change on top of that.
On 09-05-18, 11:23, Rafael J. Wysocki wrote:
> On Wed, May 9, 2018 at 11:15 AM, Viresh Kumar <[email protected]> wrote:
> > On 09-05-18, 10:56, Rafael J. Wysocki wrote:
> >> I'm kind of concerned about updating the limits via sysfs in which
> >> case the cached next frequency may be out of range, so it's better to
> >> invalidate it right away then.
> >
> > That should not be a problem as __cpufreq_driver_target() will anyway
> > clamp the target frequency to be within limits, whatever the cached
> > value of next_freq is.
>
> The fast switch case doesn't use it, though.
cpufreq_driver_fast_switch() does the same clamping :)
> > And we aren't invalidating the cached next freq immediately currently
> > as well, as we are waiting until the next time the util update handler
> > is called to set sg_policy->next_freq to UINT_MAX.
> >
> >> > What else do you have in mind to solve this problem ?
> >>
> >> Something like the below?
> >>
> >> ---
> >> kernel/sched/cpufreq_schedutil.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> >> ===================================================================
> >> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> >> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> >> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
> >> * Do not reduce the frequency if the CPU has not been idle
> >> * recently, as the reduction is likely to be premature then.
> >> */
> >> - if (busy && next_f < sg_policy->next_freq) {
> >> + if (busy && next_f < sg_policy->next_freq &&
> >> + sg_policy->next_freq != UINT_MAX) {
> >> next_f = sg_policy->next_freq;
> >>
> >> /* Reset cached freq as next_freq has changed */
> >
> > This will fix the problem we have identified currently, but adding a
> > special meaning to next_freq == UINT_MAX invites more hidden corner
> > cases like the one we just found. IMHO, using next_freq only for the
> > *real* frequency values makes its usage more transparent and readable.
> > And we already have the need_freq_update flag which we can use for
> > this special purpose, as is done in my patch.
>
> So I prefer to do the above as a -stable fix and make the UNIT_MAX
> change on top of that.
Okay, that's fine with me. Will send the next version now :)
Just to make sure, you are fine with the "Fixes" tag now (since you
objected to that earlier) ?
--
viresh
On Wed, May 9, 2018 at 11:30 AM, Viresh Kumar <[email protected]> wrote:
> On 09-05-18, 11:23, Rafael J. Wysocki wrote:
>> On Wed, May 9, 2018 at 11:15 AM, Viresh Kumar <[email protected]> wrote:
>> > On 09-05-18, 10:56, Rafael J. Wysocki wrote:
>> >> I'm kind of concerned about updating the limits via sysfs in which
>> >> case the cached next frequency may be out of range, so it's better to
>> >> invalidate it right away then.
>> >
>> > That should not be a problem as __cpufreq_driver_target() will anyway
>> > clamp the target frequency to be within limits, whatever the cached
>> > value of next_freq is.
>>
>> The fast switch case doesn't use it, though.
>
> cpufreq_driver_fast_switch() does the same clamping :)
>
>> > And we aren't invalidating the cached next freq immediately currently
>> > as well, as we are waiting until the next time the util update handler
>> > is called to set sg_policy->next_freq to UINT_MAX.
>> >
>> >> > What else do you have in mind to solve this problem ?
>> >>
>> >> Something like the below?
>> >>
>> >> ---
>> >> kernel/sched/cpufreq_schedutil.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
>> >> ===================================================================
>> >> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
>> >> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
>> >> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
>> >> * Do not reduce the frequency if the CPU has not been idle
>> >> * recently, as the reduction is likely to be premature then.
>> >> */
>> >> - if (busy && next_f < sg_policy->next_freq) {
>> >> + if (busy && next_f < sg_policy->next_freq &&
>> >> + sg_policy->next_freq != UINT_MAX) {
>> >> next_f = sg_policy->next_freq;
>> >>
>> >> /* Reset cached freq as next_freq has changed */
>> >
>> > This will fix the problem we have identified currently, but adding a
>> > special meaning to next_freq == UINT_MAX invites more hidden corner
>> > cases like the one we just found. IMHO, using next_freq only for the
>> > *real* frequency values makes its usage more transparent and readable.
>> > And we already have the need_freq_update flag which we can use for
>> > this special purpose, as is done in my patch.
>>
>> So I prefer to do the above as a -stable fix and make the UNIT_MAX
>> change on top of that.
>
> Okay, that's fine with me. Will send the next version now :)
>
> Just to make sure, you are fine with the "Fixes" tag now (since you
> objected to that earlier) ?
OK, so to be clear, I'm going to queue up the simple patch I posted
with a FIxes: tag. I'll resend it with a changelog shortly.
Then please send a UINT_MAX change on top of that and it won't be an
urgent fix any more.
On 09-05-18, 11:44, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> If the next_freq field of struct sugov_policy is set to UINT_MAX,
> it shouldn't be used for updating the CPU frequency (this is a
> special "invalid" value), but after commit b7eaf1aab9f8 (cpufreq:
> schedutil: Avoid reducing frequency of busy CPUs prematurely) it
> may be passed as the new frequency to sugov_update_commit() in
> sugov_update_single().
>
> Fix that by adding an extra check for the special UINT_MAX value
> of next_freq to sugov_update_single().
>
> Fixes: b7eaf1aab9f8 (cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely)
> Reported-by: Viresh Kumar <[email protected]>
> Cc: 4.12+ <[email protected]> # 4.12+
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> kernel/sched/cpufreq_schedutil.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
> * Do not reduce the frequency if the CPU has not been idle
> * recently, as the reduction is likely to be premature then.
> */
> - if (busy && next_f < sg_policy->next_freq) {
> + if (busy && next_f < sg_policy->next_freq &&
> + sg_policy->next_freq != UINT_MAX) {
> next_f = sg_policy->next_freq;
>
> /* Reset cached freq as next_freq has changed */
Acked-by: Viresh Kumar <[email protected]>
--
viresh
From: Rafael J. Wysocki <[email protected]>
If the next_freq field of struct sugov_policy is set to UINT_MAX,
it shouldn't be used for updating the CPU frequency (this is a
special "invalid" value), but after commit b7eaf1aab9f8 (cpufreq:
schedutil: Avoid reducing frequency of busy CPUs prematurely) it
may be passed as the new frequency to sugov_update_commit() in
sugov_update_single().
Fix that by adding an extra check for the special UINT_MAX value
of next_freq to sugov_update_single().
Fixes: b7eaf1aab9f8 (cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely)
Reported-by: Viresh Kumar <[email protected]>
Cc: 4.12+ <[email protected]> # 4.12+
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
kernel/sched/cpufreq_schedutil.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -305,7 +305,8 @@ static void sugov_update_single(struct u
* Do not reduce the frequency if the CPU has not been idle
* recently, as the reduction is likely to be premature then.
*/
- if (busy && next_f < sg_policy->next_freq) {
+ if (busy && next_f < sg_policy->next_freq &&
+ sg_policy->next_freq != UINT_MAX) {
next_f = sg_policy->next_freq;
/* Reset cached freq as next_freq has changed */
The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
occasions to discard the cached value of next freq:
- In sugov_start(), when the schedutil governor is started for a group
of CPUs.
- And whenever we need to force a freq update before rate-limit
duration, which happens when:
- there is an update in cpufreq policy limits.
- Or when the utilization of DL scheduling class increases.
In return, get_next_freq() doesn't return a cached next_freq value but
recalculates the next frequency instead.
But having special meaning for a particular value of frequency makes the
code less readable and error prone. We recently fixed a bug where the
UINT_MAX value was considered as valid frequency in
sugov_update_single().
All we need is a flag which can be used to discard the value of
sg_policy->next_freq and we already have need_freq_update for that. Lets
reuse it instead of setting next_freq to UINT_MAX.
Signed-off-by: Viresh Kumar <[email protected]>
---
V2:
- Rebased over the fix sent by Rafael
lkml.kernel.org/r/[email protected]
- Remove the additional check from sugov_update_single() as well.
- This is for 4.18 now instead of stable kernels.
kernel/sched/cpufreq_schedutil.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e23e84724f39..daaca23697dc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -95,15 +95,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
if (sg_policy->work_in_progress)
return false;
- if (unlikely(sg_policy->need_freq_update)) {
- sg_policy->need_freq_update = false;
- /*
- * This happens when limits change, so forget the previous
- * next_freq value and force an update.
- */
- sg_policy->next_freq = UINT_MAX;
+ if (unlikely(sg_policy->need_freq_update))
return true;
- }
delta_ns = time - sg_policy->last_freq_update_time;
@@ -165,8 +158,10 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
freq = (freq + (freq >> 2)) * util / max;
- if (freq == sg_policy->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
+ if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
return sg_policy->next_freq;
+
+ sg_policy->need_freq_update = false;
sg_policy->cached_raw_freq = freq;
return cpufreq_driver_resolve_freq(policy, freq);
}
@@ -305,8 +300,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
* Do not reduce the frequency if the CPU has not been idle
* recently, as the reduction is likely to be premature then.
*/
- if (busy && next_f < sg_policy->next_freq &&
- sg_policy->next_freq != UINT_MAX) {
+ if (busy && next_f < sg_policy->next_freq) {
next_f = sg_policy->next_freq;
/* Reset cached freq as next_freq has changed */
@@ -671,7 +665,7 @@ static int sugov_start(struct cpufreq_policy *policy)
sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
sg_policy->last_freq_update_time = 0;
- sg_policy->next_freq = UINT_MAX;
+ sg_policy->next_freq = 0;
sg_policy->work_in_progress = false;
sg_policy->need_freq_update = false;
sg_policy->cached_raw_freq = 0;
--
2.15.0.194.g9af6a3dea062
On Wed, May 09, 2018 at 04:05:24PM +0530, Viresh Kumar wrote:
> The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
> occasions to discard the cached value of next freq:
> - In sugov_start(), when the schedutil governor is started for a group
> of CPUs.
> - And whenever we need to force a freq update before rate-limit
> duration, which happens when:
> - there is an update in cpufreq policy limits.
> - Or when the utilization of DL scheduling class increases.
>
> In return, get_next_freq() doesn't return a cached next_freq value but
> recalculates the next frequency instead.
>
> But having special meaning for a particular value of frequency makes the
> code less readable and error prone. We recently fixed a bug where the
> UINT_MAX value was considered as valid frequency in
> sugov_update_single().
>
> All we need is a flag which can be used to discard the value of
> sg_policy->next_freq and we already have need_freq_update for that. Lets
> reuse it instead of setting next_freq to UINT_MAX.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> V2:
> - Rebased over the fix sent by Rafael
>
> lkml.kernel.org/r/[email protected]
>
> - Remove the additional check from sugov_update_single() as well.
> - This is for 4.18 now instead of stable kernels.
Reviewed-by: Joel Fernandes (Google) <[email protected]>
(please note my email address change as well in your contact/address-book).
thanks,
- Joel
>
> kernel/sched/cpufreq_schedutil.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e23e84724f39..daaca23697dc 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -95,15 +95,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> if (sg_policy->work_in_progress)
> return false;
>
> - if (unlikely(sg_policy->need_freq_update)) {
> - sg_policy->need_freq_update = false;
> - /*
> - * This happens when limits change, so forget the previous
> - * next_freq value and force an update.
> - */
> - sg_policy->next_freq = UINT_MAX;
> + if (unlikely(sg_policy->need_freq_update))
> return true;
> - }
>
> delta_ns = time - sg_policy->last_freq_update_time;
>
> @@ -165,8 +158,10 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>
> freq = (freq + (freq >> 2)) * util / max;
>
> - if (freq == sg_policy->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
> + if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
> return sg_policy->next_freq;
> +
> + sg_policy->need_freq_update = false;
> sg_policy->cached_raw_freq = freq;
> return cpufreq_driver_resolve_freq(policy, freq);
> }
> @@ -305,8 +300,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> * Do not reduce the frequency if the CPU has not been idle
> * recently, as the reduction is likely to be premature then.
> */
> - if (busy && next_f < sg_policy->next_freq &&
> - sg_policy->next_freq != UINT_MAX) {
> + if (busy && next_f < sg_policy->next_freq) {
> next_f = sg_policy->next_freq;
>
> /* Reset cached freq as next_freq has changed */
> @@ -671,7 +665,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>
> sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
> sg_policy->last_freq_update_time = 0;
> - sg_policy->next_freq = UINT_MAX;
> + sg_policy->next_freq = 0;
> sg_policy->work_in_progress = false;
> sg_policy->need_freq_update = false;
> sg_policy->cached_raw_freq = 0;
On Friday, May 11, 2018 10:47:12 PM CEST Joel Fernandes wrote:
> On Wed, May 09, 2018 at 04:05:24PM +0530, Viresh Kumar wrote:
> > The schedutil driver sets sg_policy->next_freq to UINT_MAX on certain
> > occasions to discard the cached value of next freq:
> > - In sugov_start(), when the schedutil governor is started for a group
> > of CPUs.
> > - And whenever we need to force a freq update before rate-limit
> > duration, which happens when:
> > - there is an update in cpufreq policy limits.
> > - Or when the utilization of DL scheduling class increases.
> >
> > In return, get_next_freq() doesn't return a cached next_freq value but
> > recalculates the next frequency instead.
> >
> > But having special meaning for a particular value of frequency makes the
> > code less readable and error prone. We recently fixed a bug where the
> > UINT_MAX value was considered as valid frequency in
> > sugov_update_single().
> >
> > All we need is a flag which can be used to discard the value of
> > sg_policy->next_freq and we already have need_freq_update for that. Lets
> > reuse it instead of setting next_freq to UINT_MAX.
> >
> > Signed-off-by: Viresh Kumar <[email protected]>
> > ---
> > V2:
> > - Rebased over the fix sent by Rafael
> >
> > lkml.kernel.org/r/[email protected]
> >
> > - Remove the additional check from sugov_update_single() as well.
> > - This is for 4.18 now instead of stable kernels.
>
> Reviewed-by: Joel Fernandes (Google) <[email protected]>
Applied, thanks!