sched and cpufreq fixes/cleanups

Updating CPU frequency on last dequeue of a CPU is useless. Because the utilization since CPU came out of idle can increase till the last dequeue, this means we are requesting for a higher frequency before entering idle which is not very meaningful or useful. It causes unwanted wakeups of the schedutil governor kthread in slow-switch systems resulting in large number of wake ups that could have been avoided. In an Android application playing music where the music app's thread wakes up and sleeps periodically on an Android device, its seen that the frequency increases slightly on the dequeue and is reduced when the task wakes up again. This oscillation continues between 300Mhz and 350Mhz, and while the task is running, its at 300MHz the whole time. This is pointless. Adding to that, these are unnecessary wake ups. Infact most of the time when the sugov thread wakes up, all the CPUs are idle - so it can hurt power by disturbing the cluster when it is idling.

This patch prevents a frequency update on the last dequeue. With this the number of schedutil governor thread wake ups are reduces more than 2 times (1389 -> 527).

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 kernel/sched/fair.c  | 25 ++++++++++++++++++++++---
 kernel/sched/sched.h |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

Quoted 23 lines · Joel Fernandes

Updating CPU frequency on last dequeue of a CPU is useless. Because the utilization since CPU came out of idle can increase till the last dequeue, this means we are requesting for a higher frequency before entering idle which is not very meaningful or useful. It causes unwanted wakeups of the schedutil governor kthread in slow-switch systems resulting in large number of wake ups that could have been avoided. In an Android application playing music where the music app's thread wakes up and sleeps periodically on an Android device, its seen that the frequency increases slightly on the dequeue and is reduced when the task wakes up again. This oscillation continues between 300Mhz and 350Mhz, and while the task is running, its at 300MHz the whole time. This is pointless. Adding to that, these are unnecessary wake ups. Infact most of the time when the sugov thread wakes up, all the CPUs are idle - so it can hurt power by disturbing the cluster when it is idling.

This patch prevents a frequency update on the last dequeue. With this the number of schedutil governor thread wake ups are reduces more than 2 times (1389 -> 527).

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

Quoted 4 lines

---
 kernel/sched/fair.c  | 25 ++++++++++++++++++++++---
 kernel/sched/sched.h |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

So you are doing this only for CFS, isn't that required for RT/DL as well?

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

        if (cpu_idle())
                "Don't change the freq";

Will something like that work?

Quoted 7 lines

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

         if (cpu_idle())
                 "Don't change the freq";

Will something like that work?

I thought about this and I think it wont work very well. In the dequeue path we're still running the task being dequeued so the CPU is not yet idle. What is needed here IMO is a notion that the CPU is possibly about to idle and we can get predict that from the dequeue path of the CFS class.

Also just looking at whether the CPU is currently idle or not in the governor doesn't help to differentiate between say the dequeue path / tick path. Both of which can occur when the CPU is not idle.

Any thoughts about this?

Quoted 20 lines · Joel Fernandes

Quoted 8 lines

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

         if (cpu_idle())
                 "Don't change the freq";

Will something like that work?

I thought about this and I think it wont work very well. In the dequeue path we're still running the task being dequeued so the CPU is not yet idle. What is needed here IMO is a notion that the CPU is possibly about to idle and we can get predict that from the dequeue path of the CFS class.

Also just looking at whether the CPU is currently idle or not in the governor doesn't help to differentiate between say the dequeue path / tick path. Both of which can occur when the CPU is not idle.

Quoted 1 line

Any thoughts about this?

Also if it really is the case that this bit of policy is universally desirable, I'd think it is better to do this in the scheduler and avoid a needless trip through a fn pointer out to schedutil for performance reasons.

Quoted 24 lines · Steve Muckle

Quoted 21 lines · Joel Fernandes

Quoted 8 lines

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

         if (cpu_idle())
                 "Don't change the freq";

Will something like that work?

I thought about this and I think it wont work very well. In the dequeue path we're still running the task being dequeued so the CPU is not yet idle. What is needed here IMO is a notion that the CPU is possibly about to idle and we can get predict that from the dequeue path of the CFS class.

Also just looking at whether the CPU is currently idle or not in the governor doesn't help to differentiate between say the dequeue path / tick path. Both of which can occur when the CPU is not idle.

Any thoughts about this?

Quoted 3 lines

Also if it really is the case that this bit of policy is universally desirable, I'd think it is better to do this in the scheduler and avoid a needless trip through a fn pointer out to schedutil for performance reasons.

I agree.

Peter, what do you think? Are you Ok with the approach of this patch (preventing of the cpufreq update call during dequeue)?

