From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753264AbdI1PKA (ORCPT ); Thu, 28 Sep 2017 11:10:00 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:32731 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752013AbdI1PJ7 (ORCPT ); Thu, 28 Sep 2017 11:09:59 -0400 Subject: Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path To: joelaf 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: Date: Thu, 28 Sep 2017 10:09:14 -0500 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: <30520e34-20c9-e484-9a93-57bf33baa9d6@google.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Joel, On 09/28/2017 05:53 AM, joelaf wrote: > Hi Rohit, > > On Tue, Sep 26, 2017 at 12:48 PM, Rohit Jain wrote: > [...] > >>>> } >>>> >>>> - 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. >> > 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. Thanks, Rohit > > thanks, > > - Joel >