From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970245AbdIZTp3 (ORCPT ); Tue, 26 Sep 2017 15:45:29 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:21067 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966436AbdIZTp1 (ORCPT ); Tue, 26 Sep 2017 15:45:27 -0400 Subject: Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path To: Joel Fernandes Cc: LKML , eas-dev@lists.linaro.org, Peter Zijlstra , Ingo Molnar , Atish Patra , Vincent Guittot , Dietmar Eggemann , Morten Rasmussen References: <1506384126-2862-1-git-send-email-rohit.k.jain@oracle.com> <1506384126-2862-3-git-send-email-rohit.k.jain@oracle.com> From: Rohit Jain Message-ID: Date: Tue, 26 Sep 2017 12:48:25 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/25/2017 11:53 PM, Joel Fernandes wrote: > Hi Rohit, > > Just some comments: Hi Joel, Thanks for the comments. > On Mon, Sep 25, 2017 at 5:02 PM, Rohit Jain wrote: >> While looking for CPUs to place running tasks on, the scheduler >> completely ignores the capacity stolen away by RT/IRQ tasks. >> >> This patch fixes that. >> >> Signed-off-by: Rohit Jain >> --- >> kernel/sched/fair.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 43 insertions(+), 11 deletions(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index afb701f..19ff2c3 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -6040,7 +6040,10 @@ void __update_idle_core(struct rq *rq) >> static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int target) >> { >> struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_idle_mask); >> - int core, cpu; >> + int core, cpu, rcpu, rcpu_backup; > I would call rcpu_backup as backup_cpu. OK > >> + unsigned int backup_cap = 0; >> + >> + rcpu = rcpu_backup = -1; >> >> if (!static_branch_likely(&sched_smt_present)) >> return -1; >> @@ -6057,10 +6060,20 @@ static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int >> cpumask_clear_cpu(cpu, cpus); >> if (!idle_cpu(cpu)) >> idle = false; >> + >> + if (full_capacity(cpu)) { >> + rcpu = cpu; >> + } else if ((rcpu == -1) && (capacity_of(cpu) > backup_cap)) { >> + backup_cap = capacity_of(cpu); >> + rcpu_backup = cpu; >> + } > Here you comparing capacity of different SMT threads. > >> } >> >> - if (idle) >> - return core; >> + if (idle) { >> + if (rcpu == -1) >> + return (rcpu_backup != -1 ? rcpu_backup : core); >> + return rcpu; >> + } > > This didn't make much sense to me, here you are returning either an > SMT thread or a core. That doesn't make much of a difference because > SMT threads share the same capacity (SD_SHARE_CPUCAPACITY). I think > what you want to do is find out the capacity of a 'core', not an SMT > thread, and compare the capacity of different cores and consider the > one which has least RT/IRQ interference. IIUC the capacities of each strand is scaled by IRQ and 'rt_avg' for that 'rq'. Now if the strand is idle now and gets an interrupt in the future, the 'core' would look like:    +----+----+    | I  |    |    | T  |    |    +----+----+ (I -> Interrupt, T-> Thread we are trying to schedule). whereas if the other strand on the core was taking interrupt the core would look like:    +----+----+    | I  | T  |    |    |    |    +----+----+ With this case, because we know from the past avg, one of the strands is running low on capacity, I am trying to return a better strand for the thread to start on. > >> } >> >> /* >> @@ -6076,7 +6089,8 @@ static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int >> */ >> static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int target) >> { >> - int cpu; >> + int cpu, backup_cpu = -1; >> + unsigned int backup_cap = 0; >> >> if (!static_branch_likely(&sched_smt_present)) >> return -1; >> @@ -6084,11 +6098,17 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t >> for_each_cpu(cpu, cpu_smt_mask(target)) { >> if (!cpumask_test_cpu(cpu, &p->cpus_allowed)) >> continue; >> - if (idle_cpu(cpu)) >> - return cpu; >> + if (idle_cpu(cpu)) { >> + if (full_capacity(cpu)) >> + return cpu; >> + if (capacity_of(cpu) > backup_cap) { >> + backup_cap = capacity_of(cpu); >> + backup_cpu = cpu; >> + } >> + } > Same thing here, since SMT threads share the same underlying capacity, > is there any point in comparing the capacities of each SMT thread? See above Thanks, Rohit > > thanks, > > - Joel > > [...]