thanks,

- Joel

Hi Joel,

Quoted 25 lines · Joel Fernandes

Quoted 24 lines · Steve Muckle

Quoted 21 lines · Joel Fernandes

Quoted 8 lines

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

         if (cpu_idle())
                 "Don't change the freq";

Will something like that work?

I thought about this and I think it wont work very well. In the dequeue path we're still running the task being dequeued so the CPU is not yet idle. What is needed here IMO is a notion that the CPU is possibly about to idle and we can get predict that from the dequeue path of the CFS class.

Also just looking at whether the CPU is currently idle or not in the governor doesn't help to differentiate between say the dequeue path / tick path. Both of which can occur when the CPU is not idle.

Any thoughts about this?

Quoted 8 lines

Quoted 3 lines

Also if it really is the case that this bit of policy is universally desirable, I'd think it is better to do this in the scheduler and avoid a needless trip through a fn pointer out to schedutil for performance reasons.

I agree.

Peter, what do you think? Are you Ok with the approach of this patch (preventing of the cpufreq update call during dequeue)?

I'm not really convinced that we should do change OPP at dequeue. Although i agree that it makes perfect sense to prevent increasing OPP just before going idle for mp3 playback, i'm not sure that this is always the case especially for performance oriented use case because we delay the OPP increase and as a result the responsiveness of the CPU
In fact this decision really depends about how long we are going to sleep. If the cpu will wakes up in few ms, it's worth already increasing the frequency when utilization is above the threshold because it will not decreases enough to go back to lower OPP. At the opposite, if the cpu will not wake up shortly skipping OPP change can make sense.

Regarding the reduction of the number of OPP changes, will the util_est feature provide the same amount of reduction by directly providing the estimated max utilization ?

Just to say that IMHO skipping or not OPP change at dequeue is a policy decision and not a generic one

Hi Joel,

Quoted 29 lines · Joel Fernandes

Quoted 27 lines · Steve Muckle

Quoted 16 lines · Joel Fernandes

Quoted 8 lines

Also, this more looks like a policy decision. Will it be better to put that directly into schedutil? Like this:

         if (cpu_idle())
                 "Don't change the freq";

Will something like that work?

I thought about this and I think it wont work very well. In the dequeue path we're still running the task being dequeued so the CPU is not yet idle. What is needed here IMO is a notion that the CPU is possibly about to idle and we can get predict that from the dequeue path of the CFS class.

Quoted 5 lines

Also just looking at whether the CPU is currently idle or not in the governor doesn't help to differentiate between say the dequeue path / tick path. Both of which can occur when the CPU is not idle.

Any thoughts about this?

Also if it really is the case that this bit of policy is universally desirable, I'd think it is better to do this in the scheduler and avoid a needless trip through a fn pointer out to schedutil for performance reasons.

Quoted 4 lines

I agree.

Peter, what do you think? Are you Ok with the approach of this patch (preventing of the cpufreq update call during dequeue)?

I'm not really convinced that we should do change OPP at dequeue.

Although i agree that it makes perfect sense to prevent increasing OPP just before going idle for mp3 playback, i'm not sure that this is always the case especially for performance oriented use case because we delay the OPP increase and as a result the responsiveness of the CPU

In fact this decision really depends about how long we are going to sleep. If the cpu will wakes up in few ms, it's worth already increasing the frequency when utilization is above the threshold because it will not decreases enough to go back to lower OPP. At the opposite, if the cpu will not wake up shortly skipping OPP change can make sense.

Regarding the reduction of the number of OPP changes, will the util_est feature provide the same amount of reduction by directly providing the estimated max utilization ?

Just to say that IMHO skipping or not OPP change at dequeue is a policy decision and not a generic one

Since the recent remote cpufreq callback work, its possible that a cpufreq update is triggered from a remote CPU. For single policies however, the current code uses the local CPU when trying to determine if the remote sg_cpu entered idle or is busy. This is incorrect. To remedy this, compare with the nohz tick idle_calls counter of the remote CPU.

Cc Rafael J. Wysocki <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 include/linux/tick.h             |  1 +
 kernel/sched/cpufreq_schedutil.c |  2 +-
 kernel/time/tick-sched.c         | 13 +++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

Since the remote cpufreq callback work, the cpufreq_update_util call can happen from remote CPUs. The comment about local CPUs is thus obsolete. Update it accordingly.

Cc Viresh Kumar <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 kernel/sched/fair.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/sched/fair.c +0 -0

index 4c06e52935d3..5c49fdb4c508 100644

