On 30 November 2016 at 15:24, Morten Rasmussen <[email protected]> wrote: > On Wed, Nov 30, 2016 at 02:54:00PM +0100, Vincent Guittot wrote: >> On 30 November 2016 at 14:49, Vincent Guittot >> <[email protected]> wrote: >> > On 30 November 2016 at 13:49, Morten Rasmussen <[email protected]> >> > wrote: >> >> On Fri, Nov 25, 2016 at 04:34:33PM +0100, Vincent Guittot wrote: >> >>> find_idlest_group() only compares the runnable_load_avg when looking for >> >>> the least loaded group. But on fork intensive use case like hackbench >> > >> > [snip] >> > >> >>> + min_avg_load = avg_load; >> >>> + idlest = group; >> >>> + } else if ((runnable_load < (min_runnable_load + >> >>> imbalance)) && >> >>> + (100*min_avg_load > >> >>> imbalance_scale*avg_load)) { >> >>> + /* >> >>> + * The runnable loads are close so we take >> >>> + * into account blocked load through >> >>> avg_load >> >>> + * which is blocked + runnable load >> >>> + */ >> >>> + min_avg_load = avg_load; >> >>> idlest = group; >> >>> } >> >>> >> >>> @@ -5470,13 +5495,16 @@ find_idlest_group(struct sched_domain *sd, >> >>> struct task_struct *p, >> >>> goto no_spare; >> >>> >> >>> if (this_spare > task_util(p) / 2 && >> >>> - imbalance*this_spare > 100*most_spare) >> >>> + imbalance_scale*this_spare > 100*most_spare) >> >>> return NULL; >> >>> else if (most_spare > task_util(p) / 2) >> >>> return most_spare_sg; >> >>> >> >>> no_spare: >> >>> - if (!idlest || 100*this_load < imbalance*min_load) >> >>> + if (!idlest || >> >>> + (min_runnable_load > (this_runnable_load + imbalance)) || >> >>> + ((this_runnable_load < (min_runnable_load + imbalance)) && >> >>> + (100*min_avg_load > >> >>> imbalance_scale*this_avg_load))) >> >> >> >> I don't get why you have imbalance_scale applied to this_avg_load and >> >> not min_avg_load. IIUC, you end up preferring non-local groups? >> > >> > In fact, I have keep the same condition that is used when looping the >> > group. >> > You're right that we should prefer local rq if avg_load are close and >> > test the condition >> > (100*this_avg_load > imbalance_scale*min_avg_load) instead >> >> Of course the correct condition is >> (100*this_avg_load < imbalance_scale*min_avg_load) > > Agreed, I should have read the entire thread before replying :-)
Interestingly, the original condition (100*min_avg_load > imbalance_scale*this_avg_load) gives better performance result for the hackbench test than the new one : (100*this_avg_load < imbalance_scale*min_avg_load) Matt, Have you been able to get some results for the patchset ? Vincent
