Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757030AbcC2MTb (ORCPT ); Tue, 29 Mar 2016 08:19:31 -0400 Received: from casper.infradead.org ([85.118.1.10]:46445 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756788AbcC2MT3 (ORCPT ); Tue, 29 Mar 2016 08:19:29 -0400 Date: Tue, 29 Mar 2016 14:19:24 +0200 From: Peter Zijlstra To: Srikar Dronamraju Cc: Ingo Molnar , linux-kernel@vger.kernel.org, Gautham R Shenoy , Michael Neuling , Vaidyanathan Srinivasan , tim.c.chen@linux.intel.com Subject: Re: [PATCH 1/3] sched/fair: Fix asym packing to select correct cpu Message-ID: <20160329121924.GH3408@twins.programming.kicks-ass.net> References: <1458732880-28382-1-git-send-email-srikar@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458732880-28382-1-git-send-email-srikar@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2753 Lines: 78 On Wed, Mar 23, 2016 at 05:04:40PM +0530, Srikar Dronamraju wrote: > If asymmetric packing is used when target cpu is busy, > update_sd_pick_busiest(), can select a lightly loaded cpu. > find_busiest_group() has checks to ensure asym packing is only used > when target cpu is not busy. However it may not be able to avoid a > lightly loaded cpu selected by update_sd_pick_busiest from being > selected as source cpu for eventual load balancing. So my brain completely fails to parse. What? Why? > Also when using asymmetric scheduling, always select higher cpu as > source cpu for load balancing. > > While doing this change, move the check to see if target cpu is busy > into check_asym_packing(). > Signed-off-by: Srikar Dronamraju > --- > kernel/sched/fair.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 56b7d4b..9abfb16 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -6517,6 +6517,8 @@ static bool update_sd_pick_busiest(struct lb_env *env, > if (!(env->sd->flags & SD_ASYM_PACKING)) > return true; > > + if (env->idle == CPU_NOT_IDLE) > + return true; OK, so this matches check_asym_packing() and makes sense, we don't want to pull work if we're not idle. But please add a comment with the condition, its always hard to remember the intent of these things later. > /* > * ASYM_PACKING needs to move all the work to the lowest > * numbered CPUs in the group, therefore mark all groups > @@ -6526,7 +6528,7 @@ static bool update_sd_pick_busiest(struct lb_env *env, > if (!sds->busiest) > return true; > > - if (group_first_cpu(sds->busiest) > group_first_cpu(sg)) > + if (group_first_cpu(sds->busiest) < group_first_cpu(sg)) > return true; > } Right, so you want to start by moving the highest possible cpu's work down. The end result is the same, but this way you can reach lower power states quicker. Again, please add a comment. > @@ -6672,6 +6674,9 @@ static int check_asym_packing(struct lb_env *env, struct sd_lb_stats *sds) > if (!(env->sd->flags & SD_ASYM_PACKING)) > return 0; > > + if (env->idle == CPU_NOT_IDLE) > + return 0; > + > if (!sds->busiest) > return 0; > > @@ -6864,8 +6869,7 @@ static struct sched_group *find_busiest_group(struct lb_env *env) > busiest = &sds.busiest_stat; > > /* ASYM feature bypasses nice load balance check */ > - if ((env->idle == CPU_IDLE || env->idle == CPU_NEWLY_IDLE) && > - check_asym_packing(env, &sds)) > + if (check_asym_packing(env, &sds)) > return sds.busiest; > > /* There is no busy sibling group to pull tasks from */ OK, this is an effective NOP but results in cleaner code.