You have prefixed most of the Cc'd names with "Cc: " somehow :)

Quoted 3 lines

Since the remote cpufreq callback work, the cpufreq_update_util call can happen from remote CPUs. The comment about local CPUs is thus obsolete. Update it accordingly.

We normally keep the column width as 72 in commit logs instead of 80, as with 'git log' this is indented by a tab and then we would cross 80 columns.

Quoted 19 lines

Cc Viresh Kumar <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 kernel/sched/fair.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/sched/fair.c +0 -1

index 4c06e52935d3..5c49fdb4c508 100644
@@ -3018,9 +3018,7 @@ static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq)
              /*
               * There are a few boundary cases this might miss but it should
               * get called often enough that that should (hopefully) not be
-              * a real problem -- added to that it only calls on the local

kernel/sched/fair.c static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq) · line 3024

Quoted 6 lines

kernel/sched/fair.c +1 -2

-              * CPU, so if we enqueue remotely we'll miss an update, but
-              * the next tick/schedule should update.
+              * a real problem.
               *
               * It will not get called when we go idle, because the idle
               * thread is a different class (!fair), nor will the utilization

Reviewed-by Viresh Kumar <[email protected]>

find_idlest_group_cpu goes through CPUs of a group previous selected by find_idlest_group. find_idlest_group returns NULL if the local group is the selected one and doesn't execute find_idlest_group_cpu if the group to which 'cpu' belongs to is chosen. So we're always guaranteed to call find_idlest_group_cpu with a group to which cpu is non-local. This makes one of the conditions in find_idlest_group_cpu an impossible one, which we can get rid off.

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Brendan Jackman <[email protected]>

Cc Dietmar <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched/fair.c +1 -1

index 5c49fdb4c508..740602ce799f 100644
@@ -5922,7 +5922,7 @@ find_idlest_group_cpu(struct sched_group *group, struct task_struct *p, int this
 			}
 		} else if (shallowest_idle_cpu == -1) {
 			load = weighted_cpuload(cpu_rq(i));
-			if (load < min_load || (load == min_load && i == this_cpu)) {
+			if (load < min_load) {
 				min_load = load;
 				least_loaded_cpu = i;
 			}

find_idlest_group_cpu goes through CPUs of a group previous selected by find_idlest_group. find_idlest_group returns NULL if the local group is the selected one and doesn't execute find_idlest_group_cpu if the group to which 'cpu' belongs to is chosen. So we're always guaranteed to call find_idlest_group_cpu with a group to which cpu is non-local. This makes one of

the conditions in find_idlest_group_cpu an impossible one, which we can get rid off.

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Brendan Jackman <[email protected]>

Cc Dietmar <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched/fair.c +1 -2

index 5c49fdb4c508..740602ce799f 100644
@@ -5922,7 +5922,7 @@ find_idlest_group_cpu(struct sched_group *group, struct task_struct *p, int this
                        }
                } else if (shallowest_idle_cpu == -1) {
                        load = weighted_cpuload(cpu_rq(i));
-                       if (load < min_load || (load == min_load && i == this_cpu)) {
+                       if (load < min_load) {
                                min_load = load;
                                least_loaded_cpu = i;
                        }
--

2.15.0.rc2.357.g7e34df9404-goog

Quoted 5 lines · Joel Fernandes

find_idlest_group_cpu goes through CPUs of a group previous selected by find_idlest_group. find_idlest_group returns NULL if the local group is the selected one and doesn't execute find_idlest_group_cpu if the group to which 'cpu' belongs to is chosen. So we're always guaranteed to call find_idlest_group_cpu with a group to which cpu is non-local. This makes one of

Is this still true in case of overlapping topology ?

Hi Vincent,

Quoted 8 lines · Vincent Guittot

Quoted 5 lines · Joel Fernandes

find_idlest_group_cpu goes through CPUs of a group previous selected by find_idlest_group. find_idlest_group returns NULL if the local group is the selected one and doesn't execute find_idlest_group_cpu if the group to which 'cpu' belongs to is chosen. So we're always guaranteed to call find_idlest_group_cpu with a group to which cpu is non-local. This makes one of

Is this still true in case of overlapping topology ?

Yes, I believe so. As per the code, find_idlest_group will only return a group to which this_cpu doesn't belong to. So incase an overlapping group was returned by find_idlest_group (instead of NULL), then none of the groups (among the set of overlapping groups) is local to this_cpu. Incase NULL is returned, then find_idlest_group_cpu doesn't execute at all.

Do you agree?

thanks,

- Joel

[..]