Messages in this thread Patch in this message | | | Date | Fri, 25 Jul 2014 10:46:55 -0400 | From | Rik van Riel <> | Subject | Re: [PATCH] sched: make update_sd_pick_busiest return true on a busier sd |
| |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 07/25/2014 10:29 AM, Vincent Guittot wrote: > On 25 July 2014 15:33, Rik van Riel <riel@redhat.com> wrote: On > 07/23/2014 03:41 AM, Vincent Guittot wrote: >>>> On 22 July 2014 20:45, Rik van Riel <riel@redhat.com> wrote: >>>>> Currently update_sd_pick_busiest only returns true when an >>>>> sd is overloaded, or for SD_ASYM_PACKING when a domain is >>>>> busier than average and a higher numbered domain than the >>>>> target. >>>>> >>>>> This breaks load balancing between domains that are not >>>>> overloaded, in the !SD_ASYM_PACKING case. This patch makes >>>>> update_sd_pick_busiest return true when the busiest sd yet >>>>> is encountered. >>>>> >>>>> On a 4 node system, this seems to result in the load >>>>> balancer finally putting 1 thread of a 4 thread test run of >>>>> "perf bench numa mem" on each node, where before the load >>>>> was generally not spread across all nodes. >>>>> >>>>> Behaviour for SD_ASYM_PACKING does not seem to match the >>>>> comment, in that groups with below average load average >>>>> are ignored, but I have no hardware to test that so I have >>>>> left the behaviour of that code unchanged. >>>>> >>>>> Cc: mikey@neuling.org Cc: peterz@infradead.org >>>>> Signed-off-by: Rik van Riel <riel@redhat.com> --- >>>>> kernel/sched/fair.c | 18 +++++++++++------- 1 file changed, >>>>> 11 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>>>> index fea7d33..ff4ddba 100644 --- a/kernel/sched/fair.c >>>>> +++ b/kernel/sched/fair.c @@ -5942,16 +5942,20 @@ static >>>>> bool update_sd_pick_busiest(struct lb_env *env, * numbered >>>>> CPUs in the group, therefore mark all groups * higher than >>>>> ourself as busy. */ - if ((env->sd->flags & >>>>> SD_ASYM_PACKING) && sgs->sum_nr_running && - >>>>> env->dst_cpu < group_first_cpu(sg)) { - if >>>>> (!sds->busiest) - return true; + if (env->sd->flags & >>>>> SD_ASYM_PACKING) { + if (sgs->sum_nr_running && >>>>> env->dst_cpu < group_first_cpu(sg)) { + >>>>> if (!sds->busiest) + return true; >>>>> >>>>> - if (group_first_cpu(sds->busiest) > >>>>> group_first_cpu(sg)) - return true; >>>>> + if (group_first_cpu(sds->busiest) > group_first_cpu(sg)) >>>>> + return true; + } + + return >>>>> false; } >>>>> >>>>> - return false; + /* See above: sgs->avg_load >>>>> > sds->busiest_stat.avg_load */ + return true; >>>> >>>> Hi Rik, >>>> >>>> I can see one issue with a default return set to true. You >>>> increase the number of time where we will not effectively >>>> migrate a task because we don't ensure that we will take the >>>> overloaded group if there is one. We can be in a situation >>>> where a group is overloaded but the load_balance will select >>>> a not overloaded group with an average load higher than >>>> sched_domain average value just because it is checked after. > > Look at the first line of update_sd_pick_busiest() > > static bool update_sd_pick_busiest(struct lb_env *env, struct > sd_lb_stats *sds, struct sched_group *sg, struct sg_lb_stats *sgs) > { if (sgs->avg_load <= sds->busiest_stat.avg_load) return false; > > If the group load is less than the busiest, we have already > returned false, and will not get to the code that I changed. > > >> My point was that if a sched_group A with 1 task has got a >> higher avg_load than a sched_group with 2 tasks, we will select >> sched_group A whereas we should select the other group
The code already does that, with or without my patch.
If it runs into group A first, that "return false" above will be hit for group B.
>> Furthermore, update_sd_lb_stats will always return a busiest >> group even an idle one. This will increase the number of failed >> load balance and the time spent in the it.
If the busiest group found is idle, surely find_busiest_group will see that and goto out_balanced ?
There are several safety checks in find_busiest_group to make sure NULL is returned when the imbalance found is too small to bother doing anything about.
- -- All rights reversed -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJT0m3fAAoJEM553pKExN6D1xMH/3BOTeD9aiu/nVCWnfJIdRUB 2owogPLFDlcbUekzGkjIxc4lYWj/ANVk5jOibcOLutckSJXphDc1KhqYYXaNM6rm NfoFM0//HGxJgIWMKGWYWJosPdFdrvLxNRwn8+yBnZh9el15GQSBvhKjrCeolNo2 Yy6AqicqvoMXMnzcONcAxyxwH0b6CRFuGHIOAvzjXGUvSKTU7fs1zPRAVxfORbbp HfiOybqNJW+EnH7xhJU+GSZi+X+agnRS4/axfc48FZH01/P+k21cYougC7kMxxHA MGP1YtnNYGFBqCX5QwGgw5NkMgHYNCCREh+uLwsgGCN/bkmuHJm+JcbQOQlRoRY= =v9O8 -----END PGP SIGNATURE-----
| |