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

Hi Rohit,

Just some comments:

On Mon, Sep 25, 2017 at 5:02 PM, Rohit Jain rohit.k.jain@oracle.com 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 rohit.k.jain@oracle.com

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.

•   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.

    }

    /*

@@ -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?

thanks,

- Joel

[...]