sched/fair: Clean up scale confusion

tip:sched/core Yuyang Du 3 participants 4 messages May 13, 2016 – May 23, 2016 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Yuyang Du

author

2016-05-13 03:23 UTC #1 lore raw

Quoted 14 lines · tip-bot for Peter Zijlstra

Commit-ID:  1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Gitweb:     http://git.kernel.org/tip/1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 6 May 2016 12:21:23 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 May 2016 09:55:33 +0200
sched/fair: Clean up scale confusion
Wanpeng noted that the scale_load_down() in calculate_imbalance() was weird. I agree, it should be SCHED_CAPACITY_SCALE, since we're going to compare against busiest->group_capacity, which is in [capacity] units.

Quoted 10 lines

Reported-by Wanpeng Li <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Morten Rasmussen <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Yuyang Du <[email protected]>
Cc [email protected]
Signed-off-by Ingo Molnar <[email protected]>

It is good that this issue is addressed and patch merged, however, for the record, Vincent has already had a solution for this, and we had a patch, including other cleanups (the latest version is: https://lkml.org/lkml/2016/5/3/925).
And I think Ben first pointed this out (and we then attempted to address it) as far as I can tell.

Vincent Guittot

2016-05-13 07:24 UTC #2 lore raw

Quoted 8 lines · Yuyang Du

Quoted 7 lines · tip-bot for Peter Zijlstra

Commit-ID:  1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Gitweb:     http://git.kernel.org/tip/1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 6 May 2016 12:21:23 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 May 2016 09:55:33 +0200

Quoted 6 lines

Quoted 6 lines
sched/fair: Clean up scale confusion
Wanpeng noted that the scale_load_down() in calculate_imbalance() was weird. I agree, it should be SCHED_CAPACITY_SCALE, since we're going to compare against busiest->group_capacity, which is in [capacity] units.

In fact, load_above_capacity is only about load and not about capacity.

load_above_capacity -= busiest->group_capacity is an optimization (may be a wronf one) of
load_above_capacity -= busiest->group_capacity * SCHED_LOAD_SCALE / SCHED_CAPACITY_SCALE

so we subtract load to load

Quoted 17 lines

Quoted 11 lines
Reported-by Wanpeng Li <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Morten Rasmussen <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Yuyang Du <[email protected]>
Cc [email protected]
Signed-off-by Ingo Molnar <[email protected]>
It is good that this issue is addressed and patch merged, however, for the record, Vincent has already had a solution for this, and we had a patch, including other cleanups (the latest version is: https://lkml.org/lkml/2016/5/3/925).
And I think Ben first pointed this out (and we then attempted to address it) as far as I can tell.

Morten Rasmussen

2016-05-20 10:11 UTC #3 lore raw

Quoted 16 lines · Vincent Guittot

Quoted 8 lines · Yuyang Du
Quoted 7 lines · tip-bot for Peter Zijlstra
Commit-ID:  1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Gitweb:     http://git.kernel.org/tip/1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 6 May 2016 12:21:23 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 May 2016 09:55:33 +0200
Quoted 6 lines
Quoted 6 lines
sched/fair: Clean up scale confusion
Wanpeng noted that the scale_load_down() in calculate_imbalance() was weird. I agree, it should be SCHED_CAPACITY_SCALE, since we're going to compare against busiest->group_capacity, which is in [capacity] units.

Quoted 8 lines

In fact, load_above_capacity is only about load and not about capacity.
load_above_capacity -= busiest->group_capacity is an optimization (may be a wronf one) of
load_above_capacity -= busiest->group_capacity * SCHED_LOAD_SCALE / SCHED_CAPACITY_SCALE
so we subtract load to load

I like your approach as you compute the desired minimum load, which is essentially finding the number of NICE_0_LOAD task we want in the group, and then determine how much excess load there is. So it becomes quite clear that it is load.

While it preserves existing behaviour I would question the whole NICE_0_LOAD assumption. It totally falls apart with PELT and if we have tasks with nice != 0.

Also, it doesn't address the existing unit issue as load_above_capacity is later multiplied by busiest->group_capacity when computing the imbalance. As said in the other thread, we should either kill the minimum load estimation that assumes always-running NICE_0_LOAD tasks, or at least make sure the scaling of load_above_capacity is correct. Patches attempting either solution are in the other thread.

Vincent Guittot

2016-05-23 10:28 UTC #4 lore raw

Quoted 26 lines · Morten Rasmussen

Quoted 16 lines · Vincent Guittot
Quoted 8 lines · Yuyang Du
Quoted 7 lines · tip-bot for Peter Zijlstra
Commit-ID:  1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Gitweb:     http://git.kernel.org/tip/1be0eb2a97d756fb7dd8c9baf372d81fa9699c09
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 6 May 2016 12:21:23 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 May 2016 09:55:33 +0200
Quoted 6 lines
Quoted 6 lines
sched/fair: Clean up scale confusion
Wanpeng noted that the scale_load_down() in calculate_imbalance() was weird. I agree, it should be SCHED_CAPACITY_SCALE, since we're going to compare against busiest->group_capacity, which is in [capacity] units.
Quoted 8 lines
In fact, load_above_capacity is only about load and not about capacity.
load_above_capacity -= busiest->group_capacity is an optimization (may be a wronf one) of
load_above_capacity -= busiest->group_capacity * SCHED_LOAD_SCALE / SCHED_CAPACITY_SCALE
so we subtract load to load

Quoted 8 lines

I like your approach as you compute the desired minimum load, which is essentially finding the number of NICE_0_LOAD task we want in the group, and then determine how much excess load there is. So it becomes quite clear that it is load.
While it preserves existing behaviour I would question the whole NICE_0_LOAD assumption. It totally falls apart with PELT and if we have tasks with nice != 0.

yes, i haven't found a simple use case yet to demonstrate the usefulness of this part

Quoted 7 lines

Also, it doesn't address the existing unit issue as load_above_capacity is later multiplied by busiest->group_capacity when computing the imbalance. As said in the other thread, we should either kill the minimum load estimation that assumes always-running NICE_0_LOAD tasks, or at least make sure the scaling of load_above_capacity is correct. Patches attempting either solution are in the other thread.