Hi Rohit,
Just some comments:
On Mon, Sep 25, 2017 at 5:02 PM, Rohit Jain <rohit.k.j...@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.j...@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
[...]
