Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755545Ab3HWLiL (ORCPT ); Fri, 23 Aug 2013 07:38:11 -0400 Received: from mail-qc0-f182.google.com ([209.85.216.182]:36688 "EHLO mail-qc0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753861Ab3HWLiH (ORCPT ); Fri, 23 Aug 2013 07:38:07 -0400 MIME-Version: 1.0 In-Reply-To: <20130822104257.GH31370@twins.programming.kicks-ass.net> References: <20130819160058.539049611@infradead.org> <20130819160425.157603641@infradead.org> <20130822104257.GH31370@twins.programming.kicks-ass.net> From: Paul Turner Date: Fri, 23 Aug 2013 04:37:35 -0700 Message-ID: Subject: Re: [PATCH 02/10] sched: Factor out code to should_we_balance() To: Peter Zijlstra Cc: Ingo Molnar , Joonsoo Kim , LKML , Mike Galbraith , Alex Shi , Preeti U Murthy , Vincent Guittot , Morten Rasmussen , Namhyung Kim , Lei Wen , Rik van Riel , Joonsoo Kim Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3126 Lines: 94 On Thu, Aug 22, 2013 at 3:42 AM, Peter Zijlstra wrote: > On Thu, Aug 22, 2013 at 02:58:27AM -0700, Paul Turner wrote: >> On Mon, Aug 19, 2013 at 9:01 AM, Peter Zijlstra wrote: > >> > + if (local_group) >> > load = target_load(i, load_idx); >> >> Keep the braces here: >> >> if (local_group) { >> load = target_load(i, load_idx); >> } else { >> ... > > Right you are, I misplaced that hunk in the next patch. > >> > - } else { >> > + else { >> > load = source_load(i, load_idx); >> > if (load > max_cpu_load) >> > max_cpu_load = load; > > >> > @@ -5123,12 +5120,11 @@ static int load_balance(int this_cpu, st >> > >> > schedstat_inc(sd, lb_count[idle]); >> > >> > -redo: >> > - group = find_busiest_group(&env, balance); >> > - >> > - if (*balance == 0) >> > + if (!(*should_balance = should_we_balance(&env))) >> > goto out_balanced; >> >> Given we always initialize *should_balance where we care, it might be >> more readable to write this as: > > Ah, but we don't actually, idle_balance() doesn't initialize > should_balance -- easy enough to fix though. I should have been more explicit here. idle_balance() doesn't initialize it, but it completely ignores the result; it's just passing something in case a write occurs. (Arguably it should be int unused = 1 instead of int balance = 1) > >> if (!should_we_balance(&env)) { >> *continue_balancing = 0; >> goto out_balanced; >> } >> >> [ With a rename to can_balance ] > > You confused me, your code implied > %s/should_balance/continue_balancing/g but now you suggest > %%s/should_balance/can_balance/g ? > > I'm fine with either. My bad, I started typing this comment, then went and looked at other parts of the patch and came back to it :) I think continue_balancing is more clear. > >> > >> > +redo: >> >> One behavioral change worth noting here is that in the redo case if a >> CPU has become idle we'll continue trying to load-balance in the >> !new-idle case. >> >> This could be unpleasant in the case where a package has a pinned busy >> core allowing this and a newly idle cpu to start dueling for load. >> >> While more deterministically bad in this case now, it could racily do >> this before anyway so perhaps not worth worrying about immediately. > > Ah, because the old code would effectively redo the check and find the > idle cpu and thereby our cpu would no longer be the balance_cpu. > > Indeed. And I don't think this was an intentional change. I'll go put > the redo back before should_we_balance(). > I was trying to get through the rest of these today but I'm out of time. I should be able to finish reviewing the other patches in the set tomorrow. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/