On 27 March 2015 at 23:30, Peter Zijlstra <[email protected]> wrote: > On Wed, Mar 18, 2015 at 02:31:02PM +0800, Xunlei Pang wrote: >> From: Xunlei Pang <[email protected]> >> >> In load_balance(), some members of lb_env will be assigned with >> new values in LBF_DST_PINNED case. But lb_env::flags may still >> retain LBF_ALL_PINNED if no proper tasks were found afterwards >> due to another balance, task affinity changing, etc, which can >> really happen because busiest rq lock has already been released. > > Sure.. > >> This is wrong, for example with env.dst_cpu assigned new_dst_cpu >> when going back to "redo" label, it may cause should_we_balance() >> to return false which is unreasonable. > > Why? You've got a very unlikely, very hard case, its unlikely that > anything we do will substantially improve the situation, but you make > the code uglier for it. > >> This patch restores proper status of env before "goto redo", and >> improves "out_all_pinned" and "out_one_pinned" labels. > > That doesn't even begin to explain half of what the patch does. > >> @@ -6977,12 +6978,19 @@ more_balance: >> /* All tasks on this runqueue were pinned by CPU affinity */ >> if (unlikely(env.flags & LBF_ALL_PINNED)) { >> cpumask_clear_cpu(cpu_of(busiest), cpus); >> - if (!cpumask_empty(cpus)) { >> - env.loop = 0; >> - env.loop_break = sched_nr_migrate_break; >> - goto redo; >> + if (env.new_dst_cpu != -1) { > > I really don't get this, how can this not be? > >> + env.new_dst_cpu = -1; >> + cpumask_or(cpus, cpus, >> + sched_group_cpus(sd->groups)); >> + cpumask_and(cpus, cpus, cpu_active_mask); > > More unexplained magic, why is this right?
When LBF_DST_PINNED was set, after going back to "more_balance", things may change as the changelog describes, so it can hit LBF_ALL_PINNED afterwards. Then env.cpus, env.dst_rq, env.dst_cpu held the values assigned in the LBF_DST_PINNED case which is unreasonable. When we want to redo, we must reset those values. > > The rest of the patch isn't much better. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
