From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751408AbdJDAS3 (ORCPT ); Tue, 3 Oct 2017 20:18:29 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:19369 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750865AbdJDAS2 (ORCPT ); Tue, 3 Oct 2017 20:18:28 -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> <30520e34-20c9-e484-9a93-57bf33baa9d6@google.com> From: Rohit Jain Message-ID: <0e4151ea-4952-8384-4877-ce149577f2ef@oracle.com> Date: Tue, 3 Oct 2017 17:21:30 -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: 7bit 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 Hi Joel, On 10/02/2017 09:52 PM, Joel Fernandes wrote: > Hi Rohit, > > On Thu, Sep 28, 2017 at 8:09 AM, Rohit Jain wrote: > [..] >>>> 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. >>>> >>> I know what you're trying to do but they way you've retrofitted it into >>> the >>> core looks weird (to me) and makes the code unreadable and ugly IMO. >>> >>> Why not do something simpler like skip the core if any SMT thread has been >>> running at lesser capacity? I'm not sure if this works great or if the >>> maintainers >>> will prefer your or my below approach, but I find the below diff much >>> cleaner >>> for the select_idle_core bit. It also makes more sense since resources are >>> shared at SMT level so makes sense to me to skip the core altogether for >>> this: >>> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>> index 6ee7242dbe0a..f324a84e29f1 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -5738,14 +5738,17 @@ static int select_idle_core(struct task_struct *p, >>> struct sched_domain *sd, int >>> for_each_cpu_wrap(core, cpus, target) { >>> bool idle = true; >>> + bool full_cap = true; >>> for_each_cpu(cpu, cpu_smt_mask(core)) { >>> cpumask_clear_cpu(cpu, cpus); >>> if (!idle_cpu(cpu)) >>> idle = false; >>> + if (!full_capacity(cpu)) >>> + full_cap = false; >>> } >>> - if (idle) >>> + if (idle && full_cap) >>> return core; >>> } >>> >> >> >> Well, with your changes you will skip over fully idle cores which is not >> an ideal thing either. I see that you were advocating for select >> idle+lowest capacity core, whereas I was stopping at the first idlecore. >> >> Since the whole philosophy till now in this patch is "Don't spare an >> idle CPU", I think the following diff might look better to you. Please >> note this is only for discussion sakes, I haven't fully tested it yet. >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index ec15e5f..c2933eb 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -6040,7 +6040,9 @@ 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, backup_core; >> + >> + rcpu = backup_core = -1; >> >> if (!static_branch_likely(&sched_smt_present)) >> return -1; >> @@ -6052,15 +6054,34 @@ static int select_idle_core(struct task_struct *p, >> struct sched_domain *sd, int >> >> for_each_cpu_wrap(core, cpus, target) { >> bool idle = true; >> + bool full_cap = true; >> >> for_each_cpu(cpu, cpu_smt_mask(core)) { >> cpumask_clear_cpu(cpu, cpus); >> if (!idle_cpu(cpu)) >> idle = false; >> + >> + if (!full_capacity(cpu)) { >> + full_cap = false; >> + } >> } >> >> - if (idle) >> + if (idle && full_cap) >> return core; >> + else if (idle && backup_core == -1) >> + backup_core = core; >> + } >> + >> + if (backup_core != -1) { >> + for_each_cpu(cpu, cpu_smt_mask(backup_core)) { >> + if (full_capacity(cpu)) >> + return cpu; >> + else if ((rcpu == -1) || >> + (capacity_of(cpu) > capacity_of(rcpu))) >> + rcpu = cpu; >> + } >> + >> + return rcpu; >> } >> >> >> Do let me know what you think. > I think that if there isn't a benefit in your tests in doing the above > vs the simpler approach, then I prefer the simpler approach especially > since there's no point/benefit in complicating the code for > select_idle_core. Fair enough! If there are no more concerns in this version, then I will go ahead and try out all that is discussed in this version and send an updated version. Please let me know if there are any other concerns/feedback. Thanks, Rohit > > thanks, > > - Joel