Re: [Eas-dev] [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path

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 rohit.k.jain@oracle.com 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